Uh oh!
There was an error while loading. Please reload this page.
test(data delete): cover the destructive teardown path (execute + partial-fail exit 7) - #249
Merged
Merged
Conversation
…tial-fail) runDataDelete was 22% — every test stopped at cluster discovery, so the actual DROP TABLE + file removal (unrecoverable) had no coverage. Add two fn-var test seams (default to the real funcs, zero behavior change), mirroring loadClusterFn/listDatasetsFn: - resolveClusterTargetFn: inject a canned target (fake clientset + release + PVC) without seeding the k8s objects discoverRelease/DiscoverSharedPVC look for. - teardownFn: over push.Teardown. TestRunDataDelete_Execute drives past discovery and exercises the three teardown outcomes: clean -> exit 0; table dropped but file removal fails -> exit 7 + recovery hint; fails before the drop -> exit 7 "teardown failed". The mixed-case "Churn" input also pins the case-insensitive resolveDeleteTarget match (backend#1027). make ci green. Follow-ups: loadClusterFn routing for cluster-info/doctor/ home + stageFn for ingest --overwrite; resolveClusterTarget's own success tail (needs a k8s-object fixture). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
LukasWodka
commented
Jul 14, 2026
ContributorAuthor
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 1b0e871. Configure here.
saadqbal
approved these changes
Jul 14, 2026
saadqbal
left a comment
Collaborator
There was a problem hiding this comment.
LGTM 👍 The three teardown outcomes line up exactly with runDataDelete's error handling, and pinning the case-insensitive resolveDeleteTarget match (backend#1027) via the Churn case is a nice touch. Seams default to the real fns, so no behavior change. Note: since this stack adds seams to data.go, expect a rebase as the siblings land.
Uh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Third increment from the CLI test-coverage audit — the destructive
data deletepath, a P0 (runDataDeletewas 22%; every existing test stopped at cluster discovery, so the actual teardown — aDROP TABLE+ file removal that can't be undone — had no coverage).Production changes (seams — zero behavior change)
Two
fn-var test seams, mirroring the existingloadClusterFn/listDatasetsFnpattern (both default to the real function, so production is unchanged):resolveClusterTargetFn— lets a command test inject a fully-resolved target (fake clientset + release + PVC) instead of seeding the k8s objectsdiscoverRelease/DiscoverSharedPVClook for.runDataDeletenow calls it.teardownFn— overpush.Teardown(theDROP TABLE+ file removal).runDataDelete's execute step now calls it.Test
TestRunDataDelete_Executedrives the command past discovery via the seams, then exercises the three teardown outcomes:Deletedline;… was dropped, but removing its files failed— the idempotent-DROP re-run guidance);teardown failed.The mixed-case
"Churn"input in the partial-failure case also pins the case-insensitiveresolveDeleteTargetmatch (backend#1027 — a mis-cased name used toDROPnothing and still exit 0).Why a seam, not a k8s fixture
resolveClusterTarget's success needsdiscoverRelease+DiscoverSharedPVCto find seeded objects — fiddly, and it's the established seam pattern that unblocks every command that resolves a target. TestingresolveClusterTarget's own success tail (its 47%) with real k8s objects is a separate follow-up.Not in this PR (follow-ups)
runClusterInfo/runClusterDoctor/realProbeEnvthroughloadClusterFn, and thestageFnseam forrunDataIngest's--overwritepath — plus their tests.resolveClusterTarget's own success-tail coverage (needs the k8s-object fixture).Test plan
make cigreen (build,-race, gofmt, lint, schema). New + existingdata deletetests pass.🤖 Generated with Claude Code
Note
Low Risk
No production behavior change; seams default to existing functions and the diff is tests plus indirection wiring only.
Overview
Adds test-only fn-var seams so
runDataDeletecan be exercised past cluster discovery without a real kubeconfig or cluster:resolveClusterTargetFn(injected resolved target) andteardownFn(overpush.Teardown). Production still uses the real implementations; only call sites inrunDataDeleteswitch to the seam variables.TestRunDataDelete_Executedrives the destructive execute step with--yesand mocked targets, covering success (exit 0 +Deleted), partial failure afterDROP TABLE(exit 7 + recovery hint), and failure before the drop (exit 7teardown failed). The partial-failure case uses mixed-caseChurnto lock in case-insensitive target resolution before teardown.Reviewed by Cursor Bugbot for commit 1b0e871. Bugbot is set up for automated code reviews on this repo. Configure here.