HYPERFLEET-1162 - test: Update resource_references delete test#302
Conversation
|
Skipping CI for Draft Pull Request. |
📝 WalkthroughWalkthroughSoft deletion now clears outbound resource references before conditions are recomputed and saved. Integration coverage consolidates deletion scenarios into subtests for unreferenced, referenced, cascaded, and force-deleted resources. Additional coverage verifies descriptor setup for Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Excluded labels (none allowed) (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
ma-hill
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/integration/resource_references_test.go (2)
434-440: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the
suffixhelper for consistent unique-name generation.
TestResourceReferences_PatchClearOptionalRefsinlinesfmt.Sprintf("target-clropt-%s", uuid.NewString()[:8])andfmt.Sprintf("optsource-clr-%s", uuid.NewString()[:8])instead of reusing thesuffixclosure pattern fromTestResourceReferences_Delete. Extractingsuffixto a package-level helper would eliminate this duplication.♻️ Suggested extraction
+func refSuffix(label string) string { + return fmt.Sprintf("%s-%s", label, uuid.NewString()[:8]) +}Then use
refSuffix("target-clropt")andrefSuffix("optsource-clr")in both tests.🤖 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 `@test/integration/resource_references_test.go` around lines 434 - 440, Extract the shared unique-name generation into a package-level refSuffix helper, following the suffix closure pattern used by TestResourceReferences_Delete. Update TestResourceReferences_PatchClearOptionalRefs to use refSuffix("target-clropt") and refSuffix("optsource-clr") instead of inline fmt.Sprintf and UUID slicing, and reuse the helper in both tests.Source: Path instructions
268-268: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd descriptive messages to bare assertions.
Lines 268, 319, and 327 have assertions without failure messages (
Expect(svcErr.HTTPCode).To(Equal(409)),Expect(getErr).ToNot(BeNil())). Per HyperFleet testing standards, assertions should include descriptive messages to aid debugging. As per coding guidelines, assertions include descriptive messages — no baret.Error(err).♻️ Suggested messages
- Expect(svcErr.HTTPCode).To(Equal(409)) + Expect(svcErr.HTTPCode).To(Equal(409), "referenced target delete should return 409 conflict")- Expect(svcErr.HTTPCode).To(Equal(409)) + Expect(svcErr.HTTPCode).To(Equal(409), "regular delete of referenced target should return 409")- Expect(getErr).ToNot(BeNil()) + Expect(getErr).ToNot(BeNil(), "target should not be retrievable after force-delete")Also applies to: 319-319, 327-327
🤖 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 `@test/integration/resource_references_test.go` at line 268, Update the assertions at the affected resource-reference test cases, including the checks around svcErr.HTTPCode and getErr, to include concise descriptive failure messages explaining the expected HTTP status or error condition. Preserve the existing matchers and expected values.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 `@test/integration/resource_references_test.go`:
- Around line 434-440: Extract the shared unique-name generation into a
package-level refSuffix helper, following the suffix closure pattern used by
TestResourceReferences_Delete. Update
TestResourceReferences_PatchClearOptionalRefs to use refSuffix("target-clropt")
and refSuffix("optsource-clr") instead of inline fmt.Sprintf and UUID slicing,
and reuse the helper in both tests.
- Line 268: Update the assertions at the affected resource-reference test cases,
including the checks around svcErr.HTTPCode and getErr, to include concise
descriptive failure messages explaining the expected HTTP status or error
condition. Preserve the existing matchers and expected values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: bd660b9b-2c6d-4c86-8d6a-b6bc607f73a2
📒 Files selected for processing (1)
test/integration/resource_references_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
Risk Score: 2 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 210 lines (>200) | +1 |
| Sensitive paths | none | +0 |
| Test coverage | Missing tests for: pkg/services | +1 |
Computed by hyperfleet-risk-scorer
|
/test |
|
/test all |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kuudori 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 |
99c22d1
into
openshift-hyperfleet:main
3 participants
Summary
Refactors the resource reference deletion integration tests into organized subtests and adds coverage for previously untested scenarios. The delete tests were standalone functions with duplicated setup; this consolidates them under
TestResourceReferences_Deletewitht.Run()subtests and adds an unreferenced-target test and an end-to-end source-then-target deletion flow.HYPERFLEET-1162
Changes
TestResourceReferences_DeleteTargetWhileReferenced,TestResourceReferences_DeleteSourceSucceeds,TestResourceReferences_ForceDeleteReferencedTarget) intot.Run()subtests underTestResourceReferences_DeleteUnreferencedTarget_Succeedssubtest verifying that a target with no references can be deleted and is hard-deleted from the databaseContainSubstring("referenced by RefSource")assertion to the 409 conflict test, validating the error message identifies the referencing entitySourceAndTarget_Succeeds) to also delete the target after the source is removed, verifying the full lifecycle of reference cleanupsuffixhelper closure for cleaner unique name generation across subtestsTest Plan
make test-allpassesmake lintpasses