[P1 CI] Graph benchmark can pass with missing scenarios and misreports FPS regressions #131

New issue

Open

opened 2026-07-28 22:59:56 +00:00 by lost-rob0t · 0 comments

lost-rob0t commented

2026-07-28 22:59:56 +00:00

(Migrated from github.com)

Finding

The regression comparator in PR #124 skips missing data instead of failing closed.

if (!Number.isFinite(before) || !Number.isFinite(after)) return;

It also iterates only candidate results:

for (const result of candidate.iterations?.final || candidate.results || []) {
  const previous = expected.get(key(result));
  if (!previous) continue;
  // ...
}

Therefore all of these can print Graph benchmark comparison passed:

the candidate contains zero results;
one or more baseline scenarios disappear;
a required metric is missing, null, NaN, or structurally renamed;
the fixture key changes and no longer matches the baseline.

The Markdown report has a separate direction bug. percent() assumes lower is always better:

return `${(((before - after) / before) * 100).toFixed(1)}%`;

That is correct for milliseconds but backwards for FPS: higher FPS is rendered as a negative improvement, while lower FPS is rendered as positive.

The workflow also explicitly succeeds when no checked baseline exists, so the current branch has no enforced regression gate.

Required fix

Validate complete scenario coverage

function scenarioResults(report) {
  const results = report.iterations?.final || report.results;
  if (!Array.isArray(results) || results.length === 0) {
    throw new Error("Benchmark result set is empty");
  }
  return new Map(results.map((result) => [key(result), result]));
}

const expected = scenarioResults(baseline);
const actual = scenarioResults(candidate);

for (const scenario of expected.keys()) {
  if (!actual.has(scenario)) failures.push(`${scenario}: candidate scenario is missing`);
}
for (const scenario of actual.keys()) {
  if (!expected.has(scenario)) failures.push(`${scenario}: no reviewed baseline exists`);
}

Require every gated metric

function requireFinite(value, name) {
  if (!Number.isFinite(value)) {
    failures.push(`${name}: metric is missing or non-finite`);
    return null;
  }
  return value;
}

function compareMetric(name, beforeValue, afterValue, direction = "lower") {
  const before = requireFinite(beforeValue, `${name} baseline`);
  const after = requireFinite(afterValue, `${name} candidate`);
  if (before === null || after === null) return;

  const denominator = Math.max(Math.abs(before), Number.EPSILON);
  const regression = direction === "lower"
    ? (after - before) / denominator
    : (before - after) / denominator;

  if (regression > limit) {
    failures.push(`${name} regressed ${(regression * 100).toFixed(1)}%`);
  }
}

Make report direction explicit

function improvement(before, after, direction = "lower") {
  if (!Number.isFinite(before) || !Number.isFinite(after) || before === 0) return "—";
  const delta = direction === "lower"
    ? (before - after) / Math.abs(before)
    : (after - before) / Math.abs(before);
  return `${(delta * 100).toFixed(1)}%`;
}

const rows = [
  ["First usable", before.metrics.firstUsable.median, after.metrics.firstUsable.median, "lower", "ms"],
  ["Filter", before.metrics.filter?.median, after.metrics.filter?.median, "lower", "ms"],
  ["Selection", before.metrics.selection?.median, after.metrics.selection?.median, "lower", "ms"],
  ["Viewport FPS", before.metrics.viewport?.medianFps, after.metrics.viewport?.medianFps, "higher", "fps"]
];

Enforce a baseline before merge

The CI job should fail—not echo and continue—when the baseline is absent:

test -f benchmarks/baseline/graph-baseline.json || {
  echo "Reviewed graph performance baseline is missing" >&2
  exit 1
}
npm run bench:graph:compare

Acceptance criteria

Empty candidate output fails.
Missing and extra scenarios fail.
Missing/non-finite gated metrics fail.
FPS improvement direction is correct in reports and comparisons.
Division by zero cannot produce a misleading pass.
A reviewed checked-in baseline is mandatory before PR #124 becomes merge-ready.
Unit tests cover all failure modes above.

## Finding The regression comparator in PR #124 skips missing data instead of failing closed. ```js if (!Number.isFinite(before) || !Number.isFinite(after)) return; ``` It also iterates only candidate results: ```js for (const result of candidate.iterations?.final || candidate.results || []) { const previous = expected.get(key(result)); if (!previous) continue; // ... } ``` Therefore all of these can print `Graph benchmark comparison passed`: - the candidate contains zero results; - one or more baseline scenarios disappear; - a required metric is missing, `null`, `NaN`, or structurally renamed; - the fixture key changes and no longer matches the baseline. The Markdown report has a separate direction bug. `percent()` assumes lower is always better: ```js return `${(((before - after) / before) * 100).toFixed(1)}%`; ``` That is correct for milliseconds but backwards for FPS: higher FPS is rendered as a negative improvement, while lower FPS is rendered as positive. The workflow also explicitly succeeds when no checked baseline exists, so the current branch has no enforced regression gate. ## Required fix ### Validate complete scenario coverage ```js function scenarioResults(report) { const results = report.iterations?.final || report.results; if (!Array.isArray(results) || results.length === 0) { throw new Error("Benchmark result set is empty"); } return new Map(results.map((result) => [key(result), result])); } const expected = scenarioResults(baseline); const actual = scenarioResults(candidate); for (const scenario of expected.keys()) { if (!actual.has(scenario)) failures.push(`${scenario}: candidate scenario is missing`); } for (const scenario of actual.keys()) { if (!expected.has(scenario)) failures.push(`${scenario}: no reviewed baseline exists`); } ``` ### Require every gated metric ```js function requireFinite(value, name) { if (!Number.isFinite(value)) { failures.push(`${name}: metric is missing or non-finite`); return null; } return value; } function compareMetric(name, beforeValue, afterValue, direction = "lower") { const before = requireFinite(beforeValue, `${name} baseline`); const after = requireFinite(afterValue, `${name} candidate`); if (before === null || after === null) return; const denominator = Math.max(Math.abs(before), Number.EPSILON); const regression = direction === "lower" ? (after - before) / denominator : (before - after) / denominator; if (regression > limit) { failures.push(`${name} regressed ${(regression * 100).toFixed(1)}%`); } } ``` ### Make report direction explicit ```js function improvement(before, after, direction = "lower") { if (!Number.isFinite(before) || !Number.isFinite(after) || before === 0) return "—"; const delta = direction === "lower" ? (before - after) / Math.abs(before) : (after - before) / Math.abs(before); return `${(delta * 100).toFixed(1)}%`; } const rows = [ ["First usable", before.metrics.firstUsable.median, after.metrics.firstUsable.median, "lower", "ms"], ["Filter", before.metrics.filter?.median, after.metrics.filter?.median, "lower", "ms"], ["Selection", before.metrics.selection?.median, after.metrics.selection?.median, "lower", "ms"], ["Viewport FPS", before.metrics.viewport?.medianFps, after.metrics.viewport?.medianFps, "higher", "fps"] ]; ``` ### Enforce a baseline before merge The CI job should fail—not echo and continue—when the baseline is absent: ```bash test -f benchmarks/baseline/graph-baseline.json || { echo "Reviewed graph performance baseline is missing" >&2 exit 1 } npm run bench:graph:compare ``` ## Acceptance criteria - Empty candidate output fails. - Missing and extra scenarios fail. - Missing/non-finite gated metrics fail. - FPS improvement direction is correct in reports and comparisons. - Division by zero cannot produce a misleading pass. - A reviewed checked-in baseline is mandatory before PR #124 becomes merge-ready. - Unit tests cover all failure modes above.