MGMT-23548: Ensure tests w/ gomock controllers call Finish()#10024
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@bluesort: This pull request references MGMT-23548 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.22.0" version, but no target version was set. DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a "Test Patterns" doc entry about gomock usage and updates many test suites to call Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
bot
added
the
size/M
openshift-ci
bot
added
the
approved
| It("Missing cluster name", func() { | ||
| cluster = common.Cluster{Cluster: models.Cluster{ID: &clusterID, BaseDNSDomain: baseDNSDomain}} | ||
| Expect(db.Create(&cluster).Error).ShouldNot(HaveOccurred()) | ||
| mockVersions.EXPECT().GetReleaseImage(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(&models.ReleaseImage{URL: swag.String("quay.io/release")}, nil) |
There was a problem hiding this comment.
Handling for the error covered by this spec happens before GetReleaseImage is called
| It("Missing domain base name", func() { | ||
| cluster = common.Cluster{Cluster: models.Cluster{ID: &clusterID, Name: name}} | ||
| Expect(db.Create(&cluster).Error).ShouldNot(HaveOccurred()) | ||
| mockVersions.EXPECT().GetReleaseImage(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(&models.ReleaseImage{URL: swag.String("quay.io/release")}, nil) |
There was a problem hiding this comment.
Handling for the error covered by this spec happens before GetReleaseImage is called
| }) | ||
| It("fails to uploads event data when malformed pullsecret", func() { | ||
| createOCMPullSecretWithEmptyToken(*mockK8sClient) | ||
| mockEvents.EXPECT().V2GetEvents( |
There was a problem hiding this comment.
Handling for the error covered by this spec happens before V2GetEvents is called
| }) | ||
| It("fails to uploads event data when pullsecret not found", func() { | ||
| createOCMPullSecretNotFound(*mockK8sClient) | ||
| mockEvents.EXPECT().V2GetEvents( |
There was a problem hiding this comment.
Handling for the error covered by this spec happens before V2GetEvents is called
|
/cc @eliorerz |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #10024 +/- ##
==========================================
+ Coverage 44.27% 44.42% +0.15%
==========================================
Files 416 415 -1
Lines 72549 72908 +359
==========================================
+ Hits 32121 32391 +270
- Misses 37522 37603 +81
- Partials 2906 2914 +8 🚀 New features to boost your workflow:
|
|
/retest |
bluesort
force-pushed
the
fix-missing-test-finishes
branch
from
8f9b187 to
8178e7b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/s3wrapper/filesystem_test.go`:
- Around line 59-63: Reorder the AfterEach cleanup so gomock always verifies
expectations: call ctrl.Finish() before asserting removal succeeded (i.e.,
invoke ctrl.Finish() at the start of the AfterEach), then run err :=
os.RemoveAll(baseDir) and check err with Expect only after; also add a nil guard
(if ctrl != nil { ctrl.Finish() }) to avoid panics if the controller wasn't
created. Ensure you update the AfterEach closure surrounding the symbols
AfterEach, ctrl.Finish(), os.RemoveAll, baseDir, and Expect accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d0368e8b-a480-4109-bd81-42dbeb3ed246
📒 Files selected for processing (18)
CLAUDE.mdinternal/cluster/common_test.gointernal/controller/controllers/clusterdeployments_controller_test.gointernal/host/hostcommands/container_image_availability_cmd_test.gointernal/host/hostcommands/disk_performance_cmd_test.gointernal/host/hostcommands/domain_name_resolution_cmd_test.gointernal/host/hostutil/update_host_test.gointernal/host/refresh_status_preprocessor_test.gointernal/installercache/installercache_test.gointernal/metrics/directory_usage_collector_test.gointernal/provider/registry/registry_test.gointernal/spoke_k8s_client/factory_test.gointernal/stream/notification_stream_test.gointernal/uploader/auth_utils_test.gointernal/uploader/events_uploader_test.gointernal/usage/manager_test.gopkg/kafka/json_writer_test.gopkg/s3wrapper/filesystem_test.go
✅ Files skipped from review due to trivial changes (3)
- internal/metrics/directory_usage_collector_test.go
- pkg/kafka/json_writer_test.go
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (9)
- internal/usage/manager_test.go
- internal/host/hostcommands/disk_performance_cmd_test.go
- internal/uploader/auth_utils_test.go
- internal/stream/notification_stream_test.go
- internal/installercache/installercache_test.go
- internal/host/hostcommands/domain_name_resolution_cmd_test.go
- internal/spoke_k8s_client/factory_test.go
- internal/uploader/events_uploader_test.go
- internal/controller/controllers/clusterdeployments_controller_test.go
bluesort
force-pushed
the
fix-missing-test-finishes
branch
from
8178e7b to
901eb1d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controller/controllers/clusterdeployments_controller_test.go`:
- Around line 2058-2060: The nested "cluster deletion" spec is calling
mockCtrl.Finish() in its AfterEach while the parent suite already calls
mockCtrl.Finish(), causing a duplicate Finish invocation; remove the inner
AfterEach or its mockCtrl.Finish() call so mockCtrl.Finish() is only invoked
once for the suite (locate the nested AfterEach block that contains
mockCtrl.Finish() in the cluster deletion tests and delete that line or the
entire AfterEach if it's empty otherwise leave other teardown code intact).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5abd904b-4298-4ead-a7f3-45c0122ddb11
📒 Files selected for processing (18)
CLAUDE.mdinternal/cluster/common_test.gointernal/controller/controllers/clusterdeployments_controller_test.gointernal/host/hostcommands/container_image_availability_cmd_test.gointernal/host/hostcommands/disk_performance_cmd_test.gointernal/host/hostcommands/domain_name_resolution_cmd_test.gointernal/host/hostutil/update_host_test.gointernal/host/refresh_status_preprocessor_test.gointernal/installercache/installercache_test.gointernal/metrics/directory_usage_collector_test.gointernal/provider/registry/registry_test.gointernal/spoke_k8s_client/factory_test.gointernal/stream/notification_stream_test.gointernal/uploader/auth_utils_test.gointernal/uploader/events_uploader_test.gointernal/usage/manager_test.gopkg/kafka/json_writer_test.gopkg/s3wrapper/filesystem_test.go
✅ Files skipped from review due to trivial changes (7)
- internal/metrics/directory_usage_collector_test.go
- internal/cluster/common_test.go
- internal/provider/registry/registry_test.go
- internal/installercache/installercache_test.go
- internal/host/hostcommands/disk_performance_cmd_test.go
- pkg/s3wrapper/filesystem_test.go
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (7)
- internal/uploader/auth_utils_test.go
- internal/host/refresh_status_preprocessor_test.go
- internal/host/hostcommands/domain_name_resolution_cmd_test.go
- internal/stream/notification_stream_test.go
- pkg/kafka/json_writer_test.go
- internal/spoke_k8s_client/factory_test.go
- internal/uploader/events_uploader_test.go
internal/controller/controllers/clusterdeployments_controller_test.go
Outdated
Show resolved
Hide resolved
bluesort
force-pushed
the
fix-missing-test-finishes
branch
2 times, most recently
from
6a1eb08 to
61008af
Compare
|
/test edge-unit-test |
|
@bluesort: No presubmit jobs available for openshift/assisted-service@master DetailsIn 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. |
|
/test edge-unit-test |
|
/test edge-e2e-metal-assisted-none-4-22 |
|
/override ci/prow/e2e-agent-compact-ipv4-iso-no-registry ci/prow/edge-e2e-ai-operator-ztp |
|
@bluesort: Overrode contexts on behalf of bluesort: ci/prow/e2e-agent-compact-ipv4-iso-no-registry, ci/prow/edge-e2e-ai-operator-ztp DetailsIn 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. |
CLAUDE.md
Outdated
| ### Test Patterns | ||
|
|
||
| #### Mock Testing with Gomock | ||
|
|
There was a problem hiding this comment.
nit: can we state the scope of where this is going to be called, and maybe create a test skill specificly on how to write tests in assisted (we can start by this section only)
There was a problem hiding this comment.
Pushed a dedicated assisted-service-writing-unit-tests skill to help enforce patterns like this one. Touched on it in more detail in the PR description
|
minor comments otherwise LGTM |
bluesort
force-pushed
the
fix-missing-test-finishes
branch
2 times, most recently
from
338133e to
5d39a91
Compare
openshift-ci
bot
added
size/L
|
/retest |
bluesort
force-pushed
the
fix-missing-test-finishes
branch
from
5d39a91 to
bee7ff1
Compare
openshift-ci
bot
added
size/M
|
❤️ |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bluesort, rccrdpccl The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@bluesort: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
3 participants
Problem
Tests using gomock controllers were missing
ctrl.Finish()calls in their teardown, causing false negatives where tests would pass even when mock expectations were violated. WithoutFinish():.Times(N)constraints were never verified.MaxTimes(0)violations were silently ignoredThe following 16 test files had controllers missing
Finish()calls:The list of affected files was generated with the help of Claude code and verified with the following snippet:
Solution
Add
Finish()calls to the above test files. A companion PR adds a dedicated unit-testing skill so generated tests adhere to this pattern in the future.3 test files had incorrect
EXPECT()invocations that were unchecked and thus went unnoticed:UploadEventsspecs simulating bad pull secret cases expected calls toV2GetEventswhich should not be run when a pull secret error is encountereddomainNameResolutionspecs covering error cases expected calls toGetReleaseImagewhich should not be run when the given errors are encounteredCluster Refresh Status Preprocessorspecs expected a call toValidateHostwhich never runs since the test hosts haveinfraEnvCluster Refresh Status Preprocessorspecs expected a call toValidateCluster, which is called byinternal/cluster, notinternal/hostBoth failures have been addressed by removing the incorrect expects.
List all the issues related to this PR
What environments does this code impact?
How was this code tested?
Reviewers Checklist