Added tests for cronfederatedhpa controller #5481

anujagrawal699 · 2024-09-04T18:18:50Z

Description:
This PR introduces tests for cronfederatedhpa controller. This PR tests the cronfederatedhpa_handler and some functions of cronfederatedhpa_job module. The cronfederatedhpa_controller module is harder and complex to test and we could approach it after completion of tests which are necessary, maintainable and easy.

Test Coverage:
The test coverage of the directory pkg/controllers/cronfederatedhpa has been increased from 0.00% to 27.8%.

What type of PR is this?
/kind documentation
/kind failing-test
/kind feature

What this PR does / why we need it:
This PR adds comprehensive tests for testing functionality of cronfederatedhpa controller.

Which issue(s) this PR fixes:
Fixes a part of #5470

Does this PR introduce a user-facing change?:

NONE

codecov-commenter · 2024-09-04T18:32:27Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 31.92%. Comparing base (7a2b493) to head (e648c1b).
Report is 70 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5481      +/-   ##
==========================================
+ Coverage   31.71%   31.92%   +0.20%     
==========================================
  Files         643      643              
  Lines       44429    44445      +16     
==========================================
+ Hits        14089    14187      +98     
+ Misses      29310    29220      -90     
- Partials     1030     1038       +8

Flag	Coverage Δ
unittests	`31.92% <ø> (+0.20%)`	⬆️

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.

anujagrawal699 · 2024-09-04T19:10:51Z

@zhzhuang-zju

XiShanYongYe-Chang · 2024-09-05T01:21:45Z

/assign

zhzhuang-zju · 2024-09-05T01:38:59Z

pkg/controllers/cronfederatedhpa/cronfederatedhpa_handler_test.go

+			name:        "New scale target",
+			cronFHPAKey: "default/test-cronhpa",
+			initialTarget: autoscalingv2.CrossVersionObjectReference{
+				Kind:       "Deployment",
+				Name:       "test-deployment",
+				APIVersion: "apps/v1",
+			},
+			updatedTarget: autoscalingv2.CrossVersionObjectReference{
+				Kind:       "Deployment",
+				Name:       "test-deployment",
+				APIVersion: "apps/v1",
+			},
+			expectedUpdate: false,
+		},
+		{
+			name:        "Same scale target",
+			cronFHPAKey: "default/test-cronhpa",
+			initialTarget: autoscalingv2.CrossVersionObjectReference{
+				Kind:       "Deployment",
+				Name:       "test-deployment",
+				APIVersion: "apps/v1",
+			},
+			updatedTarget: autoscalingv2.CrossVersionObjectReference{
+				Kind:       "Deployment",
+				Name:       "test-deployment",
+				APIVersion: "apps/v1",
+			},
+			expectedUpdate: false,


What's the difference between test case New scale target and Same scale target?

Yeah they are redundant. I thought to test both return statements with it

If there's no existing target for the given key, it stores the new target and returns false

If there is an existing target, it compares it with the new target and returns whether they are different.
I think we should update the New Scale Target test to start with no initial target in the handler's map.

maybe we can use different cronFHPAKey to achieve the effect of a new scale target

Okay, good idea. I think different keys also won't cause conflicts.

zhzhuang-zju · 2024-09-05T06:46:25Z

pkg/controllers/cronfederatedhpa/cronfederatedhpa_handler_test.go

+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			handler.CronFHPAScaleTargetRefUpdates(tt.cronFHPAKey, tt.initialTarget)


Is there a possibility of conflict when using the same handler for different test cases?

Yeah, what you said makes sense. I'll update the tests to ensure that each test case uses a fresh handler instance.

but the handler map has cronFHPAKey as the keys

what do u suggest @zhzhuang-zju ?

but the handler map has cronFHPAKey as the keys

sorry, I didn't get it. Could you explain what issue this might cause?

I meant using different cronFHPAKey values for each test case, even with the same handler instance, should not lead to conflicts. What do u say?

This does indeed avoid conflicts, but using different cronFHPAKeys also increases maintenance costs in the long run.

Okay, i'll use a separate cronFHPAKey for new scale target and we'll create a new handler for each test case. Is it okay?

yeah~make sense to me

karmada-bot · 2024-09-05T09:02:13Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from xishanyongye-chang. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

pkg/controllers/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

anujagrawal699 · 2024-09-05T13:28:15Z

@zhzhuang-zju

anujagrawal699 · 2024-09-07T03:43:22Z

@zhzhuang-zju Please take a look.

zhzhuang-zju

others LGTM
cc @XiShanYongYe-Chang for another look

pkg/controllers/cronfederatedhpa/cronfederatedhpa_handler_test.go

Signed-off-by: Anuj Agrawal <[email protected]> Added tests for pkg/controllers/cronfederatedhpa Signed-off-by: Anuj Agrawal <[email protected]> Added tests for controllers/cronfederatedhpa Signed-off-by: Anuj Agrawal <[email protected]>

anujagrawal699 · 2024-09-09T10:45:09Z

@XiShanYongYe-Chang CI failed.

zhzhuang-zju · 2024-09-09T11:33:13Z

/retest

anujagrawal699 · 2024-09-12T05:10:09Z

@XiShanYongYe-Chang PTAL, Thanks.

karmada-bot added kind/documentation Categorizes issue or PR as related to documentation. kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. labels Sep 4, 2024

karmada-bot requested review from whitewindmills and XiShanYongYe-Chang September 4, 2024 18:18

karmada-bot added kind/feature Categorizes issue or PR as related to a new feature. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 4, 2024

karmada-bot assigned XiShanYongYe-Chang Sep 5, 2024

zhzhuang-zju reviewed Sep 5, 2024

View reviewed changes

anujagrawal699 force-pushed the addedTests-pkg/controllers/cronfederatedhpa branch from 0e35f96 to 4bcdfed Compare September 5, 2024 09:02

anujagrawal699 mentioned this pull request Sep 7, 2024

Test Plan: Enhance Karmada controller-manager and schedule testing coverage (LFX 2024 TERM 3) #5470

Open

zhzhuang-zju reviewed Sep 9, 2024

View reviewed changes

pkg/controllers/cronfederatedhpa/cronfederatedhpa_handler_test.go Outdated Show resolved Hide resolved

Added tests for cronfederatedhpa controller

e648c1b

Signed-off-by: Anuj Agrawal <[email protected]> Added tests for pkg/controllers/cronfederatedhpa Signed-off-by: Anuj Agrawal <[email protected]> Added tests for controllers/cronfederatedhpa Signed-off-by: Anuj Agrawal <[email protected]>

anujagrawal699 force-pushed the addedTests-pkg/controllers/cronfederatedhpa branch from 4bcdfed to e648c1b Compare September 9, 2024 09:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added tests for cronfederatedhpa controller #5481

Added tests for cronfederatedhpa controller #5481

anujagrawal699 commented Sep 4, 2024

codecov-commenter commented Sep 4, 2024 •

edited

Loading

anujagrawal699 commented Sep 4, 2024

XiShanYongYe-Chang commented Sep 5, 2024

zhzhuang-zju Sep 5, 2024

anujagrawal699 Sep 5, 2024

zhzhuang-zju Sep 5, 2024

anujagrawal699 Sep 5, 2024

zhzhuang-zju Sep 5, 2024

anujagrawal699 Sep 5, 2024

anujagrawal699 Sep 5, 2024

anujagrawal699 Sep 5, 2024

zhzhuang-zju Sep 5, 2024

anujagrawal699 Sep 5, 2024

zhzhuang-zju Sep 5, 2024

anujagrawal699 Sep 5, 2024 •

edited

Loading

zhzhuang-zju Sep 5, 2024

anujagrawal699 Sep 5, 2024

karmada-bot commented Sep 5, 2024

anujagrawal699 commented Sep 5, 2024

anujagrawal699 commented Sep 7, 2024

zhzhuang-zju left a comment

anujagrawal699 commented Sep 9, 2024

zhzhuang-zju commented Sep 9, 2024

anujagrawal699 commented Sep 12, 2024

Added tests for cronfederatedhpa controller #5481

Are you sure you want to change the base?

Added tests for cronfederatedhpa controller #5481

Conversation

anujagrawal699 commented Sep 4, 2024

codecov-commenter commented Sep 4, 2024 • edited Loading

Codecov Report

anujagrawal699 commented Sep 4, 2024

XiShanYongYe-Chang commented Sep 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anujagrawal699 Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karmada-bot commented Sep 5, 2024

anujagrawal699 commented Sep 5, 2024

anujagrawal699 commented Sep 7, 2024

zhzhuang-zju left a comment

Choose a reason for hiding this comment

anujagrawal699 commented Sep 9, 2024

zhzhuang-zju commented Sep 9, 2024

anujagrawal699 commented Sep 12, 2024

codecov-commenter commented Sep 4, 2024 •

edited

Loading

anujagrawal699 Sep 5, 2024 •

edited

Loading