-
Notifications
You must be signed in to change notification settings - Fork 919
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added tests for controllers/deploymentreplicassyncer #5493
Added tests for controllers/deploymentreplicassyncer #5493
Conversation
8c04229
to
1a16c6b
Compare
The TestSetupWithManager is removed in the latest commit because it was causing CI failures and provided limited value in terms of testing the controller's core functionality. |
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 #5493 +/- ##
===========================================
+ Coverage 31.70% 45.00% +13.29%
===========================================
Files 643 658 +15
Lines 44445 53917 +9472
===========================================
+ Hits 14090 24263 +10173
+ Misses 29325 28080 -1245
- Partials 1030 1574 +544
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
1a16c6b
to
9f02242
Compare
@XiShanYongYe-Chang Please take a look. |
/assign |
pkg/controllers/deploymentreplicassyncer/deployment_replicas_syncer_controller_test.go
Outdated
Show resolved
Hide resolved
pkg/controllers/deploymentreplicassyncer/deployment_replicas_syncer_controller_test.go
Show resolved
Hide resolved
pkg/controllers/deploymentreplicassyncer/deployment_replicas_syncer_controller_test.go
Show resolved
Hide resolved
9f02242
to
54813b1
Compare
@zhzhuang-zju PTAL |
/lgtm |
Thanks~ |
54813b1
to
b765067
Compare
/lgtm |
case tc.name == "create event": | ||
result = predicateFunc.Create(event.CreateEvent{Object: tc.newObj}) | ||
case tc.name == "delete event": | ||
result = predicateFunc.Delete(event.DeleteEvent{Object: tc.oldObj}) | ||
case tc.name == "generic event": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally, the name is read-only and may be changed frequently in the future. If the switch case statement is used, it is recommended that a type field be added to indicate the name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add an event type structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other parts lgtm, thanks~
Signed-off-by: Anuj Agrawal <[email protected]> Added tests for controllers/deploymentreplicassyncer Signed-off-by: Anuj Agrawal <[email protected]> Added tests for controllers/deploymentreplicassyncer Signed-off-by: Anuj Agrawal <[email protected]> Added tests for controllers/deploymentreplicassyncer Signed-off-by: Anuj Agrawal <[email protected]> Added tests for controllers/deploymentreplicassyncer Signed-off-by: Anuj Agrawal <[email protected]> Added tests for controllers/deploymentreplicassyncer Signed-off-by: Anuj Agrawal <[email protected]>
b765067
to
455cb39
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
[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 |
Description:
This PR introduces tests for deploymentreplicassyncer controller. The new addition tests significant logic of deploymentreplicassyncer controller.
Additions:
TestPredicateFunc:
TestSetupWithManager:
TestReconcile:
TestIsDeploymentStatusCollected:
Test Coverage:
The test coverage of the deploymentreplicassyncer controller has been increased to 80.60% .
What type of PR is this?
/kind feature
Which issue(s) this PR fixes:
Fixes a part of #5470
Does this PR introduce a user-facing change?: