From ca55a3c59abdcf6b6d0aaf1c4eafc0d088af1c03 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Sat, 17 Oct 2026 01:24:29 +0100 Subject: [PATCH] fix(cli): keep CSV TOTAL normalized units in line with the per-row NU cells buildTotalRow multiplied every row's Count by RDSInstanceNUFromType, while formatNormalizedUnitsOrBlank only renders NU for RDS rows. The lookup accepts any type with three dot-separated parts, so ElastiCache types like cache.r7g.large resolved a size NU and inflated the TOTAL above the sum of the visible column. Extract rowNormalizedUnits as the single per-row NU source (0 for non-RDS services) and use it for both the cell and the TOTAL. Closes #2005 Co-Authored-By: Claude Opus 5.5 --- cmd/multi_service_csv.go | 22 ++++++++---- cmd/multi_service_csv_test.go | 64 +++++++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 6 deletions(-) diff --git a/cmd/multi_service_csv.go b/cmd/multi_service_csv.go index 7576733b6..0348a6d41 100644 --- a/cmd/multi_service_csv.go +++ b/cmd/multi_service_csv.go @@ -406,7 +406,7 @@ func buildTotalRow(results []common.PurchaseResult) []string { for i := range results { r := results[i] totalCount += r.Recommendation.Count - totalNU += float64(r.Recommendation.Count) * recommendations.RDSInstanceNUFromType(r.Recommendation.ResourceType) + totalNU += rowNormalizedUnits(r.Recommendation) totalUpfront += r.Recommendation.CommitmentCost totalSavings += r.Recommendation.EstimatedSavings if r.Recommendation.RecurringMonthlyCost != nil { @@ -466,14 +466,24 @@ func extractFamily(rec common.Recommendation) string { // to compute NU by hand to verify the bundling. Renders blank for // non-RDS rows and for sizes not in the standard NU scale. func formatNormalizedUnitsOrBlank(rec common.Recommendation) string { - if rec.Service != common.ServiceRDS && rec.Service != common.ServiceRelationalDB { + nu := rowNormalizedUnits(rec) + if nu == 0 { return "" } - nu := recommendations.RDSInstanceNUFromType(rec.ResourceType) - if nu == 0 || rec.Count == 0 { - return "" + return fmt.Sprintf("%g", nu) +} + +// rowNormalizedUnits returns the NU a row contributes (rec.Count × NU(size)) +// for RDS rows, and 0 for every other service. It is the single source of +// the per-row NU value, so the TOTAL row sums exactly what the NU cells +// above it show: ElastiCache types like "cache.r7g.large" would otherwise +// resolve a size suffix in the RDS NU lookup even though their own cell is +// blank (issue #2005). +func rowNormalizedUnits(rec common.Recommendation) float64 { + if rec.Service != common.ServiceRDS && rec.Service != common.ServiceRelationalDB { + return 0 } - return fmt.Sprintf("%g", float64(rec.Count)*nu) + return float64(rec.Count) * recommendations.RDSInstanceNUFromType(rec.ResourceType) } // extractDeployment returns the RDS deployment-option string diff --git a/cmd/multi_service_csv_test.go b/cmd/multi_service_csv_test.go index a5ed36af7..d913f33a5 100644 --- a/cmd/multi_service_csv_test.go +++ b/cmd/multi_service_csv_test.go @@ -1,10 +1,13 @@ package main import ( + "bytes" "encoding/csv" "errors" "os" "path/filepath" + "slices" + "strconv" "strings" "testing" "time" @@ -389,6 +392,67 @@ func TestWriteMultiServiceCSVReport_SortAndTotal(t *testing.T) { assert.Equal(t, "2080.00", totalRow[idxSavings]) } +// TestBuildTotalRow_NUSkipsRowsWithBlankNUCell is the regression test for +// issue #2005: the TOTAL NormalizedUnits cell must equal the sum of the +// per-row NU cells above it. ElastiCache and OpenSearch types such as +// "cache.r7g.large" have three dot-separated parts, so the NU lookup +// resolves their size suffix even though their own NU cell renders blank; +// before the fix those rows inflated the TOTAL. +func TestBuildTotalRow_NUSkipsRowsWithBlankNUCell(t *testing.T) { + results := []common.PurchaseResult{ + // 5 × db.r6g.large = 20 NU (rendered on the row). + {Recommendation: common.Recommendation{Service: common.ServiceRDS, ResourceType: "db.r6g.large", Count: 5}}, + // 2 × db.r6g.xlarge = 16 NU via the relational-db alias (rendered). + {Recommendation: common.Recommendation{Service: common.ServiceRelationalDB, ResourceType: "db.r6g.xlarge", Count: 2}}, + // Non-RDS rows whose NU cell is blank: must add nothing. + {Recommendation: common.Recommendation{Service: common.ServiceElastiCache, ResourceType: "cache.r7g.large", Count: 3}}, + {Recommendation: common.Recommendation{Service: common.ServiceOpenSearch, ResourceType: "r6g.large.search", Count: 4}}, + {Recommendation: common.Recommendation{Service: common.ServiceEC2, ResourceType: "m5.large", Count: 7}}, + } + + var buf bytes.Buffer + require.NoError(t, writeMultiServiceCSV(&buf, results)) + rows, err := csv.NewReader(&buf).ReadAll() + require.NoError(t, err) + require.Len(t, rows, len(results)+2) // header + data rows + TOTAL + + idxNU := slices.Index(rows[0], "NormalizedUnits") + require.GreaterOrEqual(t, idxNU, 0) + + var columnSum float64 + for _, row := range rows[1 : len(rows)-1] { + if row[idxNU] == "" { + continue + } + v, err := strconv.ParseFloat(row[idxNU], 64) + require.NoError(t, err) + columnSum += v + } + require.InDelta(t, 36, columnSum, 1e-9, "only the two RDS rows render NU") + + totalRow := rows[len(rows)-1] + require.Equal(t, "TOTAL", totalRow[0]) + assert.Equal(t, "36", totalRow[idxNU], "TOTAL NU must equal the sum of the visible NU cells") +} + +// TestBuildTotalRow_NUBlankWhenNoRowHasNU locks that a run with no RDS rows +// leaves the TOTAL NU cell blank instead of summing NU for blank-cell rows. +func TestBuildTotalRow_NUBlankWhenNoRowHasNU(t *testing.T) { + results := []common.PurchaseResult{ + {Recommendation: common.Recommendation{Service: common.ServiceElastiCache, ResourceType: "cache.r7g.large", Count: 3}}, + } + var buf bytes.Buffer + require.NoError(t, writeMultiServiceCSV(&buf, results)) + rows, err := csv.NewReader(&buf).ReadAll() + require.NoError(t, err) + require.Len(t, rows, 3) // header + 1 data row + TOTAL + + totalRow := rows[2] + require.Equal(t, "TOTAL", totalRow[0]) + assert.Equal(t, "3", totalRow[slices.Index(rows[0], "Count")], "Count still sums") + assert.Empty(t, totalRow[slices.Index(rows[0], "NormalizedUnits")], "NormalizedUnits stays blank when no row renders NU") +} + // TestFormatExistingCoverage locks the three-state rendering: // - ExistingCoverageKnown=false → "n/a" (CE has no data for this pool) // - ExistingCoverageKnown=true, Pct=0 → "0.0" (CE confirms zero coverage)