From 7d11366dbf7db715dd22b1d9e9b29623bfb37c2f Mon Sep 17 00:00:00 2001 From: default <216188+jdx@users.noreply.github.com> Date: Fri, 28 Aug 2026 13:23:18 +0000 Subject: [PATCH] fix: normalize pnpm 12 package benchmark data --- scripts/benchmark-data.test.js | 84 +++++++++++++++++++++++++ scripts/generate-chart.js | 112 +++++++++++++++++++-------------- scripts/package-count.sh | 5 +- 3 files changed, 153 insertions(+), 48 deletions(-) create mode 100644 scripts/benchmark-data.test.js diff --git a/scripts/benchmark-data.test.js b/scripts/benchmark-data.test.js new file mode 100644 index 0000000000..f8fc8e3ff4 --- /dev/null +++ b/scripts/benchmark-data.test.js @@ -0,0 +1,84 @@ +const assert = require("node:assert/strict"); +const fs = require("node:fs"); +const os = require("node:os"); +const path = require("node:path"); +const { spawnSync } = require("node:child_process"); +const test = require("node:test"); + +const { normalizeTiming } = require("./generate-chart.js"); + +test("normalizes per-package timing and standard deviation", () => { + assert.deepEqual(normalizeTiming({ mean: 1.2, stddev: 0.12 }, 60, true), { + value: 20, + stddev: 2, + }); +}); + +test("does not mix total seconds into per-package data without a count", () => { + assert.equal( + normalizeTiming({ mean: 1.2, stddev: 0.12 }, undefined, true), + undefined, + ); + assert.equal( + normalizeTiming({ mean: 1.2, stddev: 0.12 }, 0, true), + undefined, + ); +}); + +test("preserves total timing units", () => { + assert.deepEqual( + normalizeTiming({ mean: 1.2, stddev: 0.12 }, undefined, false), + { + value: 1.2, + stddev: 0.12, + }, + ); +}); + +for (const fixture of [ + { + name: "pnpm 11 YAML", + modules: "packageManager: pnpm@11.0.0\n", + expectedFile: "pnpm-count.txt", + }, + { + name: "pnpm 12 JSON", + modules: '{\n "packageManager": "pnpm@12.0.0"\n}\n', + expectedFile: "pacquet-count.txt", + }, +]) { + test(`attributes package counts from ${fixture.name} metadata`, (t) => { + const tempDir = fs.mkdtempSync( + path.join(os.tmpdir(), "package-count-test-"), + ); + t.after(() => fs.rmSync(tempDir, { recursive: true, force: true })); + + const outputDir = path.join(tempDir, "results"); + const packageDir = path.join(tempDir, "node_modules", "example"); + fs.mkdirSync(packageDir, { recursive: true }); + fs.writeFileSync( + path.join(tempDir, "pnpm-lock.yaml"), + "lockfileVersion: '9.0'\n", + ); + fs.writeFileSync( + path.join(tempDir, "node_modules", ".modules.yaml"), + fixture.modules, + ); + fs.writeFileSync( + path.join(packageDir, "package.json"), + '{"name":"example"}\n', + ); + + const result = spawnSync( + "bash", + [path.join(__dirname, "package-count.sh"), outputDir], + { cwd: tempDir, encoding: "utf8" }, + ); + + assert.equal(result.status, 0, result.stderr); + assert.equal( + fs.readFileSync(path.join(outputDir, fixture.expectedFile), "utf8"), + "1\n", + ); + }); +} diff --git a/scripts/generate-chart.js b/scripts/generate-chart.js index df565564a2..2014e4c731 100644 --- a/scripts/generate-chart.js +++ b/scripts/generate-chart.js @@ -8,19 +8,11 @@ const fs = require("fs"); const path = require("path"); const DATE = process.argv[2]; -if (!DATE) { - console.error("Error: Date argument is required"); - process.exit(1); -} // Optional commit SHA passed as second argument const COMMIT_SHA = process.argv[3] || ""; -const RESULTS_DIR = path.resolve("results", DATE); -if (!fs.existsSync(RESULTS_DIR)) { - console.error(`Error: Results directory ${RESULTS_DIR} does not exist`); - process.exit(1); -} +const RESULTS_DIR = path.resolve("results", DATE || ""); // Colors for different package managers const COLORS = { @@ -62,6 +54,27 @@ const parseNumeric = (value) => { return undefined; }; +const normalizeTiming = (result, count, perPackageCount) => { + if (!result || typeof result.mean !== "number") { + return undefined; + } + + if (!perPackageCount) { + return { value: result.mean, stddev: result.stddev }; + } + + if (typeof count !== "number" || count <= 0) { + return undefined; + } + + const scale = 1000 / count; + return { + value: result.mean * scale, + stddev: + typeof result.stddev === "number" ? result.stddev * scale : undefined, + }; +}; + // Read and process results function readResults(file) { try { @@ -163,27 +176,24 @@ function generateChartData(option = {}) { const didFail = pmResult.failed || !Number.isFinite(pmResult.mean); const count = packageCounts[pm]; - let value = - typeof pmResult.mean === "number" ? pmResult.mean : undefined; - - if ( - !didFail && - option.perPackageCount && - typeof count === "number" && - count > 0 && - typeof value === "number" - ) { - value = (value / count) * 1000; + const timing = didFail + ? undefined + : normalizeTiming(pmResult, count, option.perPackageCount); + + if (!didFail && option.perPackageCount && !timing) { + console.warn( + `Warning: Skipping ${pm} in ${fixture}-${variation} per-package data because its package count is missing`, + ); } - if (!didFail && typeof value === "number") { - validValues.push(value); + if (timing) { + validValues.push(timing.value); } pmEntries[pm] = { didFail, - value: didFail ? undefined : value, - stddev: didFail ? undefined : pmResult.stddev, + value: timing?.value, + stddev: timing?.stddev, count, }; }); @@ -325,29 +335,24 @@ function generateRegistryChartData(option = {}) { const didFail = registryResult.failed || !Number.isFinite(registryResult.mean); const count = packageCounts[registry]; - let value = - typeof registryResult.mean === "number" - ? registryResult.mean - : undefined; - - if ( - !didFail && - option.perPackageCount && - typeof count === "number" && - count > 0 && - typeof value === "number" - ) { - value = (value / count) * 1000; + const timing = didFail + ? undefined + : normalizeTiming(registryResult, count, option.perPackageCount); + + if (!didFail && option.perPackageCount && !timing) { + console.warn( + `Warning: Skipping ${registry} in ${fixture}-${variation} per-package data because its package count is missing`, + ); } - if (!didFail && typeof value === "number") { - validValues.push(value); + if (timing) { + validValues.push(timing.value); } pmEntries[registry] = { didFail, - value: didFail ? undefined : value, - stddev: didFail ? undefined : registryResult.stddev, + value: timing?.value, + stddev: timing?.stddev, count, }; }); @@ -489,10 +494,23 @@ const dumpChartData = () => { } }; -try { - dumpChartData(); - console.log("Chart generation complete!"); -} catch (error) { - console.error("Error generating chart:", error); - process.exit(1); +if (require.main === module) { + if (!DATE) { + console.error("Error: Date argument is required"); + process.exit(1); + } + if (!fs.existsSync(RESULTS_DIR)) { + console.error(`Error: Results directory ${RESULTS_DIR} does not exist`); + process.exit(1); + } + + try { + dumpChartData(); + console.log("Chart generation complete!"); + } catch (error) { + console.error("Error generating chart:", error); + process.exit(1); + } } + +module.exports = { normalizeTiming }; diff --git a/scripts/package-count.sh b/scripts/package-count.sh index 4d2b845ec6..4fa7057195 100644 --- a/scripts/package-count.sh +++ b/scripts/package-count.sh @@ -22,7 +22,10 @@ infer_package_manager() { # pnpm-lock.yaml, so the lockfile alone can't tell them apart. The # installer records its own version in node_modules/.modules.yaml # (packageManager: pnpm@); v12+ is pacquet. - pnpm_major=$(sed -n "s/^packageManager: ['\"]\{0,1\}pnpm@\([0-9][0-9]*\).*/\1/p" node_modules/.modules.yaml 2>/dev/null | head -1) + # pnpm 11 writes YAML, while pnpm 12 currently writes JSON despite the + # .yaml extension. Accept both `packageManager: pnpm@11...` and + # `"packageManager": "pnpm@12..."`. + pnpm_major=$(sed -En 's/^[[:space:]]*"?packageManager"?[[:space:]]*:[[:space:]]*"?pnpm@([0-9]+).*/\1/p' node_modules/.modules.yaml 2>/dev/null | head -1) if [[ -n "$pnpm_major" && "$pnpm_major" -ge 12 ]]; then echo "pacquet" else