ROSAENG-60113 | test(unit): add clusterrosa/hcp validator coverage#1243
ROSAENG-60113 | test(unit): add clusterrosa/hcp validator coverage#1243amandahla wants to merge 1 commit into
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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a new Ginkgo test suite for ChangesHCP Validators Test Suite
Estimated code review effort: 2 (Simple) | ~12 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
provider/clusterrosa/hcp/validators_test.go (2)
48-175: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting a shared harness for the three near-identical DescribeTable blocks.
validateChannelAndChannelGroupChanges(Lines 48-86),validateChannelGroupAndVersionChanges(Lines 88-135), andvalidateChannelAndVersionChanges(Lines 137-175) share the same clone/mutate/assert scaffolding, differing only in the mutate entries and the function under test. A small generic helper (e.g., accepting the validator func as a parameter) would remove this duplication.♻️ Example shared harness
func runUpdateValidatorTable( validate func(state, plan *ClusterRosaHcpState) diag.Diagnostics, mutate func(state, plan *ClusterRosaHcpState), expectedErr bool, ) { state := cloneBasicState() plan := cloneBasicState() mutate(state, plan) diags := validate(state, plan) Expect(diags.HasError()).To(Equal(expectedErr)) }🤖 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 `@provider/clusterrosa/hcp/validators_test.go` around lines 48 - 175, The three DescribeTable blocks repeat the same clone/mutate/assert setup, so extract a shared table harness and reuse it across `validateChannelAndChannelGroupChanges`, `validateChannelGroupAndVersionChanges`, and `validateChannelAndVersionChanges`. Add a small helper that accepts the validator function plus the mutate callback and expected error flag, then have each table entry call that helper instead of duplicating `cloneBasicState`, `mutate`, and `Expect(diags.HasError())` logic.
137-175: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing "state version null" entry, mirroring the sibling test suite.
validateChannelGroupAndVersionChangestests include an entry for "version set when state version is null" (Lines 125-133) because the source function treats a null-state/non-null-plan version as changed (pervalidateChannelGroupAndVersionChangesinresource.go).validateChannelAndVersionChangesimplements the identical null-handling branch but this test table has no equivalent entry, leaving that branch uncovered here.As per path instructions, "Ensure assertions cover behavior changes, not only happy paths."
✅ Suggested additional entry
Entry("version set when state version is null -> counts as version change with channel", func(state, plan *ClusterRosaHcpState) { state.Channel = types.StringValue("stable") state.Version = types.StringNull() plan.Channel = types.StringValue("candidate") plan.Version = types.StringValue("4.15.0") }, true, ),🤖 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 `@provider/clusterrosa/hcp/validators_test.go` around lines 137 - 175, The Channel and version update validation table is missing coverage for the null-state version change branch. Add an Entry to the validateChannelAndVersionChanges DescribeTable that mirrors the sibling validateChannelGroupAndVersionChanges case by setting state.Version to types.StringNull() while plan.Version is non-null and asserting the expected error behavior when channel changes alongside it. Use the existing validateChannelAndVersionChanges, cloneBasicState, and ClusterRosaHcpState symbols to keep the new case aligned with the current table structure.Source: Path instructions
🤖 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 `@provider/clusterrosa/hcp/validators_test.go`:
- Around line 48-175: The three DescribeTable blocks repeat the same
clone/mutate/assert setup, so extract a shared table harness and reuse it across
`validateChannelAndChannelGroupChanges`,
`validateChannelGroupAndVersionChanges`, and `validateChannelAndVersionChanges`.
Add a small helper that accepts the validator function plus the mutate callback
and expected error flag, then have each table entry call that helper instead of
duplicating `cloneBasicState`, `mutate`, and `Expect(diags.HasError())` logic.
- Around line 137-175: The Channel and version update validation table is
missing coverage for the null-state version change branch. Add an Entry to the
validateChannelAndVersionChanges DescribeTable that mirrors the sibling
validateChannelGroupAndVersionChanges case by setting state.Version to
types.StringNull() while plan.Version is non-null and asserting the expected
error behavior when channel changes alongside it. Use the existing
validateChannelAndVersionChanges, cloneBasicState, and ClusterRosaHcpState
symbols to keep the new case aligned with the current table structure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 7dad0353-c32f-49ba-a354-d148ff516d35
📒 Files selected for processing (1)
provider/clusterrosa/hcp/validators_test.go
Cover HCP update validators, auto_node role ARN validation, and small pure helpers used during cluster updates without exercising CRUD paths. Signed-off-by: Amanda Hager Lopes de Andrade Katz <amanda.katz@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Amanda Hager Lopes de Andrade Katz <amanda.katz@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
amandahla
force-pushed
the
ROSAENG-60113-unit-tests-4
branch
from
3adbc52 to
44d2214
Compare
|
@amandahla: The following test failed, say
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. |
1 participant
Summary
provider/clusterrosa/hcp.Planned follow-up PRs
provider/common/attrvalidators— merged (#1212)provider/clusterrosa/common+hcp/shared_vpc— merged (#1218)provider/common(root) — merged (#1231)provider/clusterrosa/hcp— untestedvalidate*/ small pure funcs — this PRprovider/clusterrosa/classic— validators only (not CRUD)provider/proxy—Contains(optional, if still uncovered)Detailed Description of the Issue
HCP cluster resource helpers for update-time validation (
validateChannel*,validateNoImmutableAttChange,validateAutoNodeRoleARN) and small pure functions (getOcmVersionMinor,getAutoNodeMode,getAutoNodeRoleARN,shouldPatchProperties) had no direct unit tests.Related Issues and PRs
ROSAENG-60113-unit-tests-4).Type of Change
Previous Behavior
No unit tests covered HCP update validators or the listed pure helpers.
Behavior After This Change
Table-driven Ginkgo unit tests validate channel/version mutual-exclusion rules, immutability checks, auto-node role ARN validation, and property-patch logic. No production code changes.
Intentional skips (e2e/subsystem checklist)
validateUpgrade— requires OCM client; defer to subsystem/e2e upgrade flows.validateNoImmutableAttChangehas representative unit coverage; subsystem immutability tests cover resource wiring.Create/Read/Update/Delete) — out of scope for this PR series entry.How to Test (Step-by-Step)
Preconditions
Test Steps
go test ./provider/clusterrosa/hcp -count=1make pre-push-checksExpected Results
provider/clusterrosa/hcpspecs pass.Proof of the Fix
make pre-push-checks(9/9 passed) and CodeRabbit local review (0 findings).Breaking Changes
Breaking Change Details / Migration Plan
N/A
Developer Verification Checklist
[JIRA-TICKET] | [TYPE][(scope)][!]: <MESSAGE>.make install-hookshas been run in this clone.make pre-push-checkspasses.Testing (check all that apply; use N/A when not relevant)
provider//internal/logic changes.subsystem/classic/orsubsystem/hcp/.*_test.go), or a subsystem negative test when integration-only (not both for the same cases unless a wiring smoke test is needed).make check-subsystem-registrypasses.Made with Cursor
Summary by CodeRabbit