From d5775a58c7fa61a25ac5bf5d220c96eb27bfbc31 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 28 May 2026 21:50:50 +0200 Subject: [PATCH 1/2] feat(common): cross-provider capacity comparator for compute recs (#82) Add frontend/src/lib/capacity.ts exporting compareByCapacity, a sort comparator that ranks compute recommendations by vCPU (primary), MemoryGB (secondary), and resource_type lexical (tie-break). Absent, null, or zero capacity values sort to the end per the ComputeDetails "0 = unknown" contract and feedback_nullable_not_zero guidance. Adds 16 tests covering basic ordering, null/zero sentinel handling, mixed AWS/Azure/GCP scenarios, reflexivity, and sort symmetry. pkg/common/types.go VCPU+MemoryGB fields were already present (PR #97); this commit wires the cross-provider ranking utility those fields enable. Refs #82 --- frontend/src/__tests__/capacity.test.ts | 170 ++++++++++++++++++++++++ frontend/src/lib/capacity.ts | 103 ++++++++++++++ 2 files changed, 273 insertions(+) create mode 100644 frontend/src/__tests__/capacity.test.ts create mode 100644 frontend/src/lib/capacity.ts diff --git a/frontend/src/__tests__/capacity.test.ts b/frontend/src/__tests__/capacity.test.ts new file mode 100644 index 000000000..f22544590 --- /dev/null +++ b/frontend/src/__tests__/capacity.test.ts @@ -0,0 +1,170 @@ +/** + * Tests for the cross-provider compute capacity comparator (issue #82). + * + * Covers: + * - compareByCapacity: vCPU primary key, MemoryGB secondary key, + * resource_type tertiary key + * - Null / absent / zero capacity values sort to the end (feedback_nullable_not_zero) + * - Mixed AWS / Azure / GCP recs produce a deterministic order + * - Non-compute details (no vcpu/memory_gb) are treated as unknown + * - Reflexivity: compareByCapacity(x, x) === 0 + * - Symmetry: sign(compareByCapacity(a, b)) === -sign(compareByCapacity(b, a)) + */ + +import { compareByCapacity } from '../lib/capacity'; + +// --------------------------------------------------------------------------- +// Helpers +// --------------------------------------------------------------------------- + +function rec( + resourceType: string, + vcpu: number | null | undefined, + memoryGb: number | null | undefined +): { details: unknown; resource_type: string } { + const details: Record = {}; + if (vcpu !== undefined && vcpu !== null) details['vcpu'] = vcpu; + if (memoryGb !== undefined && memoryGb !== null) details['memory_gb'] = memoryGb; + return { details, resource_type: resourceType }; +} + +function recNoDetails(resourceType: string): { details: undefined; resource_type: string } { + return { details: undefined, resource_type: resourceType }; +} + +// --------------------------------------------------------------------------- +// Basic ordering +// --------------------------------------------------------------------------- + +describe('compareByCapacity — primary sort: vCPU', () => { + it('sorts smaller vCPU before larger', () => { + const small = rec('m5.large', 2, 8); + const large = rec('m5.4xlarge', 16, 64); + expect(compareByCapacity(small, large)).toBeLessThan(0); + expect(compareByCapacity(large, small)).toBeGreaterThan(0); + }); + + it('is reflexive', () => { + const r = rec('m5.2xlarge', 8, 32); + expect(compareByCapacity(r, r)).toBe(0); + }); +}); + +describe('compareByCapacity — secondary sort: MemoryGB', () => { + it('breaks vCPU tie by memory ascending', () => { + const lowMem = rec('c5.xlarge', 4, 8); + const hiMem = rec('r5.xlarge', 4, 32); + expect(compareByCapacity(lowMem, hiMem)).toBeLessThan(0); + expect(compareByCapacity(hiMem, lowMem)).toBeGreaterThan(0); + }); +}); + +describe('compareByCapacity — tertiary sort: resource_type lexical', () => { + it('breaks full capacity tie by resource_type', () => { + const az = rec('Standard_D4s_v3', 4, 16); + const aws = rec('m5.xlarge', 4, 16); + // "Standard_D4s_v3" > "m5.xlarge" lexically (uppercase 'S' < lowercase 'm' in ASCII + // but both are printable — what matters is determinism and symmetry) + const cmp = compareByCapacity(az, aws); + expect(cmp).not.toBe(0); + expect(compareByCapacity(aws, az)).toBe(-cmp); + }); + + it('returns 0 for identical capacity and type', () => { + const a = rec('m5.xlarge', 4, 16); + const b = rec('m5.xlarge', 4, 16); + expect(compareByCapacity(a, b)).toBe(0); + }); +}); + +// --------------------------------------------------------------------------- +// Null / absent / zero values sort to the end (feedback_nullable_not_zero) +// --------------------------------------------------------------------------- + +describe('compareByCapacity — null/absent/zero sort last', () => { + it('null vcpu sorts after known vcpu', () => { + const known = rec('m5.large', 2, 8); + const unknown = rec('unknown-type', null, null); + expect(compareByCapacity(known, unknown)).toBeLessThan(0); + expect(compareByCapacity(unknown, known)).toBeGreaterThan(0); + }); + + it('zero vcpu (unknown sentinel) sorts after positive vcpu', () => { + const known = rec('m5.large', 2, 8); + const zeroVcpu = rec('z1d.large', 0, 8); + expect(compareByCapacity(known, zeroVcpu)).toBeLessThan(0); + }); + + it('absent details sorts after fully-populated rec', () => { + const known = rec('m5.xlarge', 4, 16); + const noDetails = recNoDetails('mystery-type'); + expect(compareByCapacity(known, noDetails)).toBeLessThan(0); + expect(compareByCapacity(noDetails, known)).toBeGreaterThan(0); + }); + + it('two unknown recs: tie resolved by resource_type', () => { + const a = recNoDetails('alpha'); + const b = recNoDetails('beta'); + expect(compareByCapacity(a, b)).toBeLessThan(0); + expect(compareByCapacity(b, a)).toBeGreaterThan(0); + }); + + it('null memoryGB sorts after known memoryGB when vCPU ties', () => { + const known = rec('c5.xlarge', 4, 8); + const nullMem = rec('something', 4, null); + expect(compareByCapacity(known, nullMem)).toBeLessThan(0); + }); +}); + +// --------------------------------------------------------------------------- +// Mixed AWS / Azure / GCP scenario +// --------------------------------------------------------------------------- + +describe('compareByCapacity — cross-provider sort', () => { + it('produces deterministic order across providers', () => { + const awsSmall = rec('m5.large', 2, 8); // AWS + const azureMid = rec('Standard_D4s_v3', 4, 16); // Azure + const gcpLarge = rec('n2-standard-8', 8, 32); // GCP + const unknown = recNoDetails('unknown'); // no catalogue entry + + const recs = [gcpLarge, unknown, awsSmall, azureMid]; + const sorted = [...recs].sort(compareByCapacity); + + expect(sorted[0]).toBe(awsSmall); + expect(sorted[1]).toBe(azureMid); + expect(sorted[2]).toBe(gcpLarge); + expect(sorted[3]).toBe(unknown); + }); + + it('Azure fractional memory (0.5 GB) sorts before 1 GB', () => { + const tiny = rec('Standard_A0', 1, 0.5); + const small = rec('Standard_A1', 1, 1.75); + expect(compareByCapacity(tiny, small)).toBeLessThan(0); + }); +}); + +// --------------------------------------------------------------------------- +// Symmetry invariant +// --------------------------------------------------------------------------- + +describe('compareByCapacity — symmetry', () => { + const pairs: Array<[ + { details: unknown; resource_type: string }, + { details: unknown; resource_type: string } + ]> = [ + [rec('m5.large', 2, 8), rec('m5.4xlarge', 16, 64)], + [rec('c5.xlarge', 4, 8), rec('r5.xlarge', 4, 32)], + [rec('m5.xlarge', 4, 16), recNoDetails('no-data')], + [recNoDetails('alpha'), recNoDetails('beta')], + ]; + + it.each(pairs)('sign(a,b) === -sign(b,a)', (a, b) => { + const ab = compareByCapacity(a, b); + const ba = compareByCapacity(b, a); + if (ab === 0) { + expect(ba).toBe(0); + } else { + expect(Math.sign(ab)).toBe(-Math.sign(ba)); + } + }); +}); diff --git a/frontend/src/lib/capacity.ts b/frontend/src/lib/capacity.ts new file mode 100644 index 000000000..06c646b28 --- /dev/null +++ b/frontend/src/lib/capacity.ts @@ -0,0 +1,103 @@ +/** + * Cross-provider compute capacity ranking utilities. + * + * Provides a comparator for sorting cloud commitment recommendations by their + * underlying compute capacity (vCPU first, then memory in GB, then instance + * type lexically). This lets a mixed AWS/Azure/GCP recommendation table be + * sorted by actual size rather than by opaque type-name strings whose + * lexicographic order carries no capacity meaning. + * + * The fields below mirror the JSON keys emitted by ComputeDetails in + * pkg/common/types.go (vcpu / memory_gb) — the shapes must stay in sync. + * + * Missing capacity values (0 or null/undefined from the API) sort to the + * end of the list rather than the front, so well-populated rows stay visible + * at the top when the user sorts ascending by size. + */ + +/** + * Minimum capability representation carried by a recommendation's details + * payload when the service is "compute". Matches the JSON produced by + * ComputeDetails (pkg/common/types.go): vcpu and memory_gb are omitempty so + * they may be absent from the wire payload. + */ +export interface ComputeCapacity { + vcpu?: number | null; + memory_gb?: number | null; + [key: string]: unknown; +} + +/** + * compareByCapacity ranks two compute recommendations by capacity. + * + * Sort key priority: + * 1. vCPU count (ascending, unknowns / 0 sort last) + * 2. MemoryGB (ascending, unknowns / 0 sort last) + * 3. resource_type lexical (ascending, stable tie-break across providers) + * + * "Unknown" means the capacity value is absent, null, or 0 (see the + * ComputeDetails godoc: "0 = unknown"). Nulls sort to the end so that + * well-populated rows stay at the top when the user chooses ascending order. + * (Rationale: feedback_nullable_not_zero.md — absent numeric fields must not + * be treated as zero in sort/rank paths.) + * + * @param a - first recommendation (only details + resource_type used) + * @param b - second recommendation (same) + * @returns negative / 0 / positive per Array.prototype.sort contract + */ +export function compareByCapacity( + a: { details?: unknown; resource_type?: string }, + b: { details?: unknown; resource_type?: string } +): number { + const ca = extractCapacity(a.details); + const cb = extractCapacity(b.details); + + const vcpuCmp = compareNullable(ca.vcpu ?? null, cb.vcpu ?? null); + if (vcpuCmp !== 0) return vcpuCmp; + + const memCmp = compareNullable(ca.memory_gb ?? null, cb.memory_gb ?? null); + if (memCmp !== 0) return memCmp; + + // Stable tie-break: lexical by resource_type across providers. + const ta = a.resource_type ?? ''; + const tb = b.resource_type ?? ''; + return ta < tb ? -1 : ta > tb ? 1 : 0; +} + +/** + * extractCapacity pulls vcpu / memory_gb from an opaque details blob. + * Returns zeros (treated as unknown by the comparator) for non-compute + * payloads or absent fields, matching the ComputeDetails omitempty + * behaviour on the wire. + */ +function extractCapacity(details: unknown): ComputeCapacity { + if ( + details !== null && + typeof details === 'object' && + !Array.isArray(details) + ) { + const d = details as Record; + return { + vcpu: typeof d['vcpu'] === 'number' ? d['vcpu'] : null, + memory_gb: + typeof d['memory_gb'] === 'number' ? d['memory_gb'] : null, + }; + } + return { vcpu: null, memory_gb: null }; +} + +/** + * compareNullable sorts null/zero values after all known-positive values. + * For two positive numbers it falls back to numeric ascending order. + * A value of 0 is treated as "unknown" per the ComputeDetails convention. + */ +function compareNullable(a: number | null, b: number | null): number { + const aUnknown = a === null || a === 0; + const bUnknown = b === null || b === 0; + + if (aUnknown && bUnknown) return 0; + if (aUnknown) return 1; // a is unknown -> sort after b + if (bUnknown) return -1; // b is unknown -> sort before it + + return (a as number) - (b as number); +} From e090cc71ba6d490a34bd853beccb6c2c12cd7c22 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 6 Jun 2026 08:47:07 +0200 Subject: [PATCH 2/2] fix(capacity): treat NaN as unknown in capacity comparator (refs #82) Number.isFinite() replaces the typeof === 'number' guard in extractCapacity so that NaN values (which pass typeof but cause NaN subtraction in compareNullable) are folded into the unknown/null bucket and sort last alongside absent/zero/null fields. The previous code would produce NaN from the subtraction path, violating the Array.prototype.sort contract and causing unstable ordering whenever a details blob contained a NaN vcpu or memory_gb. Adds three discriminating tests via a recRaw() helper that bypasses the existing rec() guard to inject NaN directly into the details blob: NaN vcpu sorts after known vcpu, NaN memory_gb sorts after known memory_gb when vcpu ties, and two all-NaN recs fall back to the resource_type tiebreaker without any NaN-comparison instability. compareByCapacity is left as a utility without wiring to the recommendations table sort: adding a vcpu/memory column requires a UX decision (new RecommendationsColumnId, rendered column header, default direction). See the extractCapacity docstring and issue #82. --- frontend/src/__tests__/capacity.test.ts | 38 +++++++++++++++++++++++++ frontend/src/lib/capacity.ts | 20 +++++++++---- 2 files changed, 52 insertions(+), 6 deletions(-) diff --git a/frontend/src/__tests__/capacity.test.ts b/frontend/src/__tests__/capacity.test.ts index f22544590..f52947926 100644 --- a/frontend/src/__tests__/capacity.test.ts +++ b/frontend/src/__tests__/capacity.test.ts @@ -32,6 +32,15 @@ function recNoDetails(resourceType: string): { details: undefined; resource_type return { details: undefined, resource_type: resourceType }; } +/** Builds a rec with raw numeric values including NaN/Infinity (bypasses rec() guards). */ +function recRaw( + resourceType: string, + vcpu: unknown, + memoryGb: unknown +): { details: unknown; resource_type: string } { + return { details: { vcpu, memory_gb: memoryGb }, resource_type: resourceType }; +} + // --------------------------------------------------------------------------- // Basic ordering // --------------------------------------------------------------------------- @@ -114,6 +123,35 @@ describe('compareByCapacity — null/absent/zero sort last', () => { const nullMem = rec('something', 4, null); expect(compareByCapacity(known, nullMem)).toBeLessThan(0); }); + + // NaN guard: typeof NaN === 'number' but NaN is not a finite capacity value. + // extractCapacity must fold NaN into the unknown bucket (null) so that + // compareNullable never sees it; NaN subtraction would return NaN and violate + // the sort contract (NaN is neither < 0 nor > 0 nor === 0). + it('NaN vcpu is treated as unknown and sorts after positive vcpu', () => { + const known = rec('m5.large', 2, 8); + const nanVcpu = recRaw('bad-type', NaN, 8); + expect(compareByCapacity(known, nanVcpu)).toBeLessThan(0); + expect(compareByCapacity(nanVcpu, known)).toBeGreaterThan(0); + }); + + it('NaN memoryGB is treated as unknown and sorts after known memoryGB when vCPU ties', () => { + const known = rec('c5.xlarge', 4, 8); + const nanMem = recRaw('r5.xlarge', 4, NaN); + expect(compareByCapacity(known, nanMem)).toBeLessThan(0); + expect(compareByCapacity(nanMem, known)).toBeGreaterThan(0); + }); + + it('two NaN-vcpu recs: tie resolved by resource_type (no NaN-comparison instability)', () => { + const a = recRaw('alpha', NaN, NaN); + const b = recRaw('beta', NaN, NaN); + const ab = compareByCapacity(a, b); + const ba = compareByCapacity(b, a); + expect(ab).toBeLessThan(0); + expect(ba).toBeGreaterThan(0); + // Symmetry: sign must flip, not collapse to 0 or NaN. + expect(Math.sign(ab)).toBe(-Math.sign(ba)); + }); }); // --------------------------------------------------------------------------- diff --git a/frontend/src/lib/capacity.ts b/frontend/src/lib/capacity.ts index 06c646b28..aabe86ae8 100644 --- a/frontend/src/lib/capacity.ts +++ b/frontend/src/lib/capacity.ts @@ -66,9 +66,18 @@ export function compareByCapacity( /** * extractCapacity pulls vcpu / memory_gb from an opaque details blob. - * Returns zeros (treated as unknown by the comparator) for non-compute - * payloads or absent fields, matching the ComputeDetails omitempty - * behaviour on the wire. + * Returns null for non-compute payloads or absent/non-finite fields, + * matching the ComputeDetails omitempty behaviour on the wire. + * + * Note: Number.isFinite() is used (not typeof === 'number') to reject NaN, + * which passes the typeof check but would produce NaN subtraction results in + * compareNullable and cause an unstable sort. NaN falls into the unknown bucket + * and sorts last alongside null/absent/0 values. + * + * Intended consumer: the recommendations table capacity sort (clicking the + * vCPU/Memory column header). Wiring that column requires a UX decision + * (adding a new RecommendationsColumnId entry and a rendered column header); + * see issue #82 for context. */ function extractCapacity(details: unknown): ComputeCapacity { if ( @@ -78,9 +87,8 @@ function extractCapacity(details: unknown): ComputeCapacity { ) { const d = details as Record; return { - vcpu: typeof d['vcpu'] === 'number' ? d['vcpu'] : null, - memory_gb: - typeof d['memory_gb'] === 'number' ? d['memory_gb'] : null, + vcpu: Number.isFinite(d['vcpu']) ? (d['vcpu'] as number) : null, + memory_gb: Number.isFinite(d['memory_gb']) ? (d['memory_gb'] as number) : null, }; } return { vcpu: null, memory_gb: null };