Skip to content

test(cli): make engine-version error-handling tests hermetic - #2207

Merged
cristim merged 1 commit into
mainfrom
test/hermetic-engine-version-errors
Oct 16, 2026
Merged

cristim merged 1 commit into
mainfrom
test/hermetic-engine-version-errors

Conversation

@cristim

@cristim cristim commented Oct 16, 2026

Copy link
Copy Markdown
Member

Closes #1328

Summary

TestQueryMajorEngineVersions_ErrorHandling and TestQueryRunningInstanceEngineVersions_ErrorHandling loaded the real AWS config and made live API calls. Each run took 34s and 9s. Both asserted only inside if err != nil, so they passed whether or not credentials existed.

  • I removed TestQueryMajorEngineVersions_ErrorHandling. The existing TestQueryMajorEngineVersions_ProfileSelection (cmd/multi_service_coverage_test.go:108) already covers its profile cases. Its engine-error case is covered by the stubbed TestQueryMajorEngineVersionsWithClient_EngineErrorContinues (:83) and _EngineFailuresAreAggregated (cmd/multi_service_extended_support_unavailable_test.go:259). The only case without a replacement is "empty profile", which asserted nothing beyond not panicking.
  • I replaced the running-instance test with TestQueryRunningInstanceEngineVersions_ProfileSelection. It uses isolateAWSEnv and nonexistent profiles, so config loading fails before any AWS call. It asserts unconditionally that the call errors, that the error names the selected profile, and that both result maps are nil.
  • This is a test-only change, with no production change.

Deviation from the issue: the issue suggests extracting a queryRunningInstanceEngineVersionsWithClient seam. I didn't add it. The region and engine failure paths already go through queryRDSInstancesInRegions (cmd/multi_service_engine_versions.go:61), which TestQueryRDSInstancesInRegions_* already stubs: all regions failing, mid-pagination failure, worker panic, empty inventory, and a cancelled context.

Evidence (local, macOS, GOTOOLCHAIN=go1.26.9)

  • Mutation probes:
    • Making loadValidationAWSConfig ignore the validation profile fails the "validation profile takes precedence" case.
    • Making queryRunningInstanceEngineVersions return a nil error fails both cases.
    • An independent reviewer reproduced the first probe in a scratch copy.
  • Timing: the new test takes 0.00s with the AWS env vars unset; the removed tests took 34s and 9s.
  • Checks:
    • make build: ok.
    • go vet ./...: clean.
    • go test -race ./cmd/...: 1433 passed.
    • golangci-lint v2.10.1 (the CI pin): 0 issues.
    • Pre-commit hooks: pass.
  • Review: two passes by an independent non-author reviewer (sonnet, high effort), with no actionable findings at c7f0e34.
  • Separate finding, not caused by this diff: go test -shuffle=on -count=2 ./cmd/ fails the flag-validation tests through shared global flag state. The same seed fails on a clean origin/main copy. I'm filing it as its own issue.

🤖 Generated with Claude Code

TestQueryMajorEngineVersions_ErrorHandling and
TestQueryRunningInstanceEngineVersions_ErrorHandling loaded the real AWS
config and issued live API calls (34s and 9s per run), and only asserted
inside `if err != nil`, so they passed whether or not credentials existed.

Remove the major-versions test: profile precedence is covered by
TestQueryMajorEngineVersions_ProfileSelection and engine failures by the
stubbed TestQueryMajorEngineVersionsWithClient_* tests. Replace the
running-instance test with TestQueryRunningInstanceEngineVersions_ProfileSelection,
which uses isolateAWSEnv and nonexistent profiles so config loading fails
before any AWS call, and unconditionally asserts the error names the
selected profile. Region and engine failure paths already have stubbed
tests (TestQueryRDSInstancesInRegions_*), so no production seam is added.

Closes #1328

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@cristim cristim added priority/p2 Backlog-worthy severity/medium Moderate harm urgency/this-quarter Within the quarter impact/internal Team-internal only effort/s Hours type/chore Maintenance / non-user-visible triaged Item has been triaged labels Oct 16, 2026
@coderabbitai

coderabbitai Bot commented Oct 16, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

This review includes 1 billable file and costs up to $0.25.

  • Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 28 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 65 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Repository: LeanerCloud/cloud-commitments-cli/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Essentials
  • Run ID: bfa051c8-55a7-4980-bac9-8b5d94d79a76
📥 Commits

Reviewing files that changed from the base of the PR and between 817ab4b and c7f0e34.

📒 Files selected for processing: 1
  • cmd/multi_service_engine_versions_test.go
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@cristim

cristim commented Oct 16, 2026

Copy link
Copy Markdown
Member Author

Merge-gate verdict: CLEAN at c7f0e34

Independent adversarial review (non-author agent, Sonnet high, cold brief, full diff origin/main...c7f0e345): no blocking findings. This is a test-only change with no production code touched.

Coverage was checked case by case against the deleted tests. All of the old assertions were guarded by if err != nil, so they were vacuous, and on a credentialed machine the tests made live AWS calls.

  • queryMajorEngineVersions "validation profile" and "fallback to main" are covered unconditionally by TestQueryMajorEngineVersions_ProfileSelection. The engine-error path is covered by EngineErrorContinues and EngineFailuresAreAggregated.
  • queryRunningInstanceEngineVersions is now covered by TestQueryRunningInstanceEngineVersions_ProfileSelection (uses isolateAWSEnv; asserts the error, nil results and the selected profile unconditionally). The test matches production at multi_service_engine_versions.go:50-83.

Non-blocking notes:

Local verification on macOS at c7f0e34:

  • Hermeticity: with AWS_REGION=bogus AWS_PROFILE=bogus HTTPS_PROXY=http://127.0.0.1:9 AWS_ACCESS_KEY_ID= AWS_SECRET_ACCESS_KEY= AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null, go test ./cmd -run 'QueryRunningInstanceEngineVersions|QueryMajorEngineVersions|QueryRDSInstancesInRegions' -count=1 -v passed all 15 top-level tests in 2.3s.
  • Mutation probes, each reverted at once: making loadValidationAWSConfig use cfg.Profile fails "validation profile takes precedence", and short-circuiting queryRunningInstanceEngineVersions to nil, nil, nil fails "falls back to main profile".
  • Full suite: go test ./... reported ok cmd 30.6s. go vet ./... (go1.26.9 per go.work) was clean.

CI is green on c7f0e34, mergeability is CLEAN, and labels are mirrored from #1328.

CR waived: quota, adversarial review + local verification + green CI. A retrospective CR review is tracked as a follow-up.

🤖 Generated with Claude Code

@cristim
cristim merged commit 1836029 into main Oct 16, 2026
12 checks passed
@cristim
cristim deleted the test/hermetic-engine-version-errors branch October 16, 2026 08:08
IAmJSD added a commit to IAmJSD/cloud-commitments-cli that referenced this pull request Oct 16, 2026
Resolve test-file conflict with LeanerCloud#2207: keep this PR's
TestIsInExtendedSupport_ZeroStartDateWarns and main's
TestQueryRunningInstanceEngineVersions_ProfileSelection; drop
TestQueryMajorEngineVersions_ErrorHandling as main deleted it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/s Hours impact/internal Team-internal only priority/p2 Backlog-worthy severity/medium Moderate harm triaged Item has been triaged type/chore Maintenance / non-user-visible urgency/this-quarter Within the quarter

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Replace live-AWS sibling tests in multi_service_engine_versions_test.go (followup to #1225)

1 participant