ROSAENG-61173 | test: complete delete cluster command coverage#3372
ROSAENG-61173 | test: complete delete cluster command coverage#3372olucasfreitas wants to merge 1 commit into
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
📝 WalkthroughWalkthroughThe cluster deletion command now creates and cleans up its runtime in a wrapper, delegates deletion behavior to Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: olucasfreitas 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 |
openshift-ci
Bot
added
the
approved
|
Closing: PR was incorrectly opened from the main repo instead of the fork. Will reopen from the fork. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cmd/dlt/cluster/cmd_test.go (1)
35-42: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRestore package-level test seams with
DeferCleanup.
confirmDeleteandrunUninstallLogsremain stubbed after each spec, allowing later package tests to inherit behavior based on execution order. Capture and restore these variables, along with modified global flags, in cleanup.🤖 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 `@cmd/dlt/cluster/cmd_test.go` around lines 35 - 42, Update the BeforeEach setup to register DeferCleanup callbacks that restore confirmDelete, runUninstallLogs, and the modified global flags to their original values after each spec. Capture each original value before overriding it, while preserving the existing per-spec test setup and isolation.Source: Coding guidelines
🤖 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.
Inline comments:
In `@cmd/dlt/cluster/cmd_test.go`:
- Around line 84-107: Strengthen the test around runWithRuntime so the mocked
delete request inspects its payload or query and asserts that bestEffort is
true, rather than accepting any request. Keep the existing warning, success, and
uninstall-output assertions, and ensure the test fails if DeleteCluster does not
receive the flag.
In `@cmd/dlt/cluster/cmd.go`:
- Around line 110-112: Update the error handling around handleClusterDelete to
wrap the returned error with descriptive cluster-deletion operation context
using %w, preserving the original error for unwrapping.
---
Nitpick comments:
In `@cmd/dlt/cluster/cmd_test.go`:
- Around line 35-42: Update the BeforeEach setup to register DeferCleanup
callbacks that restore confirmDelete, runUninstallLogs, and the modified global
flags to their original values after each spec. Capture each original value
before overriding it, while preserving the existing per-spec test setup and
isolation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: a3a5eaaf-84b7-4dc0-954e-feb42ff85d87
📒 Files selected for processing (2)
cmd/dlt/cluster/cmd.gocmd/dlt/cluster/cmd_test.go
| It("prints the best-effort warning and passes the flag through", func() { | ||
| clusterReady := test.MockCluster(func(c *cmv1.ClusterBuilder) { | ||
| c.State(cmv1.ClusterStateReady) | ||
| c.AWS(cmv1.NewAWS().STS(cmv1.NewSTS())) | ||
| }) | ||
| t.SetCluster(clusterId, clusterReady) | ||
| args.bestEffort = true | ||
|
|
||
| statusBody := fmt.Sprintf(`{ | ||
| "kind": "ClusterStatus", | ||
| "id": "%s", | ||
| "state": "ready" | ||
| }`, clusterId) | ||
| t.ApiServer.AppendHandlers(RespondWithJSON(http.StatusOK, statusBody)) | ||
| t.ApiServer.AppendHandlers(RespondWithJSON( | ||
| http.StatusOK, test.FormatClusterList([]*cmv1.Cluster{clusterReady}))) | ||
| t.ApiServer.AppendHandlers(RespondWithJSON(http.StatusOK, "")) | ||
|
|
||
| stdout, stderr, err := test.RunWithOutputCaptureAndArgv(runWithRuntime, t.RosaRuntime, Cmd, &argv) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(stderr).To(ContainSubstring("best effort")) | ||
| Expect(stderr).To(ContainSubstring("certain resources may be left behind")) | ||
| Expect(stdout).To(ContainSubstring("will start uninstalling")) | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that bestEffort reaches the delete request.
This test only verifies warning output and a successful response; the mock handler accepts the request regardless of the flag. Inspect the request in the handler or use a spy so the test fails unless DeleteCluster receives bestEffort=true.
As per path instructions, “Flag weak tests that only restate implementation or changes that weaken existing assertions.”
🤖 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 `@cmd/dlt/cluster/cmd_test.go` around lines 84 - 107, Strengthen the test
around runWithRuntime so the mocked delete request inspects its payload or query
and asserts that bestEffort is true, rather than accepting any request. Keep the
existing warning, success, and uninstall-output assertions, and ensure the test
fails if DeleteCluster does not receive the flag.
Source: Path instructions
| err := handleClusterDelete(r, cluster, clusterKey, args.bestEffort) | ||
| if err != nil { | ||
| r.Reporter.Errorf("%s", err) | ||
| os.Exit(1) | ||
| return err |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Wrap deletion errors with operation context.
Returning the raw SDK error makes the command failure harder to diagnose. Preserve it with %w while identifying the failed cluster operation.
Proposed fix
err := handleClusterDelete(r, cluster, clusterKey, args.bestEffort)
if err != nil {
- return err
+ return fmt.Errorf("failed to delete cluster %q: %w", clusterKey, err)
}As per coding guidelines, “Wrap returned errors with context using %w; do not drop the original error.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| err := handleClusterDelete(r, cluster, clusterKey, args.bestEffort) | |
| if err != nil { | |
| r.Reporter.Errorf("%s", err) | |
| os.Exit(1) | |
| return err | |
| err := handleClusterDelete(r, cluster, clusterKey, args.bestEffort) | |
| if err != nil { | |
| return fmt.Errorf("failed to delete cluster %q: %w", clusterKey, err) | |
| } |
🤖 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 `@cmd/dlt/cluster/cmd.go` around lines 110 - 112, Update the error handling
around handleClusterDelete to wrap the returned error with descriptive
cluster-deletion operation context using %w, preserving the original error for
unwrapping.
Source: Coding guidelines
1 participant
PR Summary
Complete the remaining coverage for
rosa delete clusterby adding command-level tests for the legacy delete flow and a small refactor that makes the command body testable without changing CLI behavior.Detailed Description of the Issue
PR #3298 added helper coverage for
handleClusterDelete()andbuildCommands(), but the actualrosa delete clustercommand flow was still untested. still required coverage for confirmation handling, best-effort output, STS cleanup messaging, uninstall log watching, and wrapper-level error propagation.This change extracts a testable
runWithRuntime(...)helper incmd/dlt/cluster/cmd.goand adds focused Ginkgo coverage for the remaining command behaviors while preserving the existingRun: runentrypoint, flags, and user-facing flow.Related Issues and PRs
Type of Change
Previous Behavior
cmd/dlt/clusterhad helper-level coverage, but the full command flow remained untested. Important behaviors like confirmation decline,--best-effort, STS cleanup output,--watch, and wrapper-level error handling were still gaps in Phase 2B.Behavior After This Change
The delete-cluster command now has focused command-level coverage for:
--best-effortwarning and passthrough behaviorGetClusterStateandDeleteClustererrors--watchinvoking uninstall logs while restoring the region deprecation warning flagThe command shape, flags, and user-facing behavior remain unchanged.
How to Test (Step-by-Step)
Preconditions
go.modGOTOOLCHAIN=local, useGOTOOLCHAIN=autofor the commands belowTest Steps
GOTOOLCHAIN=auto make fmtGOTOOLCHAIN=auto go test ./cmd/dlt/cluster/... -count=1GOTOOLCHAIN=auto make lintGOTOOLCHAIN=auto make rosaGOTOOLCHAIN=auto make testExpected Results
cmd/dlt/clusterpackage tests passrosabinary builds successfullyProof of the Fix
GOTOOLCHAIN=auto go test ./cmd/dlt/cluster/... -count=1passedGOTOOLCHAIN=auto make lintpassedGOTOOLCHAIN=auto make rosapassedGOTOOLCHAIN=auto make testpassedgit push -u origin HEADpassed repo pre-push checks (format, build, lint, changed-files coverage, unit and integration tests)cmd/dlt/cluster/cmd.gonow uses a thinrun()wrapper plusrunWithRuntime(...)cmd/dlt/cluster/cmd_test.gonow covers the remaining Phase 2B command scenariosBreaking Changes
Breaking Change Details / Migration Plan
N/A
Developer Verification Checklist
[JIRA-TICKET] | [TYPE]: <MESSAGE>.make install-hookshas been run in this clone.make testpasses.make lintpasses.make rosapasses.Summary by CodeRabbit