From c4d55103d3e166c652c3ed8382bd0b26c824b843 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 16 Oct 2026 14:59:19 +0200 Subject: [PATCH 1/3] test(cli): move the AWS fixture stub to a shared test file 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 --- cmd/aws_fixture_stub_test.go | 101 ++++++++++++++++++++++++ cmd/multi_service_max_instances_test.go | 101 +----------------------- 2 files changed, 105 insertions(+), 97 deletions(-) create mode 100644 cmd/aws_fixture_stub_test.go diff --git a/cmd/aws_fixture_stub_test.go b/cmd/aws_fixture_stub_test.go new file mode 100644 index 000000000..045260e5d --- /dev/null +++ b/cmd/aws_fixture_stub_test.go @@ -0,0 +1,101 @@ +package main + +import ( + "bytes" + "fmt" + "io" + "net/http" + "net/url" + "strings" + "sync" + "testing" + + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// awsFixtureStub is the offline AWS endpoint behind the duplicate check +// (checkDuplicates -> GetExistingCommitments). Every request must match a +// fixture keyed "host Action"; anything else fails the test, so a request that +// would have left the machine is reported instead of silently timing out. +type awsFixtureStub struct { + t *testing.T + mu sync.Mutex + received map[string]int + log *bytes.Buffer +} + +// awsFixtures answer the Query-protocol DescribeReserved* calls +// with an empty existing-commitment list. +var awsFixtures = map[string]string{ + "rds.us-east-1.amazonaws.com DescribeReservedDBInstances": emptyRDSFixture, + "rds.us-west-2.amazonaws.com DescribeReservedDBInstances": emptyRDSFixture, + "rds.eu-west-1.amazonaws.com DescribeReservedDBInstances": emptyRDSFixture, + "elasticache.us-east-1.amazonaws.com DescribeReservedCacheNodes": emptyElastiCacheFixture, + "elasticache.us-west-2.amazonaws.com DescribeReservedCacheNodes": emptyElastiCacheFixture, + "elasticache.eu-west-1.amazonaws.com DescribeReservedCacheNodes": emptyElastiCacheFixture, +} + +const ( + emptyRDSFixture = `` + + `` + + `fixture` + emptyElastiCacheFixture = `` + + `` + + `fixture` +) + +func (s *awsFixtureStub) RoundTrip(r *http.Request) (*http.Response, error) { + b, _ := io.ReadAll(r.Body) + form, _ := url.ParseQuery(string(b)) + key := r.URL.Host + " " + form.Get("Action") + s.mu.Lock() + s.received[key]++ + s.mu.Unlock() + body, ok := awsFixtures[key] + status := http.StatusOK + if !ok { + s.t.Errorf("unexpected AWS request %s %s%s", r.Method, key, r.URL.Path) + status, body = http.StatusInternalServerError, "unexpected request" + } + return &http.Response{ + StatusCode: status, + Header: http.Header{"Content-Type": []string{"text/xml"}}, + Body: io.NopCloser(strings.NewReader(body)), + Request: r, + }, nil +} + +// newAWSFixtureConfig returns an aws.Config whose HTTP client is the +// stub and whose credentials are anonymous, so requests are unsigned and need +// no environment. +func newAWSFixtureConfig(t *testing.T) (aws.Config, *awsFixtureStub) { + t.Helper() + stub := &awsFixtureStub{t: t, received: map[string]int{}, log: captureAppLog(t)} + return aws.Config{ + Region: "us-east-1", + HTTPClient: &http.Client{Transport: stub}, + Credentials: aws.AnonymousCredentials{}, + Retryer: func() aws.Retryer { return aws.NopRetryer{} }, + }, stub +} + +// requireDuplicateChecks asserts the duplicate check really ran against the +// stub: one call per (service, region) pair, no other request, and every +// response parsed. +func (s *awsFixtureStub) requireDuplicateChecks(t *testing.T, pairs int) { + t.Helper() + s.mu.Lock() + defer s.mu.Unlock() + total := 0 + for k, n := range s.received { + assert.Contains(t, awsFixtures, k) + assert.Equalf(t, 1, n, "duplicate-check requests for %s", k) + total += n + } + // A fixture the SDK cannot parse only logs this warning in dry-run, so the + // request count alone would not notice a broken fixture. + assert.NotContains(t, s.log.String(), "Could not check for existing RIs") + require.Equal(t, pairs, total, fmt.Sprintf("duplicate-check requests received: %v", s.received)) +} diff --git a/cmd/multi_service_max_instances_test.go b/cmd/multi_service_max_instances_test.go index 51abe204e..1dff4d972 100644 --- a/cmd/multi_service_max_instances_test.go +++ b/cmd/multi_service_max_instances_test.go @@ -1,109 +1,16 @@ package main import ( - "bytes" "context" - "fmt" - "io" - "net/http" - "net/url" "path/filepath" - "strings" - "sync" "testing" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" - "github.com/aws/aws-sdk-go-v2/aws" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" ) -// maxInstancesAWSStub is the offline AWS endpoint behind the duplicate check -// (checkDuplicates -> GetExistingCommitments). Every request must match a -// fixture keyed "host Action"; anything else fails the test, so a request that -// would have left the machine is reported instead of silently timing out. -type maxInstancesAWSStub struct { - t *testing.T - mu sync.Mutex - received map[string]int - log *bytes.Buffer -} - -// maxInstancesAWSFixtures answer the Query-protocol DescribeReserved* calls -// with an empty existing-commitment list. -var maxInstancesAWSFixtures = map[string]string{ - "rds.us-east-1.amazonaws.com DescribeReservedDBInstances": maxInstancesEmptyRDS, - "rds.us-west-2.amazonaws.com DescribeReservedDBInstances": maxInstancesEmptyRDS, - "rds.eu-west-1.amazonaws.com DescribeReservedDBInstances": maxInstancesEmptyRDS, - "elasticache.us-east-1.amazonaws.com DescribeReservedCacheNodes": maxInstancesEmptyElastiCache, - "elasticache.us-west-2.amazonaws.com DescribeReservedCacheNodes": maxInstancesEmptyElastiCache, - "elasticache.eu-west-1.amazonaws.com DescribeReservedCacheNodes": maxInstancesEmptyElastiCache, -} - -const ( - maxInstancesEmptyRDS = `` + - `` + - `fixture` - maxInstancesEmptyElastiCache = `` + - `` + - `fixture` -) - -func (s *maxInstancesAWSStub) RoundTrip(r *http.Request) (*http.Response, error) { - b, _ := io.ReadAll(r.Body) - form, _ := url.ParseQuery(string(b)) - key := r.URL.Host + " " + form.Get("Action") - s.mu.Lock() - s.received[key]++ - s.mu.Unlock() - body, ok := maxInstancesAWSFixtures[key] - status := http.StatusOK - if !ok { - s.t.Errorf("unexpected AWS request %s %s%s", r.Method, key, r.URL.Path) - status, body = http.StatusInternalServerError, "unexpected request" - } - return &http.Response{ - StatusCode: status, - Header: http.Header{"Content-Type": []string{"text/xml"}}, - Body: io.NopCloser(strings.NewReader(body)), - Request: r, - }, nil -} - -// newMaxInstancesAWSConfig returns an aws.Config whose HTTP client is the -// stub and whose credentials are anonymous, so requests are unsigned and need -// no environment. -func newMaxInstancesAWSConfig(t *testing.T) (aws.Config, *maxInstancesAWSStub) { - t.Helper() - stub := &maxInstancesAWSStub{t: t, received: map[string]int{}, log: captureAppLog(t)} - return aws.Config{ - Region: "us-east-1", - HTTPClient: &http.Client{Transport: stub}, - Credentials: aws.AnonymousCredentials{}, - Retryer: func() aws.Retryer { return aws.NopRetryer{} }, - }, stub -} - -// requireDuplicateChecks asserts the duplicate check really ran against the -// stub: one call per (service, region) pair, no other request, and every -// response parsed. -func (s *maxInstancesAWSStub) requireDuplicateChecks(t *testing.T, pairs int) { - t.Helper() - s.mu.Lock() - defer s.mu.Unlock() - total := 0 - for k, n := range s.received { - assert.Contains(t, maxInstancesAWSFixtures, k) - assert.Equalf(t, 1, n, "duplicate-check requests for %s", k) - total += n - } - // A fixture the SDK cannot parse only logs this warning in dry-run, so the - // request count alone would not notice a broken fixture. - assert.NotContains(t, s.log.String(), "Could not check for existing RIs") - require.Equal(t, pairs, total, fmt.Sprintf("duplicate-check requests received: %v", s.received)) -} - // maxInstancesFixtureServices and maxInstancesFixtureRegions describe the // multi-service, multi-region fan-out the run-wide cap has to survive. A // single-service or single-region fixture stays green with the per-region bug @@ -169,7 +76,7 @@ func TestMaxInstancesCapsWholeRunAcrossServicesAndRegions(t *testing.T) { const maxInstances = 10 ctx := context.Background() - awsCfg, awsStub := newMaxInstancesAWSConfig(t) + awsCfg, awsStub := newAWSFixtureConfig(t) origCfg := toolCfg t.Cleanup(func() { toolCfg = origCfg }) @@ -242,7 +149,7 @@ func TestMaxInstancesKeepsHighestSavingsNotFirstFetched(t *testing.T) { ) ctx := context.Background() - awsCfg, awsStub := newMaxInstancesAWSConfig(t) + awsCfg, awsStub := newAWSFixtureConfig(t) origCfg := toolCfg t.Cleanup(func() { toolCfg = origCfg }) @@ -317,7 +224,7 @@ func TestMaxInstancesNeverPurchasesBelowMinCount(t *testing.T) { ) ctx := context.Background() - awsCfg, awsStub := newMaxInstancesAWSConfig(t) + awsCfg, awsStub := newAWSFixtureConfig(t) origCfg := toolCfg t.Cleanup(func() { toolCfg = origCfg }) @@ -407,7 +314,7 @@ func TestDropTruncatedBelowMinCount(t *testing.T) { // run that never asked for one. func TestMaxInstancesNotAppliedWhenUnset(t *testing.T) { ctx := context.Background() - awsCfg, awsStub := newMaxInstancesAWSConfig(t) + awsCfg, awsStub := newAWSFixtureConfig(t) origCfg := toolCfg t.Cleanup(func() { toolCfg = origCfg }) From 14b934024865438a778b73f21205d160b5dcd2f1 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 16 Oct 2026 15:01:00 +0200 Subject: [PATCH 2/3] test(cli): point isolateAWSEnv at a local deny endpoint 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 --- cmd/multi_service_test.go | 38 ++++++++++++++++++++++++++++++++------ 1 file changed, 32 insertions(+), 6 deletions(-) diff --git a/cmd/multi_service_test.go b/cmd/multi_service_test.go index 99644bf9b..9568c0eb9 100644 --- a/cmd/multi_service_test.go +++ b/cmd/multi_service_test.go @@ -7,10 +7,13 @@ import ( "encoding/json" "fmt" "log" + "net/http" + "net/http/httptest" "os" "path/filepath" "strconv" "strings" + "sync/atomic" "testing" "time" @@ -1627,11 +1630,12 @@ func TestFilterAndAdjustRecommendations(t *testing.T) { } // isolateAWSEnv pins the AWS environment to deterministic invalid static -// credentials so the runToolFromCSV tests behave identically on credentialed -// developer machines and bare CI runners: config loading succeeds, every -// outbound AWS API call fails and takes the warn-and-continue path, and no -// real account is ever touched. -func isolateAWSEnv(t *testing.T) { +// credentials and points every SDK client at a local server that answers 403 +// AccessDenied, so the runToolFromCSV tests behave identically on credentialed +// developer machines and bare CI runners: config loading succeeds, every AWS +// API call fails fast on the warn-and-continue path, and nothing leaves the +// machine. It returns the number of requests the local server has received. +func isolateAWSEnv(t *testing.T) *atomic.Int64 { t.Helper() t.Setenv("AWS_ACCESS_KEY_ID", "invalid-test-key-id") t.Setenv("AWS_SECRET_ACCESS_KEY", "invalid-test-secret") @@ -1640,6 +1644,26 @@ func isolateAWSEnv(t *testing.T) { t.Setenv("AWS_CONFIG_FILE", os.DevNull) t.Setenv("AWS_SHARED_CREDENTIALS_FILE", os.DevNull) t.Setenv("AWS_EC2_METADATA_DISABLED", "true") + + hits := &atomic.Int64{} + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + hits.Add(1) + w.Header().Set("Content-Type", "application/x-amz-json-1.1") + w.Header().Set("X-Amzn-ErrorType", "AccessDeniedException") + w.WriteHeader(http.StatusForbidden) + _, _ = w.Write([]byte(`{"__type":"AccessDeniedException","message":"denied by test"}`)) + })) + t.Cleanup(srv.Close) + + // A developer's global endpoint overrides must not bypass the local server. + for _, kv := range os.Environ() { + if name, _, _ := strings.Cut(kv, "="); strings.HasPrefix(name, "AWS_ENDPOINT_URL_") { + t.Setenv(name, "") + } + } + t.Setenv("AWS_IGNORE_CONFIGURED_ENDPOINT_URLS", "") + t.Setenv("AWS_ENDPOINT_URL", srv.URL) + return hits } // readPurchaseReport parses the CSV purchase report written by runToolFromCSV @@ -1693,7 +1717,7 @@ func writeTestRecommendationsCSV(t *testing.T, csvData string) string { func TestRunToolFromCSV(t *testing.T) { origCfg := toolCfg defer func() { toolCfg = origCfg }() - isolateAWSEnv(t) + awsHits := isolateAWSEnv(t) // Column names match writeMultiServiceCSVReport's header (the real // round-trip shape). The previous fixture used headers the parser @@ -1750,6 +1774,8 @@ elasticache,us-west-2,cache.t3.micro,redis,1,1yr,all-upfront,111111111111 } }) } + // Without a request the env wiring in isolateAWSEnv would be vacuous. + assert.Positive(t, awsHits.Load(), "isolateAWSEnv's local endpoint must have served the duplicate checks") } // TestRunToolFromCSV_DuplicateCheckFailureRefusesToPurchase reproduces #1941: From 7a7a4274d79fe25ad8c7888ebfc34c96be5caa20 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 16 Oct 2026 15:09:24 +0200 Subject: [PATCH 3/3] test(cli): route processService and fetchAllRecs tests to the fixture 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 --- cmd/aws_fixture_stub_test.go | 97 +++++++++++++++++++++------- cmd/cancellation_prepurchase_test.go | 16 ++++- cmd/multi_service_coverage_test.go | 5 +- cmd/multi_service_helpers_test.go | 22 ++----- cmd/multi_service_test.go | 29 +++++++-- 5 files changed, 119 insertions(+), 50 deletions(-) diff --git a/cmd/aws_fixture_stub_test.go b/cmd/aws_fixture_stub_test.go index 045260e5d..cb1353fe3 100644 --- a/cmd/aws_fixture_stub_test.go +++ b/cmd/aws_fixture_stub_test.go @@ -4,8 +4,10 @@ import ( "bytes" "fmt" "io" + "maps" "net/http" "net/url" + "path" "strings" "sync" "testing" @@ -15,26 +17,34 @@ import ( "github.com/stretchr/testify/require" ) -// awsFixtureStub is the offline AWS endpoint behind the duplicate check -// (checkDuplicates -> GetExistingCommitments). Every request must match a -// fixture keyed "host Action"; anything else fails the test, so a request that -// would have left the machine is reported instead of silently timing out. +// awsFixtureStub is the offline AWS endpoint behind the tests that build their +// own aws.Config. Every request must match a fixture keyed "service operation"; +// anything else fails the test, so a request that would have left the machine +// is reported instead of silently timing out. The region in the host is +// ignored for the lookup but kept in received ("host operation"). type awsFixtureStub struct { t *testing.T mu sync.Mutex received map[string]int + fixtures map[string]awsFixture log *bytes.Buffer } -// awsFixtures answer the Query-protocol DescribeReserved* calls -// with an empty existing-commitment list. -var awsFixtures = map[string]string{ - "rds.us-east-1.amazonaws.com DescribeReservedDBInstances": emptyRDSFixture, - "rds.us-west-2.amazonaws.com DescribeReservedDBInstances": emptyRDSFixture, - "rds.eu-west-1.amazonaws.com DescribeReservedDBInstances": emptyRDSFixture, - "elasticache.us-east-1.amazonaws.com DescribeReservedCacheNodes": emptyElastiCacheFixture, - "elasticache.us-west-2.amazonaws.com DescribeReservedCacheNodes": emptyElastiCacheFixture, - "elasticache.eu-west-1.amazonaws.com DescribeReservedCacheNodes": emptyElastiCacheFixture, +// awsFixture is one canned response; a zero status means 200. +type awsFixture struct { + body string + status int +} + +// awsFixtures hold the default per-operation responses: empty existing +// commitments, one SDK-parseable body per protocol (Query XML, rest-json). +var awsFixtures = map[string]awsFixture{ + "rds DescribeReservedDBInstances": {body: emptyRDSFixture}, + "elasticache DescribeReservedCacheNodes": {body: emptyElastiCacheFixture}, + "savingsplans DescribeSavingsPlans": {body: `{"savingsPlans":[]}`}, + "ec2 DescribeRegions": {body: describeRegionsFixture}, + // A non-retryable error, for tests of the fetch-failure path. + "ce GetReservationCoverage": {status: http.StatusBadRequest, body: `{"__type":"AccessDeniedException","message":"denied by fixture"}`}, } const ( @@ -44,25 +54,51 @@ const ( emptyElastiCacheFixture = `` + `` + `fixture` + // Unsorted on purpose: getAllAWSRegions must sort. + describeRegionsFixture = `fixture` + + `us-west-2eu-west-1` ) +// operation names the API call: the Query "Action", else the JSON +// X-Amz-Target suffix, else the last path segment (rest-json). +func awsOperation(r *http.Request, form url.Values) string { + if a := form.Get("Action"); a != "" { + return a + } + if tgt := r.Header.Get("X-Amz-Target"); tgt != "" { + _, op, _ := strings.Cut(tgt, ".") + return op + } + return path.Base(r.URL.Path) +} + func (s *awsFixtureStub) RoundTrip(r *http.Request) (*http.Response, error) { - b, _ := io.ReadAll(r.Body) + var b []byte + if r.Body != nil { + b, _ = io.ReadAll(r.Body) + } form, _ := url.ParseQuery(string(b)) - key := r.URL.Host + " " + form.Get("Action") + op := awsOperation(r, form) s.mu.Lock() - s.received[key]++ + s.received[r.URL.Host+" "+op]++ s.mu.Unlock() - body, ok := awsFixtures[key] - status := http.StatusOK + svc, _, _ := strings.Cut(r.URL.Host, ".") + fx, ok := s.fixtures[svc+" "+op] if !ok { - s.t.Errorf("unexpected AWS request %s %s%s", r.Method, key, r.URL.Path) - status, body = http.StatusInternalServerError, "unexpected request" + s.t.Errorf("unexpected AWS request %s %s %s", r.Method, r.URL.Host, op) + fx = awsFixture{status: http.StatusInternalServerError, body: "unexpected request"} + } + if fx.status == 0 { + fx.status = http.StatusOK + } + contentType := "application/x-amz-json-1.1" + if strings.HasPrefix(fx.body, "<") { + contentType = "text/xml" } return &http.Response{ - StatusCode: status, - Header: http.Header{"Content-Type": []string{"text/xml"}}, - Body: io.NopCloser(strings.NewReader(body)), + StatusCode: fx.status, + Header: http.Header{"Content-Type": []string{contentType}}, + Body: io.NopCloser(strings.NewReader(fx.body)), Request: r, }, nil } @@ -72,7 +108,7 @@ func (s *awsFixtureStub) RoundTrip(r *http.Request) (*http.Response, error) { // no environment. func newAWSFixtureConfig(t *testing.T) (aws.Config, *awsFixtureStub) { t.Helper() - stub := &awsFixtureStub{t: t, received: map[string]int{}, log: captureAppLog(t)} + stub := &awsFixtureStub{t: t, received: map[string]int{}, fixtures: maps.Clone(awsFixtures), log: captureAppLog(t)} return aws.Config{ Region: "us-east-1", HTTPClient: &http.Client{Transport: stub}, @@ -81,6 +117,15 @@ func newAWSFixtureConfig(t *testing.T) (aws.Config, *awsFixtureStub) { }, stub } +// requireRequests asserts the exact "host operation" request counts the stub +// received, so a test cannot pass without its AWS calls reaching the stub. +func (s *awsFixtureStub) requireRequests(t *testing.T, want map[string]int) { + t.Helper() + s.mu.Lock() + defer s.mu.Unlock() + assert.Equal(t, want, s.received) +} + // requireDuplicateChecks asserts the duplicate check really ran against the // stub: one call per (service, region) pair, no other request, and every // response parsed. @@ -90,7 +135,9 @@ func (s *awsFixtureStub) requireDuplicateChecks(t *testing.T, pairs int) { defer s.mu.Unlock() total := 0 for k, n := range s.received { - assert.Contains(t, awsFixtures, k) + host, op, _ := strings.Cut(k, " ") + svc, _, _ := strings.Cut(host, ".") + assert.Contains(t, s.fixtures, svc+" "+op) assert.Equalf(t, 1, n, "duplicate-check requests for %s", k) total += n } diff --git a/cmd/cancellation_prepurchase_test.go b/cmd/cancellation_prepurchase_test.go index e80de9519..683010a6a 100644 --- a/cmd/cancellation_prepurchase_test.go +++ b/cmd/cancellation_prepurchase_test.go @@ -112,9 +112,15 @@ func TestFetchAllRecs_DefaultRunKeepsRecommendationsInRegionsOutsideTheCallersOp client.On("GetRecommendations", mock.Anything, mock.Anything). Return([]common.Recommendation{rec("us-east-1"), rec("eu-central-2")}, nil) - recs, _, err := fetchAllRecs(context.Background(), aws.Config{}, client, NewAccountAliasCache(aws.Config{}), []common.ServiceType{common.ServiceRDS}, engineVersionData{}, cfg, nil) + awsCfg, awsStub := newAWSFixtureConfig(t) + recs, _, err := fetchAllRecs(context.Background(), awsCfg, client, NewAccountAliasCache(awsCfg), []common.ServiceType{common.ServiceRDS}, engineVersionData{}, cfg, nil) require.NoError(t, err) + // One duplicate check per region: us-east-1 and eu-central-2. + awsStub.requireRequests(t, map[string]int{ + "rds.us-east-1.amazonaws.com DescribeReservedDBInstances": 1, + "rds.eu-central-2.amazonaws.com DescribeReservedDBInstances": 1, + }) regions := make([]string, 0, len(recs)) for i := range recs { regions = append(regions, recs[i].Region) @@ -235,6 +241,7 @@ func TestFetchAllRecs_AccountFilters(t *testing.T) { } base := Config{Regions: []string{"us-east-1"}, Coverage: 100, TermYears: 1, PaymentOption: "partial-upfront"} // Member accounts cannot call DescribeAccount: every lookup is denied. + dupChecks := 0 run := func(t *testing.T, cfg Config, accounts ...string) (accountsOut []string, orgCalls int, err error) { t.Helper() found := make([]common.Recommendation, 0, len(accounts)) @@ -245,7 +252,11 @@ func TestFetchAllRecs_AccountFilters(t *testing.T) { client.On("GetRecommendations", mock.Anything, mock.Anything).Return(found, nil) org := &MockOrganizationsClient{} org.On("DescribeAccount", mock.Anything, mock.Anything).Return(nil, errors.New("AccessDenied")) - recs, _, err := fetchAllRecs(context.Background(), aws.Config{}, client, NewAccountAliasCacheWithClient(org), []common.ServiceType{common.ServiceRDS}, engineVersionData{}, cfg, nil) + awsCfg, awsStub := newAWSFixtureConfig(t) + recs, _, err := fetchAllRecs(context.Background(), awsCfg, client, NewAccountAliasCacheWithClient(org), []common.ServiceType{common.ServiceRDS}, engineVersionData{}, cfg, nil) + for _, n := range awsStub.received { + dupChecks += n + } for i := range recs { accountsOut = append(accountsOut, recs[i].Account) } @@ -296,6 +307,7 @@ func TestFetchAllRecs_AccountFilters(t *testing.T) { require.NoError(t, err) assert.Equal(t, 1, calls) }) + assert.Equal(t, 4, dupChecks, "duplicate checks served by the fixture stub") } func TestRequireResolvedAccountNames_CSVRows(t *testing.T) { diff --git a/cmd/multi_service_coverage_test.go b/cmd/multi_service_coverage_test.go index bdd7b4a68..1cb30e5e2 100644 --- a/cmd/multi_service_coverage_test.go +++ b/cmd/multi_service_coverage_test.go @@ -860,7 +860,7 @@ func TestFetchExistingCoverage_LookbackDays(t *testing.T) { // ce:GetReservationCoverage permission would. func TestFetchExistingCoverage_FetchFailure(t *testing.T) { ctx := context.Background() - awsCfg := aws.Config{Region: "us-east-1"} + awsCfg, awsStub := newAWSFixtureConfig(t) realClient := awsprovider.NewRecommendationsClientDirect(awsCfg) t.Run("purchase run aborts: returns the error and a nil map", func(t *testing.T) { @@ -876,4 +876,7 @@ func TestFetchExistingCoverage_FetchFailure(t *testing.T) { require.NoError(t, err, "a dry run must not abort; nothing is bought") assert.Nil(t, got) }) + + // Each case asked CE once; the fixture answers with a non-retryable 400. + awsStub.requireRequests(t, map[string]int{"ce.us-east-1.amazonaws.com GetReservationCoverage": 2}) } diff --git a/cmd/multi_service_helpers_test.go b/cmd/multi_service_helpers_test.go index dfee0d931..8f0a740d8 100644 --- a/cmd/multi_service_helpers_test.go +++ b/cmd/multi_service_helpers_test.go @@ -3,7 +3,6 @@ package main import ( "context" "errors" - "os" "testing" "time" @@ -89,24 +88,13 @@ func TestGetAllAWSRegions(t *testing.T) { }) } - t.Run("Integration test", func(t *testing.T) { - // This test requires actual AWS credentials - if os.Getenv("AWS_ACCESS_KEY_ID") == "" { - t.Skip("Skipping integration test: AWS credentials not present") - } - - cfg := aws.Config{Region: "us-east-1"} + t.Run("SDK client path against the fixture endpoint", func(t *testing.T) { + cfg, awsStub := newAWSFixtureConfig(t) regions, err := getAllAWSRegions(ctx, cfg) - if err == nil { - assert.NotNil(t, regions) - assert.Greater(t, len(regions), 0) - - // Verify regions are sorted - for i := 1; i < len(regions); i++ { - assert.LessOrEqual(t, regions[i-1], regions[i]) - } - } + require.NoError(t, err) + assert.Equal(t, []string{"eu-west-1", "us-west-2"}, regions, "regions are sorted") + awsStub.requireRequests(t, map[string]int{"ec2.us-east-1.amazonaws.com DescribeRegions": 1}) }) } diff --git a/cmd/multi_service_test.go b/cmd/multi_service_test.go index 9568c0eb9..8498296ea 100644 --- a/cmd/multi_service_test.go +++ b/cmd/multi_service_test.go @@ -212,7 +212,14 @@ func TestProcessService_EdgeCases(t *testing.T) { func TestProcessServiceWithMocks(t *testing.T) { useCallerAccount(t) ctx := context.Background() - awsCfg := aws.Config{Region: "us-east-1"} + awsCfg, awsStub := newAWSFixtureConfig(t) + // The real-purchase case below relies on a failed duplicate check refusing + // the purchase (fail closed); an empty fixture would instead let it reach + // the purchase call. + awsStub.fixtures["elasticache DescribeReservedCacheNodes"] = awsFixture{ + status: http.StatusForbidden, + body: `{"__type":"AccessDeniedException","message":"denied by test"}`, + } // Save original values origCfg := toolCfg @@ -315,15 +322,23 @@ func TestProcessServiceWithMocks(t *testing.T) { assert.Empty(t, recs) assert.Empty(t, results) } + if !tt.isDryRun { + assert.Empty(t, results, "a failed duplicate check must refuse a real purchase") + } mockClient.AssertExpectations(t) }) } + + awsStub.requireRequests(t, map[string]int{ + "rds.us-east-1.amazonaws.com DescribeReservedDBInstances": 1, + "elasticache.eu-west-1.amazonaws.com DescribeReservedCacheNodes": 1, + }) } func TestProcessService_SavingsPlansAccountLevel(t *testing.T) { ctx := context.Background() - awsCfg := aws.Config{Region: "us-east-1"} + awsCfg, awsStub := newAWSFixtureConfig(t) origCfg := toolCfg defer func() { toolCfg = origCfg }() @@ -353,6 +368,7 @@ func TestProcessService_SavingsPlansAccountLevel(t *testing.T) { // Verify Savings Plans queried only once mockClient.AssertNumberOfCalls(t, "GetRecommendations", 1) mockClient.AssertExpectations(t) + awsStub.requireRequests(t, map[string]int{"savingsplans.amazonaws.com DescribeSavingsPlans": 1}) } func TestProcessService_WithInstanceLimit(t *testing.T) { @@ -367,7 +383,7 @@ func TestProcessService_WithInstanceLimit(t *testing.T) { // TestProcessService_InstanceLimitRefusesRealPurchase covers that. ctx := context.Background() - awsCfg := aws.Config{Region: "us-east-1"} + awsCfg, awsStub := newAWSFixtureConfig(t) origCfg := toolCfg defer func() { toolCfg = origCfg }() @@ -406,6 +422,7 @@ func TestProcessService_WithInstanceLimit(t *testing.T) { "the per-region path must not apply --max-instances; the cap is run-wide") mockClient.AssertExpectations(t) + awsStub.requireDuplicateChecks(t, 1) } // TestProcessService_InstanceLimitRefusesRealPurchase pins the fail-closed @@ -457,7 +474,7 @@ func TestProcessService_InstanceLimitRefusesRealPurchase(t *testing.T) { func TestProcessService_WithOverrideCount(t *testing.T) { ctx := context.Background() - awsCfg := aws.Config{Region: "us-east-1"} + awsCfg, awsStub := newAWSFixtureConfig(t) origCfg := toolCfg defer func() { toolCfg = origCfg }() @@ -485,11 +502,12 @@ func TestProcessService_WithOverrideCount(t *testing.T) { } mockClient.AssertExpectations(t) + awsStub.requireDuplicateChecks(t, 1) } func TestProcessService_MultipleRegions(t *testing.T) { ctx := context.Background() - awsCfg := aws.Config{Region: "us-east-1"} + awsCfg, awsStub := newAWSFixtureConfig(t) origCfg := toolCfg defer func() { toolCfg = origCfg }() @@ -521,6 +539,7 @@ func TestProcessService_MultipleRegions(t *testing.T) { // Verify each region was queried mockClient.AssertNumberOfCalls(t, "GetRecommendations", 3) mockClient.AssertExpectations(t) + awsStub.requireDuplicateChecks(t, 3) } // ==================== Helper Function Tests ====================