Skip to content

Commit 63fdd11

Browse files
committed
test(recommendations): address override review comments
Harden the console.warn spy cleanup in the frontend override refresh test, add the missing override-lookup error pass-through regression, and align the dashboard coverage test with explicit zero-coverage semantics.
1 parent 4f99797 commit 63fdd11

4 files changed

Lines changed: 40 additions & 21 deletions

File tree

frontend/src/__tests__/settings-accounts.test.ts

Lines changed: 17 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -616,21 +616,23 @@ describe('Overrides panel — AWS payment selector', () => {
616616
mockLoadRecommendations.mockRejectedValueOnce(new Error('network blip'));
617617
const consoleWarnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {});
618618

619-
const panel = await openOverridesPanel('acc-1');
620-
const select = panel.querySelector('select.override-payment-select') as HTMLSelectElement;
621-
select.value = 'all-upfront';
622-
select.dispatchEvent(new Event('change'));
623-
await new Promise(r => setTimeout(r, 0));
624-
625-
expect(api.saveAccountServiceOverride).toHaveBeenCalledTimes(1);
626-
expect(consoleWarnSpy).toHaveBeenCalled();
627-
// No error toast should have been shown for the refresh failure: the
628-
// success toast from the save path is what the user sees.
629-
const toastCalls = mockShowToast.mock.calls.map(c => c[0]);
630-
const errorToasts = toastCalls.filter(t => (t as { kind?: string }).kind === 'error');
631-
expect(errorToasts).toHaveLength(0);
632-
633-
consoleWarnSpy.mockRestore();
619+
try {
620+
const panel = await openOverridesPanel('acc-1');
621+
const select = panel.querySelector('select.override-payment-select') as HTMLSelectElement;
622+
select.value = 'all-upfront';
623+
select.dispatchEvent(new Event('change'));
624+
await new Promise(r => setTimeout(r, 0));
625+
626+
expect(api.saveAccountServiceOverride).toHaveBeenCalledTimes(1);
627+
expect(consoleWarnSpy).toHaveBeenCalled();
628+
// No error toast should have been shown for the refresh failure: the
629+
// success toast from the save path is what the user sees.
630+
const toastCalls = mockShowToast.mock.calls.map(c => c[0]);
631+
const errorToasts = toastCalls.filter(t => (t as { kind?: string }).kind === 'error');
632+
expect(errorToasts).toHaveLength(0);
633+
} finally {
634+
consoleWarnSpy.mockRestore();
635+
}
634636
});
635637

636638
test('Inherit is disabled when override already has a payment set (no clear-field channel)', async () => {

internal/api/handler_dashboard.go

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -146,9 +146,9 @@ func summarizeRecommendationsWithCoverage(
146146
return total, byService
147147
}
148148

149-
// scaledSavings returns rec.Savings * min(coverage, 100) / 100 when a
150-
// non-zero coverage entry exists for the rec's (account, provider, service)
151-
// triple. Otherwise returns rec.Savings unchanged.
149+
// scaledSavings returns rec.Savings * min(max(coverage, 0), 100) / 100 when
150+
// a coverage entry exists for the rec's (account, provider, service) triple.
151+
// Otherwise returns rec.Savings unchanged.
152152
func scaledSavings(rec config.RecommendationRecord, coverageByKey map[string]float64) float64 {
153153
if rec.CloudAccountID == nil || coverageByKey == nil {
154154
return rec.Savings
@@ -165,7 +165,6 @@ func scaledSavings(rec config.RecommendationRecord, coverageByKey map[string]flo
165165
}
166166
return rec.Savings * coverage / 100
167167
}
168-
}
169168

170169
// resolveCoverageByAccountKey returns a map of AccountConfigKey -> resolved
171170
// coverage% for every (account, provider, service) triple represented in

internal/api/handler_dashboard_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -172,10 +172,10 @@ func TestSummarizeRecommendationsWithCoverage(t *testing.T) {
172172
wantTotal: 50 + 200,
173173
},
174174
{
175-
name: "zero coverage treated as 'no scaling configured'",
175+
name: "zero coverage scales savings to zero",
176176
recs: []config.RecommendationRecord{rec(acctA, 100)},
177177
coverage: map[string]float64{keyA: 0},
178-
wantTotal: 100,
178+
wantTotal: 0,
179179
},
180180
{
181181
name: "coverage at 100 applies no scaling",

internal/scheduler/scheduler_overrides_test.go

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -260,6 +260,24 @@ func TestApplyAccountOverrides_LookupError_PassesThrough(t *testing.T) {
260260
assert.Len(t, recs, 1, "un-filtered list returned on lookup failure")
261261
}
262262

263+
func TestApplyAccountOverrides_OverrideLookupError_PassesThrough(t *testing.T) {
264+
// Mirrors the global-config lookup failure contract: if the per-account
265+
// override lookup fails, the page should over-show rather than blank.
266+
ctx := context.Background()
267+
store := &mockOverrideStore{
268+
recs: []config.RecommendationRecord{rdsRec("acct-A", "us-east-1", "mysql")},
269+
globals: map[string]*config.ServiceConfig{
270+
"aws|rds": {Provider: "aws", Service: "rds", Enabled: true},
271+
},
272+
getOverrideErr: errors.New("override lookup timeout"),
273+
}
274+
s := &Scheduler{config: store}
275+
276+
recs, err := s.ListRecommendations(ctx, config.RecommendationFilter{})
277+
require.NoError(t, err, "ListRecommendations swallows the override-resolver error")
278+
assert.Len(t, recs, 1, "un-filtered list returned on override lookup failure")
279+
}
280+
263281
// Issue #196 acceptance criterion (mirrored from issue #111):
264282
//
265283
// Seed a global ServiceConfig + a per-account override that disables one

0 commit comments

Comments
 (0)