diff --git a/scripts/comparators/file-size.mjs b/scripts/comparators/file-size.mjs index affce49a..81e2e865 100644 --- a/scripts/comparators/file-size.mjs +++ b/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 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)}`, + '', + '
', + 'File size details', + '', + '| File | Main | PR | Change |', + '| --- | ---: | ---: | ---: |', + rows.join('\n'), + '', + '
', + ].join('\n') ); } diff --git a/scripts/comparators/files.mjs b/scripts/comparators/files.mjs new file mode 100644 index 00000000..7e7d1035 --- /dev/null +++ b/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(); +}; diff --git a/scripts/comparators/object-assertion.mjs b/scripts/comparators/object-assertion.mjs index 0d670edd..33408523 100644 --- a/scripts/comparators/object-assertion.mjs +++ b/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) => `
\n${summary}\n\n\`\`\`diff\n${diff}\n\`\`\`\n\n
`; 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(); diff --git a/scripts/comparators/performance.mjs b/scripts/comparators/performance.mjs index 4e2f5b0e..f3c594ee 100644 --- a/scripts/comparators/performance.mjs +++ b/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,7 +91,7 @@ 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); @@ -104,13 +99,10 @@ export const comparePerformance = async ( 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** (single CI run)', ...rows].join( + '\n' + ); }; diff --git a/scripts/constants.mjs b/scripts/constants.mjs index 4f5a5d56..fc23dfd0 100644 --- a/scripts/constants.mjs +++ b/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 = diff --git a/src/__tests__/comparators.test.mjs b/src/__tests__/comparators.test.mjs index a1e7e483..c91ce8bf 100644 --- a/src/__tests__/comparators.test.mjs +++ b/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, /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/); +});