chore: improve comparison comments (#923)

* chore: improve comparison comments

* fixup!

Author: avivkeller

scripts/comparators/file-size.mjs

@@ -1,7 +1,8 @@
-import { stat, readdir } from 'node:fs/promises';
+import { stat } from 'node:fs/promises';
 import path from 'node:path';
 
-import { BASE, BENCHMARK_FILE, HEAD, TITLE } from '../constants.mjs';
+import { BASE, HEAD, TITLE } from '../constants.mjs';
+import { listOutputFiles } from './files.mjs';
 import { comparePerformance } from './performance.mjs';
 
 const UNITS = ['B', 'KB', 'MB', 'GB'];
@@ -26,7 +27,7 @@ const formatBytes = bytes => {
  * @returns {Promise<Map<string, number>>} Map of filename to size in bytes
  */
 const getStats = async dir => {
-  const files = (await readdir(dir)).filter(file => file !== BENCHMARK_FILE);
+  const files = await listOutputFiles(dir);
   return new Map(
     await Promise.all(
       files.map(async f => [f, (await stat(path.join(dir, f))).size])
@@ -37,18 +38,16 @@ const getStats = async dir => {
 // Fetch stats for both directories in parallel
 const [baseStats, headStats] = await Promise.all([BASE, HEAD].map(getStats));
 
-const didChange = f =>
-  baseStats.has(f) && headStats.has(f) && baseStats.get(f) !== headStats.get(f);
+const didChange = f => baseStats.get(f) !== headStats.get(f);
 
 const toDiffObject = f => ({
   file: f,
-  base: baseStats.get(f),
-  head: headStats.get(f),
-  diff: headStats.get(f) - baseStats.get(f),
+  base: baseStats.get(f) ?? 0,
+  head: headStats.get(f) ?? 0,
+  diff: (headStats.get(f) ?? 0) - (baseStats.get(f) ?? 0),
 });
 
-// Find files that exist in both directories but have different sizes,
-// then sort by absolute diff (largest changes first)
+// Find files whose presence or size changed, then show the largest changes first.
 const changed = [...new Set([...baseStats.keys(), ...headStats.keys()])]
   .filter(didChange)
   .map(toDiffObject)
@@ -58,19 +57,30 @@ const sections = [];
 
 // Output markdown table if there are changes
 if (changed.length) {
+  const totalDiff = changed.reduce((total, { diff }) => total + diff, 0);
+  const totalSign = totalDiff > 0 ? '+' : '';
   const rows = changed.map(({ file, base, head, diff }) => {
     const sign = diff > 0 ? '+' : '';
-    const percent = `${sign}${((diff / base) * 100).toFixed(2)}%`;
-    const diffFormatted = `${sign}${formatBytes(diff)} (${percent})`;
+    const percent =
+      base === 0 ? '' : ` (${sign}${((diff / base) * 100).toFixed(1)}%)`;
+    const diffFormatted = `${sign}${formatBytes(diff)}${percent}`;
 
-    return `| \`${file}\` | ${formatBytes(base)} | ${formatBytes(head)} | ${diffFormatted} |`;
+    return `| \`${file}\` | ${baseStats.has(file) ? formatBytes(base) : '—'} | ${headStats.has(file) ? formatBytes(head) : '—'} | ${diffFormatted} |`;
   });
 
   sections.push(
-    '### Output size',
-    '| File | Base | Head | Diff |',
-    '|-|-|-|-|',
-    rows.join('\n')
+    [
+      `**Output size:** ${changed.length} ${changed.length === 1 ? 'file' : 'files'} changed · net ${totalSign}${formatBytes(totalDiff)}`,
+      '',
+      '<details>',
+      '<summary>File size details</summary>',
+      '',
+      '| File | Main | PR | Change |',
+      '| --- | ---: | ---: | ---: |',
+      rows.join('\n'),
+      '',
+      '</details>',
+    ].join('\n')
   );
 }
 

scripts/comparators/files.mjs

@@ -0,0 +1,21 @@
+import { glob } from 'node:fs/promises';
+import path from 'node:path';
+
+import { BENCHMARK_FILE, COMPARISON_FILE } from '../constants.mjs';
+
+const METADATA_FILES = new Set([BENCHMARK_FILE, COMPARISON_FILE]);
+
+export const listOutputFiles = async directory => {
+  const entries = glob('**/*', {
+    cwd: directory,
+    withFileTypes: true,
+    exclude: entry => METADATA_FILES.has(entry.name),
+  });
+
+  return (await Array.fromAsync(entries))
+    .filter(entry => entry.isFile())
+    .map(entry =>
+      path.relative(directory, path.join(entry.parentPath, entry.name))
+    )
+    .sort();
+};

scripts/comparators/object-assertion.mjs

@@ -1,16 +1,30 @@
 import assert from 'node:assert';
-import { readdir, readFile } from 'node:fs/promises';
+import { readFile } from 'node:fs/promises';
 import { join } from 'node:path';
 
-import { BASE, BENCHMARK_FILE, HEAD, TITLE } from '../constants.mjs';
+import { BASE, HEAD, TITLE } from '../constants.mjs';
+import { listOutputFiles } from './files.mjs';
 import { comparePerformance } from './performance.mjs';
 
-const files = (await readdir(BASE)).filter(file => file !== BENCHMARK_FILE);
+const [baseFiles, headFiles] = await Promise.all(
+  [BASE, HEAD].map(directory => listOutputFiles(directory))
+);
+const baseFileSet = new Set(baseFiles);
+const headFileSet = new Set(headFiles);
+const files = [...new Set([...baseFiles, ...headFiles])];
 
 export const details = (summary, diff) =>
   `<details>\n<summary>${summary}</summary>\n\n\`\`\`diff\n${diff}\n\`\`\`\n\n</details>`;
 
 const getFileDiff = async file => {
+  if (!baseFileSet.has(file)) {
+    return `- \`${file}\` added`;
+  }
+
+  if (!headFileSet.has(file)) {
+    return `- \`${file}\` removed`;
+  }
+
   const basePath = join(BASE, file);
   const headPath = join(HEAD, file);
 
@@ -31,7 +45,10 @@ const filteredResults = results.filter(Boolean);
 
 const sections = [];
 if (filteredResults.length) {
-  sections.push('### Output', filteredResults.join('\n'));
+  sections.push(
+    `**Output:** ${filteredResults.length} ${filteredResults.length === 1 ? 'file differs' : 'files differ'}`,
+    filteredResults.join('\n')
+  );
 }
 
 const performance = await comparePerformance();

scripts/comparators/performance.mjs

@@ -27,14 +27,17 @@ const formatPercent = (base, diff) => {
     return 'n/a';
   }
 
-  const percent = (diff / base) * 100;
-  return `${percent > 0 ? '+' : ''}${percent.toFixed(2)}%`;
+  return `${Math.abs((diff / base) * 100).toFixed(1)}%`;
 };
 
-const formatDiff = (base, head, formatter) => {
+const formatChange = (base, head, { increase, decrease }) => {
   const diff = head - base;
-  const value = `${diff > 0 ? '+' : ''}${formatter(diff)}`;
-  return `${value} (${formatPercent(base, diff)})`;
+
+  if (diff === 0) {
+    return 'unchanged';
+  }
+
+  return `${formatPercent(base, diff)} ${diff > 0 ? increase : decrease}`;
 };
 
 const readBenchmark = async directory => {
@@ -53,24 +56,16 @@ const readBenchmark = async directory => {
 
 const METRICS = [
   {
-    label: 'Elapsed time',
+    label: 'Generation time',
     value: benchmark => benchmark.elapsedSeconds,
     format: formatSeconds,
+    change: { increase: 'slower', decrease: 'faster' },
   },
   {
-    label: 'User CPU time',
-    value: benchmark => benchmark.userCpuSeconds,
-    format: formatSeconds,
-  },
-  {
-    label: 'System CPU time',
-    value: benchmark => benchmark.systemCpuSeconds,
-    format: formatSeconds,
-  },
-  {
-    label: 'Peak resident memory',
+    label: 'Peak memory',
     value: benchmark => benchmark.maxRssKiB * 1024,
     format: formatBytes,
+    change: { increase: 'higher', decrease: 'lower' },
   },
 ];
 
@@ -96,21 +91,18 @@ export const comparePerformance = async (
     return '';
   }
 
-  const rows = METRICS.map(({ label, value, format }) => {
+  const rows = METRICS.map(({ label, value, format, change }) => {
     const baseValue = value(base);
     const headValue = value(head);
 
     if (!Number.isFinite(baseValue) || !Number.isFinite(headValue)) {
       throw new TypeError(`Invalid ${label.toLowerCase()} benchmark value`);
     }
 
-    return `| ${label} | ${format(baseValue)} | ${format(headValue)} | ${formatDiff(baseValue, headValue, format)} |`;
+    return `- **${label}:** ${formatChange(baseValue, headValue, change)} (${format(baseValue)} → ${format(headValue)})`;
   });
 
-  return [
-    '### Performance',
-    '| Metric | Base | Head | Diff |',
-    '|-|-|-|-|',
-    ...rows,
-  ].join('\n');
+  return ['**Performance estimate** <sub>(single CI run)</sub>', ...rows].join(
+    '\n'
+  );
 };

scripts/constants.mjs

@@ -11,6 +11,7 @@ export const TITLE =
   process.env.TITLE || `## \`${process.env.GENERATOR ?? '...'}\` Generator`;
 
 export const BENCHMARK_FILE = 'benchmark.json';
+export const COMPARISON_FILE = 'comparison.txt';
 
 // MDN Constants
 export const MDN_COMPAT_URL =

src/__tests__/comparators.test.mjs

@@ -54,7 +54,7 @@ const runComparator = async (name, base, head) => {
   return stdout;
 };
 
-test('comparePerformance formats benchmark differences', async t => {
+test('comparePerformance summarizes benchmark differences', async t => {
   const { base, head } = await createDirectories(t);
 
   await Promise.all([
@@ -71,16 +71,14 @@ test('comparePerformance formats benchmark differences', async t => {
 
   assert.match(
     result,
-    /Elapsed time \| 2\.00 s \| 3\.00 s \| \+1\.00 s \(\+50\.00%\)/
+    /\*\*Generation time:\*\* 50\.0% slower \(2\.00 s → 3\.00 s\)/
   );
   assert.match(
     result,
-    /User CPU time \| 1\.00 s \| 750\.00 ms \| -250\.00 ms \(-25\.00%\)/
-  );
-  assert.match(
-    result,
-    /Peak resident memory \| 1\.00 MB \| 1\.50 MB \| \+512\.00 KB \(\+50\.00%\)/
+    /\*\*Peak memory:\*\* 50\.0% higher \(1\.00 MB → 1\.50 MB\)/
   );
+  assert.match(result, /single CI run/);
+  assert.doesNotMatch(result, /CPU time/);
 });
 
 test('comparePerformance omits results when an artifact has no benchmark', async t => {
@@ -101,7 +99,7 @@ test('comparePerformance rejects invalid benchmark values', async t => {
 
   await assert.rejects(
     comparePerformance(base, head),
-    /Invalid peak resident memory benchmark value/
+    /Invalid peak memory benchmark value/
   );
 });
 
@@ -118,9 +116,14 @@ test('file-size comparator combines output and performance results', async t =>
   const result = await runComparator('file-size', base, head);
 
   assert.equal(result.match(/## `test` Generator/g)?.length, 1);
-  assert.match(result, /### Output size/);
+  assert.match(result, /Output size:.*1 file changed · net \+11\.00 B/);
+  assert.match(result, /<summary>File size details<\/summary>/);
+  assert.match(
+    result,
+    /\| File \| Main \| PR \| Change \|\n\| --- \| ---: \| ---: \| ---: \|/
+  );
   assert.match(result, /`result\.txt`/);
-  assert.match(result, /### Performance/);
+  assert.match(result, /Performance estimate/);
   assert.doesNotMatch(result, /benchmark\.json/);
 });
 
@@ -137,7 +140,33 @@ test('object comparator treats benchmark data as metadata', async t => {
   const result = await runComparator('object-assertion', base, head);
 
   assert.equal(result.match(/## `test` Generator/g)?.length, 1);
-  assert.doesNotMatch(result, /### Output\n/);
-  assert.match(result, /### Performance/);
+  assert.doesNotMatch(result, /\*\*Output:/);
+  assert.match(result, /Performance estimate/);
   assert.doesNotMatch(result, /benchmark\.json/);
 });
+
+test('comparators report added and removed output files', async t => {
+  const { base, head } = await createDirectories(t);
+  const baseOutput = path.join(base, 'generator');
+  const headOutput = path.join(head, 'generator');
+
+  await Promise.all([mkdir(baseOutput), mkdir(headOutput)]);
+  await Promise.all([
+    writeFile(path.join(baseOutput, 'removed.json'), '{"old":true}', 'utf8'),
+    writeFile(path.join(headOutput, 'added.json'), '{"new":true}', 'utf8'),
+    writeFile(path.join(head, 'comparison.txt'), '', 'utf8'),
+  ]);
+
+  const [sizes, objects] = await Promise.all([
+    runComparator('file-size', base, head),
+    runComparator('object-assertion', base, head),
+  ]);
+
+  assert.match(sizes, /2 files changed/);
+  assert.match(sizes, /`generator\/added\.json` \| — \| 12\.00 B/);
+  assert.match(sizes, /`generator\/removed\.json` \| 12\.00 B \| —/);
+  assert.match(objects, /`generator\/added\.json` added/);
+  assert.match(objects, /`generator\/removed\.json` removed/);
+  assert.doesNotMatch(sizes, /comparison\.txt|4\.00 KB/);
+  assert.doesNotMatch(objects, /comparison\.txt/);
+});