test(cmd): assert what commands actually send to the API - #65
Open
aaearon wants to merge 10 commits into
Open
Conversation
Merge argument capture into mockAccessRequestService and delete the capturing variant, and record request histories for mockElevateService and mockGroupsElevator. Move withInteractiveTTY into the shared helpers file, renamed to _test.go so test-only code stops linking into the binary.
Cover the finalize decision, cancel/get request IDs, list params, submit payload, the validateSubmitFields call site, the non-TTY guards on get/approve/reject, the CSP-only GCP rejection, and login's auto-configure branch and authenticate flags.
Add argument and guard coverage for elevateCloud/elevateGroup/resolveAndElevate payloads, the four grant env --favorite paths, ErrorInfo handling, the three empty-results guards, total multi-CSP failure, env's fresh post-prompt context, the auth cache flag, selector input and --group/--groups precedence. Extract shouldShowVerboseHint so the hint condition is testable without restating it, drop a duplicated multi-CSP case and widen the concurrency bound.
All 42 argument-capture rows reverified fail-to-pass with -count=1; records the mutation forms used where the literal form does not compile.
Extracting shouldShowVerboseHint made the predicate testable but left the call site unpinned: executeWithHint calls the same predicate, so deleting the whole if-block from Execute() kept go test ./cmd/ green. TestIntegration_VerboseHint drives the compiled binary and asserts the hint is present on a runtime error and absent for an unknown subcommand, an unknown flag, and an already-verbose run.
TestRunRequestSubmit_MissingFlags_NonInteractive relied on go test's ambient non-TTY stdin, the one violation of the MUST added in PR4. It fails under a forced TTY; withInteractiveTTY(t, false) makes the precondition its name already claims an assertion. Also switches the GCP negative assertion to len(submitCalls) != 0, which distinguishes 'never called' from 'called with nil'.
TestRunLogin_AutoConfiguresMissingProfile reaches runConfigure, and survey writes prompts straight to os.Stdout rather than the cobra buffer, so every go test ./cmd/ emitted control sequences. One of them, ESC[6n, makes the terminal reply on stdin and corrupt the shell prompt. withDiscardedStdout redirects os.Stdout for the test's duration, restoring it via t.Cleanup, with no production prompt seam.
The CLAUDE.md note claimed the old production cmd/test_helpers.go 'linked test-only code into the binary'. It did not: that file imported only bytes and cobra, and 'go list -deps . | rg testing' matches on neither the base nor HEAD. The move was preventive. The two remaining lastSubmit() != nil negatives become len(submitCalls) != 0, matching the elevateCalls form already used in the env tests and distinguishing 'never called' from 'called with nil'.
# Conflicts: # docs/mutation-ledger.md
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Part 4 of 8. Base:
test/isolation-harness(#63) — must merge first.Why
The shared mocks in
cmd/test_mocks.godiscarded their arguments, so tests asserted against canned mock return values rather than what the command sent. Verified consequences that shipped green:grant request rejectcould sendAPPROVED— the tests only checked the printed word, which the command generates locally.--state PENDINGcould list everything;--descinverted;--searchdropped.WorkspaceID/RoleIDor blank the CSP and org ID.validateSubmitFieldscall site was undetectable — the validator was only tested in isolation.What
Capture merged into
mockAccessRequestService(the capturing subclass is deleted), plusmockElevateServiceandmockGroupsElevator. History slices withlastX()accessors, defensive copies, andreason string+reasonSet boolsonilis distinguishable from"".No mutex, deliberately. Only listers fan out (
helpers.go:44,59,71;root.go:316,326); the elevate and request-service call sites are joined before any elevate path is reached, so concurrent access is unreachable, not merely unobserved. Independently re-derived in review, plus-race -count=20and-race -shuffle=on -count=5clean.Also closes:
findItemByDisplayreturning the wrong item (silent wrong-target elevation), the fourenv --favoritepaths, group/envErrorInfohandling (a POLICY_DENIED result printed success and exited 0), three empty-results guards, total multi-CSP failure, and at.Skipwhose stated reason was factually wrong.Notes
cmd/test_helpers.gowas a production file; addingwithInteractiveTTYto it would have importedtestinginto the shipped binary. Moved to_test.go.go list -deps . | grep -c '^testing$'→ 0 on both base and head.shouldShowVerboseHintmadeexecuteWithHintunable to disagree withExecute(), so deleting the hint block passed the unit suite. Pinned from the integration side instead — and the ledger records that dropping-tags=integrationfrom CI would unpin it again.go test's stdin happens not to be a TTY. Now explicit.42 ledger rows, all mutation-reverified. Adversarial review performed (Codex credits exhausted; review by a Claude agent). All findings fixed.