Added test for namespace controller #5368
Conversation
Signed-off-by: Anuj Agrawal <[email protected]>
karmada-bot
added
the
kind/failing-test
karmada-bot
added
the
kind/feature
karmada-bot
added
the
size/L
|
@XiShanYongYe-Chang TestController_SetupWithManager failed during the CI workflow but its running fine locally. |
|
/assign |
Signed-off-by: Anuj Agrawal <[email protected]>
karmada-bot
added
size/XL
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #5368 +/- ##
==========================================
+ Coverage 29.01% 29.54% +0.52%
==========================================
Files 632 632
Lines 43862 43835 -27
==========================================
+ Hits 12728 12949 +221
+ Misses 30218 29934 -284
- Partials 916 952 +36
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@XiShanYongYe-Chang CI passed. |
| } | ||
| } | ||
|
|
||
| func TestController_buildWorksWithOverridePolicy(t *testing.T) { |
There was a problem hiding this comment.
Is it convenient to combine this test method with TestController_buildWorks into one test method?
There was a problem hiding this comment.
I think separate tests will ensure that override policy behavior doesn't interfere with basic functionality. I could create helper functions for common setup code if you say.
There was a problem hiding this comment.
You can modify it to see the effect.
| assert.NotNil(t, work) | ||
| } | ||
|
|
||
| func TestController_SetupWithManager(t *testing.T) { |
There was a problem hiding this comment.
Can the following three functions, including this one, be organized into table-driven tests? That would be easier to read and maintain. I do have tests in the current style of testing, but they don't seem to work very well at the moment.
There was a problem hiding this comment.
Yeah, that would be more readable and maintainable.
| assert.Error(t, err, "SetupWithManager should return an error when schemes are not properly set up") | ||
| } | ||
|
|
||
| func TestClusterNamespaceFn(t *testing.T) { |
There was a problem hiding this comment.
Sorry, I didn't understand which function body the function tests.
There was a problem hiding this comment.
The two you mentioned are clusterNamespaceFn and clusterOverridePolicyNamespaceFn which are MapFuncs used in the controller's SetupWithManager() method to handle events related to Cluster and ClusterOverridePolicy resources.
There was a problem hiding this comment.
I understand what you are saying, but where is the code guarded by these two test methods?
There was a problem hiding this comment.
Okay, i got it. I think i need to refractor it. Thank You for pointing out.
There was a problem hiding this comment.
@XiShanYongYe-Chang I'm not able to test them in isolation. I think we should move ahead without them.
There was a problem hiding this comment.
Yes, I don't think they need to be tested.
| assert.Contains(t, requests, reconcile.Request{NamespacedName: types.NamespacedName{Name: "namespace2"}}) | ||
| } | ||
|
|
||
| func TestClusterOverridePolicyNamespaceFn(t *testing.T) { |
|
@XiShanYongYe-Chang PTAL |
Signed-off-by: Anuj Agrawal <[email protected]>
karmada-bot
added
size/L
Signed-off-by: Anuj Agrawal <[email protected]>
|
/retest @XiShanYongYe-Chang |
|
@anujagrawal699: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Hey @anujagrawal699, could you please review my PR as well? Thanks! |
Sure! |
|
/retest |
|
/ok-to-test |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: XiShanYongYe-Chang The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
karmada-bot
added
the
approved
Thanks for approval. I could have squashed the commits. |
I forgot again. |
Its Okay. Thanks! |
Description:
This PR introduces tests for namespace controller and adds a test file that significantly enchances its code coverage.
Additions:
Test Coverage:
Test coverage has been increased from 0% to 41.7%, reflecting the complexity of the controller. To check test coverage, run
go test -coverprofile=coverage.outin the pkg/controllers/namespace directory.What type of PR is this?
/kind failing-test
/kind feature
What this PR does / why we need it:
This PR enhances the testing coverage of the namespace controller, which is crucial for managing namespace operations and ensuring the controller’s stability and functionality. By adding these tests, we ensure that potential edge cases and critical functionalities are properly validated, leading to improved stability and performance of the namespace controller.
Which issue(s) this PR fixes:
Fixes a part of #5235
Does this PR introduce a user-facing change?: