From c0597b155f7cc5abff6b4c30d500b65b6378b792 Mon Sep 17 00:00:00 2001 From: avivkeller Date: Sun, 9 Aug 2026 18:11:42 -0400 Subject: [PATCH 1/2] comparator-fixes --- scripts/__tests__/comparators.test.mjs | 96 ++++++++++++++++++++++- scripts/comparators/file-size.mjs | 98 +++++++++++++----------- scripts/comparators/files.mjs | 89 ++++++++++++++++++++- scripts/comparators/object-assertion.mjs | 68 ++++++++-------- scripts/comparators/report.mjs | 62 +++++++++++++++ 5 files changed, 337 insertions(+), 76 deletions(-) create mode 100644 scripts/comparators/report.mjs diff --git a/scripts/__tests__/comparators.test.mjs b/scripts/__tests__/comparators.test.mjs index ddcde32d..aec2e073 100644 --- a/scripts/__tests__/comparators.test.mjs +++ b/scripts/__tests__/comparators.test.mjs @@ -139,12 +139,106 @@ 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:/); assert.match(result, /Performance estimate/); assert.doesNotMatch(result, /benchmark\.json/); }); +test('comparators fold away runs that only moved performance', async t => { + const { base, head } = await createDirectories(t); + + await Promise.all([ + writeFile(path.join(base, 'result.json'), '{"value":true}', 'utf8'), + writeFile(path.join(head, 'result.json'), '{"value":true}', 'utf8'), + writeBenchmark(base), + writeBenchmark(head, { ...benchmark, elapsedSeconds: 3 }), + ]); + + const [sizes, objects] = await Promise.all([ + runComparator('file-size', base, head), + runComparator('object-assertion', base, head), + ]); + + for (const result of [sizes, objects]) { + assert.doesNotMatch(result, /## `test` Generator/); + assert.match( + result, + /`test` Generator — performance-only changes<\/summary>/ + ); + assert.match(result, /Generation time:\*\* 50\.0% slower/); + } +}); + +test('comparators pair renamed files that kept their contents', async t => { + const { base, head } = await createDirectories(t); + const baseAssets = path.join(base, 'assets'); + const headAssets = path.join(head, 'assets'); + + await Promise.all([mkdir(baseAssets), mkdir(headAssets)]); + await Promise.all([ + writeFile( + path.join(baseAssets, 'compat.module-2nt43HHR.mjs'), + 'export const compat = 1;', + 'utf8' + ), + writeFile( + path.join(headAssets, 'compat.module-CSTB7Jvj.mjs'), + 'export const compat = 1;', + 'utf8' + ), + ]); + + const result = await runComparator('file-size', base, head); + + assert.match(result, /Output size:\*\* 1 file renamed/); + assert.doesNotMatch(result, /changed · net/); + assert.match( + result, + /- `assets\/compat\.module-2nt43HHR\.mjs → assets\/compat\.module-CSTB7Jvj\.mjs`$/m + ); +}); + +test('comparators weigh a renamed file against the one it replaced', async t => { + const { base, head } = await createDirectories(t); + const baseAssets = path.join(base, 'assets'); + const headAssets = path.join(head, 'assets'); + + await Promise.all([mkdir(baseAssets), mkdir(headAssets)]); + await Promise.all([ + writeFile(path.join(baseAssets, 'index-2nt43HHR.mjs'), 'a', 'utf8'), + writeFile( + path.join(headAssets, 'index-CSTB7Jvj.mjs'), + 'a much longer', + 'utf8' + ), + ]); + + const result = await runComparator('file-size', base, head); + + assert.match(result, /Output size:\*\* 1 file changed · net \+12\.00 B/); + assert.match( + result, + /`assets\/index-2nt43HHR\.mjs → assets\/index-CSTB7Jvj\.mjs` \| 1\.00 B \| 13\.00 B \| \+12\.00 B/ + ); + assert.doesNotMatch(result, /—/); +}); + +test('comparators keep unrelated files apart when only the name looks hashed', async t => { + const { base, head } = await createDirectories(t); + + // Both names end in eight characters, but neither reads like a base64 hash. + await Promise.all([ + writeFile(path.join(base, 'bundle-longname.mjs'), 'a', 'utf8'), + writeFile(path.join(head, 'bundle-nickname.mjs'), 'a much longer', 'utf8'), + ]); + + const result = await runComparator('file-size', base, head); + + assert.match(result, /2 files changed/); + assert.match(result, /`bundle-longname\.mjs` \| 1\.00 B \| —/); + assert.match(result, /`bundle-nickname\.mjs` \| — \| 13\.00 B/); +}); + test('comparators report added and removed output files', async t => { const { base, head } = await createDirectories(t); const baseOutput = path.join(base, 'generator'); diff --git a/scripts/comparators/file-size.mjs b/scripts/comparators/file-size.mjs index 81e2e865..31116d88 100644 --- a/scripts/comparators/file-size.mjs +++ b/scripts/comparators/file-size.mjs @@ -1,9 +1,10 @@ import { stat } from 'node:fs/promises'; import path from 'node:path'; -import { BASE, HEAD, TITLE } from '../constants.mjs'; -import { listOutputFiles } from './files.mjs'; +import { BASE, HEAD } from '../constants.mjs'; +import { pairOutputFiles } from './files.mjs'; import { comparePerformance } from './performance.mjs'; +import { count, isRename, pairName, report } from './report.mjs'; const UNITS = ['B', 'KB', 'MB', 'GB']; @@ -21,74 +22,85 @@ const formatBytes = bytes => { return `${(bytes / Math.pow(1024, i)).toFixed(2)} ${UNITS[i]}`; }; -/** - * Gets all files in a directory with their sizes - * @param {string} dir - Directory path to scan - * @returns {Promise>} Map of filename to size in bytes - */ -const getStats = async dir => { - const files = await listOutputFiles(dir); - return new Map( - await Promise.all( - files.map(async f => [f, (await stat(path.join(dir, f))).size]) - ) - ); -}; +const sizeOf = async (directory, file) => + file ? (await stat(path.join(directory, file))).size : 0; -// Fetch stats for both directories in parallel -const [baseStats, headStats] = await Promise.all([BASE, HEAD].map(getStats)); +const entries = await Promise.all( + (await pairOutputFiles(BASE, HEAD)).map(async pair => { + const [base, head] = await Promise.all([ + sizeOf(BASE, pair.base), + sizeOf(HEAD, pair.head), + ]); -const didChange = f => baseStats.get(f) !== headStats.get(f); + return { ...pair, baseSize: base, headSize: head, diff: head - base }; + }) +); -const toDiffObject = f => ({ - file: f, - base: baseStats.get(f) ?? 0, - head: headStats.get(f) ?? 0, - diff: (headStats.get(f) ?? 0) - (baseStats.get(f) ?? 0), -}); +// A renamed file that weighs the same weighs the same: its hash turned over, +// which earns a line but not a row among the real size movements. +const renamed = entries.filter(entry => isRename(entry) && !entry.diff); // 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) +const changed = entries + .filter(({ base, head, diff }) => diff || !base || !head) .sort((a, b) => Math.abs(b.diff) - Math.abs(a.diff)); const sections = []; // Output markdown table if there are changes -if (changed.length) { +if (changed.length || renamed.length) { const totalDiff = changed.reduce((total, { diff }) => total + diff, 0); const totalSign = totalDiff > 0 ? '+' : ''; - const rows = changed.map(({ file, base, head, diff }) => { + + const rows = changed.map(entry => { + const { base, head, baseSize, headSize, diff } = entry; const sign = diff > 0 ? '+' : ''; const percent = - base === 0 ? '' : ` (${sign}${((diff / base) * 100).toFixed(1)}%)`; + baseSize === 0 + ? '' + : ` (${sign}${((diff / baseSize) * 100).toFixed(1)}%)`; const diffFormatted = `${sign}${formatBytes(diff)}${percent}`; - return `| \`${file}\` | ${baseStats.has(file) ? formatBytes(base) : '—'} | ${headStats.has(file) ? formatBytes(head) : '—'} | ${diffFormatted} |`; + return `| \`${pairName(entry)}\` | ${base ? formatBytes(baseSize) : '—'} | ${head ? formatBytes(headSize) : '—'} | ${diffFormatted} |`; }); + const summary = [ + changed.length && + `${count(changed.length, 'file', 'files')} changed · net ${totalSign}${formatBytes(totalDiff)}`, + renamed.length && `${count(renamed.length, 'file', 'files')} renamed`, + ].filter(Boolean); + + const details = [ + changed.length && + [ + '| File | Main | PR | Change |', + '| --- | ---: | ---: | ---: |', + rows.join('\n'), + ].join('\n'), + renamed.length && + [ + '**Renamed** (identical contents unless noted)', + renamed + .map( + entry => + `- \`${pairName(entry)}\`${entry.identical ? '' : ' (same size, different contents)'}` + ) + .join('\n'), + ].join('\n'), + ].filter(Boolean); + sections.push( [ - `**Output size:** ${changed.length} ${changed.length === 1 ? 'file' : 'files'} changed · net ${totalSign}${formatBytes(totalDiff)}`, + `**Output size:** ${summary.join(' · ')}`, '', '
', 'File size details', '', - '| File | Main | PR | Change |', - '| --- | ---: | ---: | ---: |', - rows.join('\n'), + details.join('\n\n'), '', '
', ].join('\n') ); } -const performance = await comparePerformance(); -if (performance) { - sections.push(performance); -} - -if (sections.length) { - console.log(`${TITLE}\n\n${sections.join('\n\n')}\n`); -} +report(sections, await comparePerformance()); diff --git a/scripts/comparators/files.mjs b/scripts/comparators/files.mjs index 7e7d1035..98cba6ee 100644 --- a/scripts/comparators/files.mjs +++ b/scripts/comparators/files.mjs @@ -1,9 +1,18 @@ -import { glob } from 'node:fs/promises'; +import { createHash } from 'node:crypto'; +import { glob, readFile } 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]); +const HASH_STAMP = /-([A-Za-z0-9_-]{8})(?=\.[^./]+$)/; + +const isStamp = value => /\d/.test(value) && /[A-Z]/.test(value); + +const withoutStamp = file => + file.replace(HASH_STAMP, (stamp, hash) => + isStamp(hash) ? '-[hash]' : stamp + ); export const listOutputFiles = async directory => { const entries = glob('**/*', { @@ -19,3 +28,81 @@ export const listOutputFiles = async directory => { ) .sort(); }; + +const hashFiles = async (directory, files) => + new Map( + await Promise.all( + files.map(async file => [ + file, + createHash('sha256') + .update(await readFile(path.join(directory, file))) + .digest('hex'), + ]) + ) + ); + +const groupBy = (files, key) => + files.reduce((groups, file) => { + const group = key(file); + return groups.set(group, [...(groups.get(group) ?? []), file]); + }, new Map()); + +/** + * Takes the first candidate under `key` that nothing else has claimed yet + */ +const claim = (groups, key, unclaimed) => { + const match = (groups.get(key) ?? []).find(file => unclaimed.has(file)); + unclaimed.delete(match); + return match; +}; + +/** + * Pairs base and head outputs into one entry per file + */ +export const pairOutputFiles = async (baseDirectory, headDirectory) => { + const [baseFiles, headFiles] = await Promise.all( + [baseDirectory, headDirectory].map(listOutputFiles) + ); + + const headNames = new Set(headFiles); + const kept = baseFiles.filter(file => headNames.has(file)); + const keptNames = new Set(kept); + + const baseOnly = baseFiles.filter(file => !keptNames.has(file)); + const headOnly = headFiles.filter(file => !keptNames.has(file)); + + // Only the leftovers are read; equal names already agree on identity. + const [baseHashes, headHashes] = await Promise.all([ + hashFiles(baseDirectory, baseOnly), + hashFiles(headDirectory, headOnly), + ]); + + const unclaimed = new Set(headOnly); + const byContent = groupBy(headOnly, file => headHashes.get(file)); + const bySlot = groupBy(headOnly, withoutStamp); + + const matched = baseOnly + // Equal bytes are proof of a match, so every one of them is settled before + // a slot is handed out on the weaker evidence of a name. + .map(base => ({ + base, + head: claim(byContent, baseHashes.get(base), unclaimed), + })) + .map(({ base, head }) => + head + ? { base, head, identical: true } + : { + base, + head: claim(bySlot, withoutStamp(base), unclaimed), + identical: false, + } + ); + + const sortKey = ({ base, head }) => head ?? base; + + return [ + ...kept.map(file => ({ base: file, head: file, identical: false })), + ...matched, + ...[...unclaimed].map(head => ({ head, identical: false })), + ].sort((a, b) => (sortKey(a) < sortKey(b) ? -1 : 1)); +}; diff --git a/scripts/comparators/object-assertion.mjs b/scripts/comparators/object-assertion.mjs index 33408523..137f3831 100644 --- a/scripts/comparators/object-assertion.mjs +++ b/scripts/comparators/object-assertion.mjs @@ -2,60 +2,66 @@ import assert from 'node:assert'; import { readFile } from 'node:fs/promises'; import { join } from 'node:path'; -import { BASE, HEAD, TITLE } from '../constants.mjs'; -import { listOutputFiles } from './files.mjs'; +import { BASE, HEAD } from '../constants.mjs'; +import { pairOutputFiles } from './files.mjs'; import { comparePerformance } from './performance.mjs'; +import { count, isRename, pairName, report } from './report.mjs'; -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])]; +const pairs = await pairOutputFiles(BASE, HEAD); 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`; +const getFileDiff = async pair => { + const { base, head, identical } = pair; + + if (!base) { + return `- \`${head}\` added`; } - if (!headFileSet.has(file)) { - return `- \`${file}\` removed`; + if (!head) { + return `- \`${base}\` removed`; } - const basePath = join(BASE, file); - const headPath = join(HEAD, file); + // The pair was matched on its bytes, so parsing it could only prove what the + // hashes already did. + if (identical) { + return null; + } - const baseContent = JSON.parse(await readFile(basePath, 'utf-8')); - const headContent = JSON.parse(await readFile(headPath, 'utf-8')); + const baseContent = JSON.parse(await readFile(join(BASE, base), 'utf-8')); + const headContent = JSON.parse(await readFile(join(HEAD, head), 'utf-8')); try { assert.deepStrictEqual(headContent, baseContent); return null; } catch ({ message }) { - return details(file, message); + return details(pairName(pair), message); } }; -const results = await Promise.all(files.map(getFileDiff)); +const results = await Promise.all( + pairs.map(async pair => ({ pair, diff: await getFileDiff(pair) })) +); + +const differences = results.filter(({ diff }) => diff).map(({ diff }) => diff); -const filteredResults = results.filter(Boolean); +// A rename whose contents survived is already spelled out beside its diff, so +// only the ones carrying no other news are counted here. +const renamed = results.filter(({ pair, diff }) => !diff && isRename(pair)); const sections = []; -if (filteredResults.length) { +if (differences.length || renamed.length) { + const summary = [ + differences.length && + count(differences.length, 'file differs', 'files differ'), + renamed.length && `${count(renamed.length, 'file', 'files')} renamed`, + ].filter(Boolean); + sections.push( - `**Output:** ${filteredResults.length} ${filteredResults.length === 1 ? 'file differs' : 'files differ'}`, - filteredResults.join('\n') + `**Output:** ${summary.join(' · ')}`, + ...(differences.length ? [differences.join('\n')] : []) ); } -const performance = await comparePerformance(); -if (performance) { - sections.push(performance); -} - -if (sections.length) { - console.log(`${TITLE}\n\n${sections.join('\n\n')}\n`); -} +report(sections, await comparePerformance()); diff --git a/scripts/comparators/report.mjs b/scripts/comparators/report.mjs new file mode 100644 index 00000000..a81c62e0 --- /dev/null +++ b/scripts/comparators/report.mjs @@ -0,0 +1,62 @@ +import { TITLE } from '../constants.mjs'; + +const HEADLINE = TITLE.replace(/^#+\s*/, ''); + +/** + * @typedef {{ base?: string, head?: string, identical: boolean }} Pair + */ + +/** + * @param {Pair} pair - Paired output file + * @returns {boolean} Whether the file changed name between the two builds + */ +export const isRename = ({ base, head }) => + Boolean(base && head && base !== head); + +/** + * Names a pair, spelling out both sides of a rename. + * + * @param {Pair} pair - Paired output file + * @returns {string} Display name + */ +export const pairName = pair => + isRename(pair) ? `${pair.base} → ${pair.head}` : (pair.head ?? pair.base); + +/** + * @param {number} amount - How many things there are + * @param {string} singular - Wording for one of them + * @param {string} plural - Wording for any other count + * @returns {string} Counted phrase, e.g. "2 files" + */ +export const count = (amount, singular, plural) => + `${amount} ${amount === 1 ? singular : plural}`; + +/** + * Prints one generator's slice of the pull request comment. + * + * @param {Array} sections - Output differences worth showing up front + * @param {string} performance - Performance summary, or an empty string + * @returns {void} + */ +export const report = (sections, performance) => { + if (sections.length) { + console.log( + `${TITLE}\n\n${[...sections, performance].filter(Boolean).join('\n\n')}\n` + ); + + return; + } + + if (performance) { + console.log( + [ + '
', + `${HEADLINE} — performance-only changes`, + '', + performance, + '', + '
', + ].join('\n') + ); + } +}; From 36af99457623669b282d03b45f587b0cde456fff Mon Sep 17 00:00:00 2001 From: avivkeller Date: Sun, 9 Aug 2026 18:19:07 -0400 Subject: [PATCH 2/2] comparator-fixes --- scripts/__tests__/comparators.test.mjs | 27 ------------------- scripts/comparators/performance.mjs | 6 ++--- scripts/comparators/report.mjs | 37 +++++++++++++------------- 3 files changed, 20 insertions(+), 50 deletions(-) diff --git a/scripts/__tests__/comparators.test.mjs b/scripts/__tests__/comparators.test.mjs index aec2e073..e4ed57af 100644 --- a/scripts/__tests__/comparators.test.mjs +++ b/scripts/__tests__/comparators.test.mjs @@ -77,8 +77,6 @@ test('comparePerformance summarizes benchmark differences', async t => { result, /\*\*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 => { @@ -144,31 +142,6 @@ test('object comparator treats benchmark data as metadata', async t => { assert.doesNotMatch(result, /benchmark\.json/); }); -test('comparators fold away runs that only moved performance', async t => { - const { base, head } = await createDirectories(t); - - await Promise.all([ - writeFile(path.join(base, 'result.json'), '{"value":true}', 'utf8'), - writeFile(path.join(head, 'result.json'), '{"value":true}', 'utf8'), - writeBenchmark(base), - writeBenchmark(head, { ...benchmark, elapsedSeconds: 3 }), - ]); - - const [sizes, objects] = await Promise.all([ - runComparator('file-size', base, head), - runComparator('object-assertion', base, head), - ]); - - for (const result of [sizes, objects]) { - assert.doesNotMatch(result, /## `test` Generator/); - assert.match( - result, - /`test` Generator — performance-only changes<\/summary>/ - ); - assert.match(result, /Generation time:\*\* 50\.0% slower/); - } -}); - test('comparators pair renamed files that kept their contents', async t => { const { base, head } = await createDirectories(t); const baseAssets = path.join(base, 'assets'); diff --git a/scripts/comparators/performance.mjs b/scripts/comparators/performance.mjs index f3c594ee..d81ff160 100644 --- a/scripts/comparators/performance.mjs +++ b/scripts/comparators/performance.mjs @@ -77,7 +77,7 @@ const METRICS = [ * * @param {string} baseDirectory - Base artifact directory * @param {string} headDirectory - Head artifact directory - * @returns {Promise} Markdown table, or an empty string + * @returns {Promise} Markdown list, or an empty string */ export const comparePerformance = async ( baseDirectory = BASE, @@ -102,7 +102,5 @@ export const comparePerformance = async ( return `- **${label}:** ${formatChange(baseValue, headValue, change)} (${format(baseValue)} → ${format(headValue)})`; }); - return ['**Performance estimate** (single CI run)', ...rows].join( - '\n' - ); + return rows.join('\n'); }; diff --git a/scripts/comparators/report.mjs b/scripts/comparators/report.mjs index a81c62e0..0301b4e5 100644 --- a/scripts/comparators/report.mjs +++ b/scripts/comparators/report.mjs @@ -1,6 +1,20 @@ import { TITLE } from '../constants.mjs'; -const HEADLINE = TITLE.replace(/^#+\s*/, ''); +/** + * Folds the performance estimate + * + * @param {string} performance - Performance summary + * @returns {string} Collapsed Markdown section + */ +const fold = performance => + [ + '
', + '

Performance estimate

', + '', + performance, + '', + '
', + ].join('\n'); /** * @typedef {{ base?: string, head?: string, identical: boolean }} Pair @@ -39,24 +53,9 @@ export const count = (amount, singular, plural) => * @returns {void} */ export const report = (sections, performance) => { - if (sections.length) { - console.log( - `${TITLE}\n\n${[...sections, performance].filter(Boolean).join('\n\n')}\n` - ); - - return; - } + const body = [...sections, performance && fold(performance)].filter(Boolean); - if (performance) { - console.log( - [ - '
', - `${HEADLINE} — performance-only changes`, - '', - performance, - '', - '
', - ].join('\n') - ); + if (body.length) { + console.log(`${TITLE}\n\n${body.join('\n\n')}\n`); } };