ROSAENG-62146: New action tests making sure that the actions are doing what they are supposed to do and only that - #59
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
needs-rebase
|
Warning Review limit reached
Next review available in: 47 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository: openshift-online/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
WalkthroughThe change expands table-driven Kubernetes action tests with reusable fixtures and broader operation coverage. It updates Go dependencies required by the tests and adds a compile-time check that ChangesActions and authorization
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 6❌ Failed checks (6 inconclusive)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
internal/actions/actions_test.go (3)
146-175: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
actionImplNamein the subtest name.The struct field
actionImplNameis set for each case but never used. The subtest name usestest.action.Name(), which repeats the action verb already asserted in the test. UsingactionImplNamemakes failures easier to attribute.♻️ Proposed change
- t.Run(fmt.Sprintf("When using a %s action, name should be %s", test.action.Name(), test.actionName), func(t *testing.T) { + t.Run(fmt.Sprintf("When using a %s, name should be %s", test.actionImplName, test.actionName), func(t *testing.T) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/actions/actions_test.go` around lines 146 - 175, Update the subtest name in TestAction_Name to use the table’s actionImplName field instead of test.action.Name(), while preserving the existing expected actionName assertion.
20-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRegister list kinds explicitly to remove the fixture workaround.
NewSimpleDynamicClientderives list kinds from the scheme and the seeded objects. The scheme here is empty, so the tracker only knows a list kind when at least one object of that kind is seeded. The comment at Line 366 documents this workaround.NewSimpleDynamicClientWithCustomListKindsremoves the constraint and lets a case start with an empty tracker.♻️ Proposed change
func newFakeClient(objects ...runtime.Object) *dynamicfake.FakeDynamicClient { scheme := runtime.NewScheme() - return dynamicfake.NewSimpleDynamicClient(scheme, objects...) + listKinds := map[schema.GroupVersionResource]string{ + {Group: "", Version: "v1", Resource: "configmaps"}: "ConfigMapList", + {Group: "", Version: "v1", Resource: "secrets"}: "SecretList", + {Group: "", Version: "v1", Resource: "nodes"}: "NodeList", + } + return dynamicfake.NewSimpleDynamicClientWithCustomListKinds(scheme, listKinds, objects...) }#!/bin/bash # Confirm the constructor signature in the pinned client-go version. ver=$(fd -H -t f '^go\.mod$' --exec rg -N -o 'k8s.io/client-go v[0-9.]+' | head -1 | awk '{print $2}') echo "client-go $ver" curl -s "https://raw.githubusercontent.com/kubernetes/client-go/${ver}/dynamic/fake/simple.go" \ | rg -n 'func NewSimpleDynamicClient(WithCustomListKinds)?\(' -A 6🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/actions/actions_test.go` around lines 20 - 23, Update newFakeClient to use NewSimpleDynamicClientWithCustomListKinds and register the required resource-to-list-kind mappings explicitly, preserving the variadic seeded-object setup. Remove the fixture workaround documented near the affected tests so cases can begin with an empty tracker.
25-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix the typo in the helper name.
newClientOjectis missing ab. Rename it tonewClientObjectand update the caller at Line 39.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/actions/actions_test.go` around lines 25 - 36, Rename the test helper newClientOject to newClientObject and update its caller accordingly, preserving the helper’s behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/actions/actions_test.go`:
- Around line 146-175: Update the subtest name in TestAction_Name to use the
table’s actionImplName field instead of test.action.Name(), while preserving the
existing expected actionName assertion.
- Around line 20-23: Update newFakeClient to use
NewSimpleDynamicClientWithCustomListKinds and register the required
resource-to-list-kind mappings explicitly, preserving the variadic seeded-object
setup. Remove the fixture workaround documented near the affected tests so cases
can begin with an empty tracker.
- Around line 25-36: Rename the test helper newClientOject to newClientObject
and update its caller accordingly, preserving the helper’s behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift-online/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 19d70c58-6240-44d6-bec7-91404efd913a
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.modinternal/actions/actions_test.gointernal/authorization/authorization.go
…g what they are supposed to do and only that
dfc4131 to
4c40f33
Compare
openshift-ci
Bot
added
needs-rebase
|
PR needs rebase. 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. |
1 participant
I focused on the actions. Please tell me if this is enough to cover https://redhat.atlassian.net/browse/ROSAENG-62146
Summary by CodeRabbit
Tests
Maintenance