Skip to content

test(cli): keep the CSV, processService and fetchAllRecs tests off the network - #2225

Merged
cristim merged 3 commits into
mainfrom
test/hermetic-csv-and-sp-tests
Oct 16, 2026
Merged

cristim merged 3 commits into
mainfrom
test/hermetic-csv-and-sp-tests

Conversation

@cristim

@cristim cristim commented Oct 16, 2026

Copy link
Copy Markdown
Member

Closes #2162

Summary

14 cmd tests sent real requests to AWS endpoints. This PR makes them hermetic. The change is test-only, with no production code changes. Measured with a CONNECT-logging proxy, the sweep goes from 74 off-host connections to 0.

  1. The AWS fixture stub moves to cmd/aws_fixture_stub_test.go. The stub came from test(cli): route MaxInstances duplicate checks to a stub transport #2215. The move is a straight cut, rename and paste with no behaviour change. The stub is then extended:

    • It understands JSON (ce), REST-path (savingsplans) and Query operation keys.
    • Fixtures are region-agnostic and can carry an HTTP status.
    • Fixtures are per instance, so a test can override one with an error response.
    • requireRequests asserts what the stub actually received.
  2. isolateAWSEnv points the SDK at a local deny endpoint. It starts an httptest server that answers 403 and sets AWS_ENDPOINT_URL to it. It also clears AWS_IGNORE_CONFIGURED_ENDPOINT_URLS and every AWS_ENDPOINT_URL_* variable, so a developer's own endpoint settings can't send requests off the machine. This covers the five runToolFromCSV tests, which build their config internally. The 403 is not retried, so these tests take the same warn-and-continue path they took before with invalid credentials.

  3. Nine literal-config tests use the stub through aws.Config.HTTPClient. These are five processService tests, two fetchAllRecs tests, TestFetchExistingCoverage_FetchFailure and TestGetAllAWSRegions. Most are dry runs and take the empty-fixture success path. Two keep a failure path, because that is what they test:

    • TestProcessServiceWithMocks keeps an ElastiCache 403 and now asserts that the real-purchase case returns no results.
    • TestFetchExistingCoverage_FetchFailure uses a ce 400.

    TestGetAllAWSRegions used to skip its subtest without credentials. It now runs unconditionally on an ec2 fixture and asserts exactly one request.

The issue's TestCSVCap* claims were stale: those tests, TestCSVMinCount* and TestFilterAndAdjustRecommendations_MaxInstancesApplied already made 0 requests, because TestMain stubs the engine-version and caller-account fetchers.

Evidence

All runs are local, on macOS, with GOTOOLCHAIN=go1.26.9, using fixtures and a CONNECT-logging proxy.

Group CONNECTs before CONNECTs after
runToolFromCSV (5 tests) 37 0
processService (5 tests) 16 0
fetchAllRecs (2 tests) 12 0
FetchExistingCoverage 6 0
GetAllAWSRegions 3 0
  • Whole package: 74 CONNECTs before and 0 after. An independent reviewer confirmed the 0 behind a proxy whose canary connection was logged.
  • Hostile environment: with ambient credentials, AWS_IGNORE_CONFIGURED_ENDPOINT_URLS=true and service-specific AWS_ENDPOINT_URL_* overrides set, the 14 tests pass with 0 CONNECTs.
  • Mutation probes: each of these makes the named test fail.
    • No AWS_ENDPOINT_URL set: TestRunToolFromCSV.
    • The savingsplans, ce or ec2 fixture deleted: the test that uses it.
    • A malformed rds body: TestProcessService_WithInstanceLimit.
    • The ElastiCache 403 override removed: TestProcessServiceWithMocks.
    • A literal config restored: requireRequests.
  • Checks:
  • Review:
    • The plan was reviewed in two adversarial passes by an independent reviewer (sonnet, high). That review added clearing the endpoint overrides, the whole-package complement and the per-test fail-or-success choices.
    • The same reviewer then reviewed the diff in two passes. There were no actionable findings at 7a7a427, after a rebase onto current main with no content change.

Follow-ups filed

🤖 Generated with Claude Code

cristim and others added 3 commits October 16, 2026 16:01
The offline AWS stub added for the max-instances duplicate check is needed
by tests in several other files. Move it to cmd/aws_fixture_stub_test.go and
rename it to awsFixtureStub / newAWSFixtureConfig. No behaviour change.

Refs #2162

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
isolateAWSEnv only set invalid static credentials, so the runToolFromCSV
tests still sent requests to the real organizations, rds and elasticache
endpoints and failed on the 403 from there. Serve a local 403
AccessDeniedException through AWS_ENDPOINT_URL instead, clear
AWS_IGNORE_CONFIGURED_ENDPOINT_URLS and every AWS_ENDPOINT_URL_* so a
developer's overrides cannot bypass it, and return the request counter.
TestRunToolFromCSV asserts the counter so the wiring cannot go vacuous.

Refs #2162

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… stub

Nine tests built an aws.Config literal with no HTTP client, so their
duplicate, coverage and region lookups went to the real rds, elasticache,
savingsplans, ce and ec2 endpoints. Inject the shared fixture stub instead
and assert the requests it served, so a test cannot pass without reaching it.

The stub learns three things for these callers: the operation comes from the
form Action, else X-Amz-Target, else the URL path tail; the fixture lookup
ignores the region in the host; a fixture may carry a status. The tests keep
the path they exercised before: the fetch-failure and fail-closed real
purchase cases get an error fixture, the dry-run cases an empty one, and
the GetAllAWSRegions integration subtest runs against a fixture and no longer
needs real credentials.

Closes #2162

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/m Days type/bug Defect 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 6 billable files and costs up to $1.50.

  • 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 22 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 64 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: c13ae3b3-add9-4ce9-ad58-1080f541b038
📥 Commits

Reviewing files that changed from the base of the PR and between e8543f0 and 7a7a427.

📒 Files selected for processing: 6
  • cmd/aws_fixture_stub_test.go
  • cmd/cancellation_prepurchase_test.go
  • cmd/multi_service_coverage_test.go
  • cmd/multi_service_helpers_test.go
  • cmd/multi_service_max_instances_test.go
  • cmd/multi_service_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

Independent review verdict: CLEAN at 7a7a427

Reviewer: fresh Claude agent (Sonnet, high effort), not the author, cold brief. Coordinator: claude-cc-reviewer.

Blocking findings: none.

Non-blocking notes:

  • The fixtures are now region-agnostic. The old stub accepted only three regions, so a duplicate check against a new region used to fail. The pair count and the exactly-one-per-key assertions still hold, so this is a minor relaxation.
  • isolateAWSEnv clears AWS_ENDPOINT_URL_* via t.Setenv(name, ""), which relies on empty meaning unset (true for the SDK).

What was attacked and how (GOTOOLCHAIN=go1.26.9, AWS config at /dev/null, HTTPS_PROXY=http://127.0.0.1:9, AWS_EC2_METADATA_DISABLED=true):

  • Network: the touched tests on origin/main produce 17 "connection refused" lines and take 58s. At this head they produce 0 and take 3.8s. The 9 remaining "Could not check" lines are the intended warn-and-continue paths from the local stub.
  • Assertions: only the conditional, skip-if-no-credentials TestGetAllAWSRegions subtest was removed. Its replacement asserts unconditionally (NoError, sorted regions, exactly 1 request). Everything else only adds assertions.
  • Reuse: the test(cli): route MaxInstances duplicate checks to a stub transport #2215 stub moved verbatim into aws_fixture_stub_test.go. multi_service_max_instances_test.go uses it at 5 sites, so nothing is duplicated.
  • Global state: fixtures are per instance (maps.Clone), env changes go through t.Setenv, and the httptest server is closed in t.Cleanup.
  • go vet ./cmd/ is clean. go test -race -count=1 ./cmd/... passes 1469, and -race -count=10 -shuffle=on on the touched tests passes 670.
  • Mutation probes: deleting the DescribeRegions fixture and removing t.Setenv("AWS_ENDPOINT_URL", ...) each make a test fail.

Gate:

@cristim
cristim merged commit c877d28 into main Oct 16, 2026
12 checks passed
@cristim
cristim deleted the test/hermetic-csv-and-sp-tests branch October 16, 2026 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/m Days impact/internal Team-internal only priority/p2 Backlog-worthy severity/medium Moderate harm triaged Item has been triaged type/bug Defect urgency/this-quarter Within the quarter

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test(cmd): existing tests reach real AWS endpoints

1 participant