Add unit tests for getConfigResources in ModuleDefinitionSet and improve context readability

[he1l0world]

Description

This PR adds unit test coverage for the getConfigResources() method in the ModuleDefinitionSet class to validate context resource logic. This ensures modules are loading the correct Spring context files, including contexts, inherited contexts, and overridden contexts.

To improve readability and avoid confusion, some Spring context XML files were renamed to match their associated modules more intuitively.

Additionally, a small test refactor was made: the instantiation of ModuleBasedContextFactory was moved to the top of the test class as a shared field, since the factory is stateless and does not need to be recreated for each test case

Types of changes

• [ ] Breaking change (fix or feature that would cause existing functionality to change)
• [ ] New feature (non-breaking change which adds functionality)
• [ ] Bug fix (non-breaking change which fixes an issue)
• [X] Enhancement (improves an existing feature and functionality)
• [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
• [ ] build/CI
• [X] test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

• [ ] Major
• [X] Minor

Bug Severity

• [ ] BLOCKER
• [ ] Critical
• [ ] Major
• [ ] Minor
• [ ] Trivial

Screenshots (if appropriate):

Not applicable

How Has This Been Tested?

Added testConfigResources() in ModuleBasedContextFactoryTest.java with assertions for expected Spring resource files.

How did you try to break this feature and the system with this change?

• Provided intentionally incorrect module names to verify fallbacks.

[DaanHoogland]

@he1l0world , your change looks generally good to but one remarks: You have a base test method but then call it from the same test method several times, these seem different test cases to me. Does it make sense to split them?

[codecov[bot]]

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 16.58%. Comparing base (f8c4121) to head (bbd30a3). :warning: Report is 79 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #11042      +/-   ##
============================================
- Coverage     16.60%   16.58%   -0.03%     
- Complexity    13925    13988      +63     
============================================
  Files          5730     5745      +15     
  Lines        508254   510847    +2593     
  Branches      61789    62140     +351     
============================================
+ Hits          84387    84708     +321     
- Misses       414431   416668    +2237     
- Partials       9436     9471      +35     

Flag	Coverage Δ
uitests	3.91% <ø> (-0.03%)	:arrow_down:
unittests	17.47% <ø> (-0.02%)	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[he1l0world]

@he1l0world , your change looks generally good to but one remarks: You have a base test method but then call it from the same test method several times, these seem different test cases to me. Does it make sense to split them?

Yes, it makes sense to me. I will update them

[DaanHoogland]

@blueorangutan package

[blueorangutan]

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

[blueorangutan]

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 13820

[he1l0world]

This failed test case seems unrelated to the changes, but I just want to confirm. Let me know if you think otherwise.

[DaanHoogland]

This failed test case seems unrelated to the changes, but I just want to confirm. Let me know if you think otherwise.

it does @he1l0world , we just need a second review for merging. If no-one volunteers you might want to look at some who worked on related code recently and ask them.

[he1l0world]

Sounds good! I’ll loop in @sureshanaparti here, I noticed you’ve worked on similar code in CloudStackSpringContext.java recently and was wondering if you’d be open to reviewing this change. Really appreciate your time, thank you!

This failed test case seems unrelated to the changes, but I just want to confirm. Let me know if you think otherwise.

it does @he1l0world , we just need a second review for merging. If no-one volunteers you might want to look at some who worked on related code recently and ask them.

[he1l0world]

Most of the changes in this code were made quite a while ago...

[he1l0world]

Hi @JoaoJandre, I noticed you’ve worked on similar code before. Would you be open to reviewing this change when you get a chance? Appreciate it!

[JoaoJandre]

Hi @JoaoJandre, I noticed you’ve worked on similar code before. Would you be open to reviewing this change when you get a chance? Appreciate it!

Hello @he1l0world, sure, I'll try to review it soon

[blueorangutan]

[SF] Trillian Build Failed (tid-13619)

[JoaoJandre]

@he1l0world I'm not very familiar with the ModuleBasedContextFactory class. That being said, I think that the test naming you did is not very intuitive and makes it hard to understand what method and what you are testing. I would suggest you change your tests names to something like <methodName>Test<BehaviorBeingTested> so that it is clear from a quick glance what is being tested.

[he1l0world]

@JoaoJandre Thanks a lot for the feedback! I agree that test naming should be clear and intuitive.

From what I’ve seen, most tests in the Apache CloudStack codebase follow conventions like test<MethodName> or test<SomeBehavior>, without necessarily repeating the method name being tested if it's already implied. In this PR, all the tests are focused on getConfigResources, I tried to keep the names focused on the specific behaviors being tested to keep things readable.

That said, if you feel the <methodName>Test<BehaviorBeingTested> format would make things clearer, I’d be happy to rename the tests. Totally open to aligning with whatever the team prefers!

[blueorangutan]

[SF] Trillian Build Failed (tid-13635)

[blueorangutan]

[SF] Trillian Build Failed (tid-13636)

[DaanHoogland]

@blueorangutan package

[blueorangutan]

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

[blueorangutan]

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 13958

[blueorangutan]

[SF] Trillian Build Failed (tid-13645)

[JoaoJandre]

@JoaoJandre Thanks a lot for the feedback! I agree that test naming should be clear and intuitive.

From what I’ve seen, most tests in the Apache CloudStack codebase follow conventions like test<MethodName> or test<SomeBehavior>, without necessarily repeating the method name being tested if it's already implied. In this PR, all the tests are focused on getConfigResources, I tried to keep the names focused on the specific behaviors being tested to keep things readable.

That said, if you feel the <methodName>Test<BehaviorBeingTested> format would make things clearer, I’d be happy to rename the tests. Totally open to aligning with whatever the team prefers!

@he1l0world currently there is no set convention that you must abide by as far as I know. But it is my opinion that it would make the tests easier to understand.

In any case, on a first glance I did not see anything wrong/weird with your tests. I'll review them again soon just to be sure (as I said, I'm still not very familiar with this part of the code).

[he1l0world]

Thanks so much for the reply, @JoaoJandre. Really appreciate you taking the time!

That makes sense, totally understand your perspective on clarity. If it still feels unclear after your second look (or if others feel the same), I’m more than happy to rename the tests to make things clearer.

Let me know what you think after a second look!

[blueorangutan]

[SF] Trillian test result (tid-13651) Environment: kvm-ubuntu24 (x2), Advanced Networking with Mgmt server u24 Total time taken: 59793 seconds Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr11042-t13651-kvm-ubuntu24.zip Smoke tests completed. 137 look OK, 4 have errors, 0 did not run Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_oobm_issue_power_cycle	Error	2.33	test_outofbandmanagement_nestedplugin.py
test_oobm_issue_power_off	Error	2.31	test_outofbandmanagement_nestedplugin.py
test_oobm_issue_power_on	Error	2.34	test_outofbandmanagement_nestedplugin.py
test_oobm_issue_power_reset	Error	2.34	test_outofbandmanagement_nestedplugin.py
test_oobm_issue_power_soft	Error	2.39	test_outofbandmanagement_nestedplugin.py
test_oobm_issue_power_status	Error	2.38	test_outofbandmanagement_nestedplugin.py
test_oobm_background_powerstate_sync	Failure	20.98	test_outofbandmanagement.py
test_oobm_enabledisable_across_clusterzones	Error	29.32	test_outofbandmanagement.py
test_oobm_issue_power_cycle	Error	10.80	test_outofbandmanagement.py
test_oobm_issue_power_off	Error	9.72	test_outofbandmanagement.py
test_oobm_issue_power_on	Error	10.99	test_outofbandmanagement.py
test_oobm_issue_power_reset	Error	10.82	test_outofbandmanagement.py
test_oobm_issue_power_soft	Error	9.77	test_outofbandmanagement.py
test_oobm_issue_power_status	Error	10.80	test_outofbandmanagement.py
test_oobm_multiple_mgmt_server_ownership	Failure	175.94	test_outofbandmanagement.py
test_oobm_zchange_password	Error	4.38	test_outofbandmanagement.py
test_01_webhook_deliveries	Failure	10.62	test_webhook_delivery.py
test_hostha_kvm_host_degraded	Error	10.55	test_hostha_kvm.py
test_hostha_kvm_host_fencing	Error	7.80	test_hostha_kvm.py
test_hostha_kvm_host_recovering	Error	9.94	test_hostha_kvm.py

[DaanHoogland]

@blueorangutan package

[blueorangutan]

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

[blueorangutan]

Packaging result [SF]: ✔️ el8 ✔️ el9 ✖️ debian ✔️ suse15. SL-JID 14010

[he1l0world]

Thanks again for reviewing, @JoaoJandre !

[blueorangutan]

[SF] Trillian test result (tid-13698) Environment: kvm-ol9 (x2), Advanced Networking with Mgmt server ol9 Total time taken: 59391 seconds Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr11042-t13698-kvm-ol9.zip Smoke tests completed. 137 look OK, 4 have errors, 0 did not run Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
ContextSuite context=TestClusterDRS>:setup	Error	0.00	test_cluster_drs.py
test_list_system_vms_metrics_history	Failure	0.22	test_metrics_api.py
test_oobm_issue_power_cycle	Error	1.23	test_outofbandmanagement_nestedplugin.py
test_oobm_issue_power_off	Error	1.24	test_outofbandmanagement_nestedplugin.py
test_oobm_issue_power_on	Error	0.24	test_outofbandmanagement_nestedplugin.py
test_oobm_issue_power_reset	Error	2.27	test_outofbandmanagement_nestedplugin.py
test_oobm_issue_power_soft	Error	2.23	test_outofbandmanagement_nestedplugin.py
test_oobm_issue_power_status	Error	1.22	test_outofbandmanagement_nestedplugin.py
test_01_webhook_deliveries	Failure	9.05	test_webhook_delivery.py

[sureshanaparti]

Hi @he1l0world Can you check/address the comments. Thanks.