From 55249c34fbb55eddbfb85b85b04d9f10eb23d83a Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 16 Oct 2026 00:12:09 +0100 Subject: [PATCH] fix(cli): classify errored provider purchase results as failures (#1900) A provider can return PurchaseResult{Success: true, Error: non-nil}. purchaseAuditStatus, purchaseSingleRec and logPurchaseOutcome keyed only on result.Success, so such a result was audited as "success" and printed with a success marker. Add purchaseSucceeded (Success && Error == nil) and use it for the audit status and the diagnostics on both the recommendation and CSV purchase paths. The returned PurchaseResult is left unchanged. Add a newServiceClient seam so the regression test drives the real purchaseSingleRec path with a fake provider. Closes #1900 Co-Authored-By: Claude Opus 5.5 --- cmd/multi_service.go | 19 ++++- cmd/purchase_result_classification_test.go | 88 ++++++++++++++++++++++ 2 files changed, 103 insertions(+), 4 deletions(-) create mode 100644 cmd/purchase_result_classification_test.go diff --git a/cmd/multi_service.go b/cmd/multi_service.go index d43a39bd1..3b6370104 100644 --- a/cmd/multi_service.go +++ b/cmd/multi_service.go @@ -596,11 +596,22 @@ func probeAuditLog(isDryRun bool, auditLogPath string) error { return nil } +// newServiceClient is the service-client factory used by purchaseSingleRec. +// It is a seam so tests can drive the real purchase path with a fake provider. +var newServiceClient = createServiceClient + +// purchaseSucceeded reports whether a purchase result is a success: the +// provider set Success AND reported no error. A provider may return +// Success=true with a non-nil Error; that is a failure (issue #1900). +func purchaseSucceeded(result common.PurchaseResult) bool { + return result.Success && result.Error == nil +} + // purchaseAuditStatus derives the audit status for a completed (non-dry-run) // purchase attempt. Callers handle the dry-run ("skipped") case separately, // since that never reaches a PurchaseResult from an actual API call. func purchaseAuditStatus(result common.PurchaseResult) string { - if result.Success { + if purchaseSucceeded(result) { return "success" } return "error" @@ -627,7 +638,7 @@ func purchaseSingleRec(ctx context.Context, awsCfg aws.Config, rec common.Recomm } regionalCfg := awsCfg.Copy() regionalCfg.Region = clientRegion - serviceClient := createServiceClient(rec.Service, regionalCfg) + serviceClient := newServiceClient(rec.Service, regionalCfg) if serviceClient == nil { AppLogger.Printf(" ⚠️ No service client for %s\n", rec.Service) return common.PurchaseResult{Success: false}, "error" @@ -635,7 +646,7 @@ func purchaseSingleRec(ctx context.Context, awsCfg aws.Config, rec common.Recomm result := executePurchase(ctx, rec, label, index, serviceClient, cfg) status := purchaseAuditStatus(result) - if result.Success { + if purchaseSucceeded(result) { AppLogger.Printf(" ✅ %s\n", result.CommitmentID) } else { AppLogger.Printf(" ❌ %v\n", result.Error) @@ -1048,7 +1059,7 @@ func runOnePurchase(ctx context.Context, rec common.Recommendation, label, regio } func logPurchaseOutcome(result common.PurchaseResult) { - if result.Success { + if purchaseSucceeded(result) { AppLogger.Printf(" ✅ Success: %s\n", result.CommitmentID) return } diff --git a/cmd/purchase_result_classification_test.go b/cmd/purchase_result_classification_test.go new file mode 100644 index 000000000..77f1eeb0a --- /dev/null +++ b/cmd/purchase_result_classification_test.go @@ -0,0 +1,88 @@ +package main + +import ( + "context" + "errors" + "testing" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/pkg/provider" + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" +) + +// purchaseClassificationCases covers how a provider's PurchaseCommitment +// return is classified (issue #1900): a result is a success only when +// Success is true AND Error is nil. +func purchaseClassificationCases() []struct { + name string + result common.PurchaseResult + err error + wantStatus string + wantMarker string +} { + providerErr := errors.New("provider reported partial failure") + return []struct { + name string + result common.PurchaseResult + err error + wantStatus string + wantMarker string + }{ + {"success", common.PurchaseResult{Success: true, CommitmentID: "ri-ok"}, nil, "success", "✅"}, + {"success flag with provider error", common.PurchaseResult{Success: true, CommitmentID: "ri-err", Error: providerErr}, nil, "error", "❌"}, + {"success false", common.PurchaseResult{Success: false, Error: providerErr}, nil, "error", "❌"}, + {"separate go error", common.PurchaseResult{Success: true, CommitmentID: "ri-goerr"}, errors.New("api call failed"), "error", "❌"}, + } +} + +func TestPurchaseSingleRec_ClassifiesErroredProviderResultAsFailure(t *testing.T) { + t.Setenv("DISABLE_PURCHASE_DELAY", "true") + for _, tc := range purchaseClassificationCases() { + t.Run(tc.name, func(t *testing.T) { + client := &MockServiceClient{} + client.On("PurchaseCommitment", mock.Anything, mock.Anything, mock.Anything).Return(tc.result, tc.err) + oldFactory := newServiceClient + newServiceClient = func(common.ServiceType, aws.Config) provider.ServiceClient { return client } + t.Cleanup(func() { newServiceClient = oldFactory }) + + cfg := Config{CallerAccountID: defaultTestAccount} + var status string + out := captureAppOutput(t, func() { + _, status = purchaseSingleRec(context.Background(), aws.Config{}, validEC2Rec(), 1, false, cfg) + }) + + client.AssertNumberOfCalls(t, "PurchaseCommitment", 1) + assert.Equal(t, tc.wantStatus, status) + assert.Contains(t, out, tc.wantMarker) + if tc.wantStatus == "error" { + assert.NotContains(t, out, "✅") + } + }) + } +} + +func TestRunOnePurchase_ClassifiesErroredProviderResultAsFailure(t *testing.T) { + for _, tc := range purchaseClassificationCases() { + t.Run(tc.name, func(t *testing.T) { + client := &MockServiceClient{} + client.On("PurchaseCommitment", mock.Anything, mock.Anything, mock.Anything).Return(tc.result, tc.err) + rec := validEC2Rec() + cfg := Config{CallerAccountID: defaultTestAccount} + + var status string + out := captureAppOutput(t, func() { + var result common.PurchaseResult + result, status, _ = runOnePurchase(context.Background(), rec, rec.Region, rec.Region, 1, false, client, cfg) + logPurchaseOutcome(result) + }) + + assert.Equal(t, tc.wantStatus, status) + assert.Contains(t, out, tc.wantMarker) + if tc.wantStatus == "error" { + assert.NotContains(t, out, "✅") + } + }) + } +}