Add unit tests for getConfigResources in ModuleDefinitionSet and improve context readability #11042

he1l0world · 2025-06-17T05:22:43Z

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)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)
build/CI
test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

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 · 2025-06-17T06:55:26Z

@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 · 2025-06-17T06:56:57Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 16.60%. Comparing base (f8c4121) to head (390ac7b).

Additional details and impacted files

@@            Coverage Diff            @@
##               main   #11042   +/-   ##
=========================================
  Coverage     16.60%   16.60%           
- Complexity    13925    13926    +1     
=========================================
  Files          5730     5730           
  Lines        508254   508254           
  Branches      61789    61789           
=========================================
+ Hits          84387    84390    +3     
+ Misses       414431   414430    -1     
+ Partials       9436     9434    -2

Flag	Coverage Δ
uitests	`3.93% <ø> (ø)`
unittests	`17.49% <ø> (+<0.01%)`	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

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

he1l0world · 2025-06-17T07:07:49Z

@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

clgtm

DaanHoogland · 2025-06-17T16:42:26Z

@blueorangutan package

blueorangutan · 2025-06-17T16:44:03Z

@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 · 2025-06-17T18:08:51Z

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

he1l0world · 2025-06-20T05:25:54Z

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

DaanHoogland · 2025-06-23T08:25:31Z

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 · 2025-06-23T22:39:41Z

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 · 2025-06-23T22:47:05Z

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

he1l0world · 2025-06-26T19:57:02Z

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 · 2025-06-27T12:42:23Z

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 · 2025-06-27T15:36:06Z

[SF] Trillian Build Failed (tid-13619)

JoaoJandre · 2025-06-27T16:54:05Z

@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 · 2025-06-27T20:02:00Z

@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 · 2025-06-30T07:34:45Z

[SF] Trillian Build Failed (tid-13635)

blueorangutan · 2025-06-30T08:14:41Z

[SF] Trillian Build Failed (tid-13636)

DaanHoogland · 2025-06-30T10:49:39Z

@blueorangutan package

blueorangutan · 2025-06-30T10:52:03Z

@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 · 2025-06-30T12:25:23Z

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

blueorangutan · 2025-06-30T19:15:25Z

[SF] Trillian Build Failed (tid-13645)

JoaoJandre · 2025-07-01T12:15:04Z

@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 · 2025-07-01T16:55:48Z

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 · 2025-07-02T03:13:33Z

[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

JoaoJandre

Tests look ok to me. Still wish they where renamed but I will not block the PR because of it.

DaanHoogland · 2025-07-03T13:27:05Z

@blueorangutan package

blueorangutan · 2025-07-03T13:28:06Z

@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 · 2025-07-03T14:50:46Z

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

he1l0world · 2025-07-05T02:48:23Z

Thanks again for reviewing, @JoaoJandre !

blueorangutan · 2025-07-07T23:14:52Z

[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

he1l0world added 2 commits June 17, 2025 03:15

add test cases for getConfigResources

ee994b3

split tests for each cases

390ac7b

he1l0world force-pushed the test/context-factory-test branch from c3a797d to 390ac7b Compare June 17, 2025 07:18

DaanHoogland approved these changes Jun 17, 2025

View reviewed changes

he1l0world mentioned this pull request Jun 17, 2025

Improve Inherited Context Lookup in getConfigResources #11048

Open

14 tasks

DaanHoogland assigned sureshanaparti Jun 24, 2025

JoaoJandre approved these changes Jul 3, 2025

View reviewed changes

Add unit tests for getConfigResources in ModuleDefinitionSet and improve context readability #11042

Are you sure you want to change the base?

Add unit tests for getConfigResources in ModuleDefinitionSet and improve context readability #11042

Uh oh!

Conversation

he1l0world commented Jun 17, 2025

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

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

Uh oh!

DaanHoogland commented Jun 17, 2025

Uh oh!

codecov bot commented Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

he1l0world commented Jun 17, 2025

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Jun 17, 2025

Uh oh!

blueorangutan commented Jun 17, 2025

Uh oh!

blueorangutan commented Jun 17, 2025

Uh oh!

he1l0world commented Jun 20, 2025

Uh oh!

DaanHoogland commented Jun 23, 2025

Uh oh!

he1l0world commented Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

he1l0world commented Jun 23, 2025

Uh oh!

he1l0world commented Jun 26, 2025

Uh oh!

JoaoJandre commented Jun 27, 2025

Uh oh!

blueorangutan commented Jun 27, 2025

Uh oh!

JoaoJandre commented Jun 27, 2025

Uh oh!

he1l0world commented Jun 27, 2025

Uh oh!

blueorangutan commented Jun 30, 2025

Uh oh!

blueorangutan commented Jun 30, 2025

Uh oh!

DaanHoogland commented Jun 30, 2025

Uh oh!

blueorangutan commented Jun 30, 2025

Uh oh!

blueorangutan commented Jun 30, 2025

Uh oh!

blueorangutan commented Jun 30, 2025

Uh oh!

JoaoJandre commented Jul 1, 2025

Uh oh!

he1l0world commented Jul 1, 2025

Uh oh!

blueorangutan commented Jul 2, 2025

Uh oh!

JoaoJandre left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Jul 3, 2025

Uh oh!

blueorangutan commented Jul 3, 2025

Uh oh!

blueorangutan commented Jul 3, 2025

Uh oh!

he1l0world commented Jul 5, 2025

Uh oh!

codecov bot commented Jun 17, 2025 •

edited

Loading

he1l0world commented Jun 23, 2025 •

edited

Loading