From 7de04e3c3227915eb7fd977fd3031677a3eae562 Mon Sep 17 00:00:00 2001 From: Christopher Hiller Date: Wed, 17 Dec 2025 23:34:16 -0800 Subject: [PATCH 1/3] chore(ci): add Node.js v20 to CI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit chore(ci): add Node.js 20 support for CI - Add runtime check for profiling limitation on Node.js 20 (--cpu-prof not allowed in NODE_OPTIONS for security reasons) - Show helpful error message directing users to Node.js 22+ - Fix run-tests-v20.js to use process.execPath instead of 'node' - Add isNode20 helper to test/util.ts for version-specific test skipping - Split profiling tests into Node 20 and Node 22+ groups 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude --- .github/workflows/nodejs.yml | 6 +- scripts/run-tests-v20.js | 24 +++ src/services/profiler/profile-runner.ts | 15 ++ test/integration/profile-command.test.ts | 215 ++++++++++++++--------- test/unit/profile-runner.test.ts | 173 ++++++++++-------- test/util.ts | 13 ++ 6 files changed, 290 insertions(+), 156 deletions(-) create mode 100644 scripts/run-tests-v20.js diff --git a/.github/workflows/nodejs.yml b/.github/workflows/nodejs.yml index 6827d4a..aba431b 100644 --- a/.github/workflows/nodejs.yml +++ b/.github/workflows/nodejs.yml @@ -11,7 +11,7 @@ jobs: strategy: matrix: include: - # - node-version: '20' + - node-version: '20' - node-version: '22' - node-version: '24' steps: @@ -20,5 +20,9 @@ jobs: - uses: ./.github/actions/prepare with: node-version: ${{ matrix.node-version }} + - name: Test (Node 20) + if: matrix.node-version == '20' + run: node scripts/run-tests-v20.js 'test/**/*.test.ts' - name: Test + if: matrix.node-version != '20' run: npm test diff --git a/scripts/run-tests-v20.js b/scripts/run-tests-v20.js new file mode 100644 index 0000000..818041b --- /dev/null +++ b/scripts/run-tests-v20.js @@ -0,0 +1,24 @@ +#!/usr/bin/env node +import { globSync } from 'glob'; +/** + * Test runner for Node.js v20 + * + * Node.js v20's `node --test` command doesn't support glob patterns. This + * script uses the `glob` package to find test files and passes them to `node + * --test`. + */ +import { execFileSync } from 'node:child_process'; + +const pattern = process.argv[2] || 'test/**/*.test.ts'; +const files = globSync(pattern); + +if (files.length === 0) { + console.error(`No files matched pattern: ${pattern}`); + process.exit(1); +} + +execFileSync( + process.execPath, + ['--import', 'tsx', '--test', '--test-reporter=spec', ...files], + { stdio: 'inherit' }, +); diff --git a/src/services/profiler/profile-runner.ts b/src/services/profiler/profile-runner.ts index b50da70..96d46aa 100644 --- a/src/services/profiler/profile-runner.ts +++ b/src/services/profiler/profile-runner.ts @@ -26,17 +26,32 @@ interface RunOptions { timeout?: number; } +/** + * Node.js major version number + */ +const nodeMajorVersion = Number(process.versions.node.split('.')[0]); + /** * Run a command with Node.js profiling enabled * * @param command - Command to run (e.g., "npm test") * @param options - Execution options * @returns Path to generated *.cpuprofile file + * @throws Error if running on Node.js 20 (--cpu-prof not allowed in + * NODE_OPTIONS) */ export const runWithProfiling = async ( command: string, options: RunOptions = {}, ): Promise => { + // Node.js 20 blocks --cpu-prof in NODE_OPTIONS for security reasons + if (nodeMajorVersion === 20) { + throw new Error( + 'CPU profiling requires Node.js 22 or later. ' + + 'Node.js 20 blocks --cpu-prof in NODE_OPTIONS for security reasons.', + ); + } + const cwd = options.cwd || process.cwd(); // Create profiles directory diff --git a/test/integration/profile-command.test.ts b/test/integration/profile-command.test.ts index 0a388d7..c23378b 100644 --- a/test/integration/profile-command.test.ts +++ b/test/integration/profile-command.test.ts @@ -1,99 +1,152 @@ import assert from 'node:assert/strict'; -import { execSync } from 'node:child_process'; +import { execSync, spawnSync } from 'node:child_process'; import { mkdtemp, rm, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { dirname, join } from 'node:path'; import { describe, it } from 'node:test'; import { fileURLToPath } from 'node:url'; +import { isNode20 } from '../util.js'; import { fixtures } from './fixture-paths.js'; const __filename = fileURLToPath(import.meta.url); const __dirname = dirname(__filename); +// Node.js v20 doesn't allow --cpu-prof in NODE_OPTIONS for security reasons +// Tests are split into two groups: Node 20 tests and Node 22+ tests +const node20Test = isNode20 ? it : it.skip; +const node22PlusTest = isNode20 ? it.skip : it; + describe('Profile Command Integration', () => { - it('should profile a simple script and generate report', async () => { - const tmpDir = await mkdtemp(join(tmpdir(), 'modestbench-profile-')); - - try { - // Create package.json - await writeFile( - join(tmpDir, 'package.json'), - JSON.stringify({ name: 'test-project' }), - ); - - // Run profile command using fixture - const output = execSync( - `node ${join(__dirname, '../../dist/cli/index.js')} analyze "node ${fixtures.profileHotFunction}"`, - { - cwd: tmpDir, - encoding: 'utf-8', - }, - ); - - // Verify output contains expected sections - assert.ok(output.includes('Profile Analysis'), 'Should contain header'); - assert.ok( - output.includes('Benchmark Candidates'), - 'Should contain candidates section', - ); - assert.ok( - output.includes('hotFunction') || output.includes('ticks'), - 'Should contain function data', - ); - } finally { - await rm(tmpDir, { force: true, recursive: true }); - } - }); + describe('on Node.js 20', () => { + node20Test( + 'should show helpful error when profiling is unavailable', + async () => { + const tmpDir = await mkdtemp(join(tmpdir(), 'modestbench-profile-')); + + try { + await writeFile( + join(tmpDir, 'package.json'), + JSON.stringify({ name: 'test-project' }), + ); + + // Use process.execPath to ensure we run with the same Node version + const result = spawnSync( + process.execPath, + [ + join(__dirname, '../../dist/cli/index.js'), + 'analyze', + `node ${fixtures.profileHotFunction}`, + ], + { + cwd: tmpDir, + encoding: 'utf-8', + }, + ); - it('should support --group-by-file option', async () => { - const tmpDir = await mkdtemp(join(tmpdir(), 'modestbench-profile-')); - - try { - await writeFile( - join(tmpDir, 'package.json'), - JSON.stringify({ name: 'test-project' }), - ); - - const output = execSync( - `node ${join(__dirname, '../../dist/cli/index.js')} analyze "node ${fixtures.profileMultiFunction}" --group-by-file`, - { - cwd: tmpDir, - encoding: 'utf-8', - }, - ); - - assert.ok( - output.includes('Grouped by File'), - 'Should contain grouped by file header', - ); - } finally { - await rm(tmpDir, { force: true, recursive: true }); - } + assert.notEqual(result.status, 0, 'Should exit with non-zero code'); + assert.ok( + result.stderr.includes('Node.js 22 or later') || + result.stderr.includes('CPU profiling requires'), + 'Should show helpful error message about Node.js version', + ); + } finally { + await rm(tmpDir, { force: true, recursive: true }); + } + }, + ); }); - it('should respect --min-percent threshold', async () => { - const tmpDir = await mkdtemp(join(tmpdir(), 'modestbench-profile-')); - - try { - await writeFile( - join(tmpDir, 'package.json'), - JSON.stringify({ name: 'test-project' }), - ); - - // Run with high threshold - should filter out most functions - const output = execSync( - `node ${join(__dirname, '../../dist/cli/index.js')} analyze "node ${fixtures.profileHotFunction}" --min-percent 50`, - { - cwd: tmpDir, - encoding: 'utf-8', - }, - ); - - // Should still have header but fewer functions - assert.ok(output.includes('Profile Analysis'), 'Should have header'); - } finally { - await rm(tmpDir, { force: true, recursive: true }); - } + describe('on Node.js 22+', () => { + node22PlusTest( + 'should profile a simple script and generate report', + async () => { + const tmpDir = await mkdtemp(join(tmpdir(), 'modestbench-profile-')); + + try { + // Create package.json + await writeFile( + join(tmpDir, 'package.json'), + JSON.stringify({ name: 'test-project' }), + ); + + // Run profile command using fixture + const output = execSync( + `node ${join(__dirname, '../../dist/cli/index.js')} analyze "node ${fixtures.profileHotFunction}"`, + { + cwd: tmpDir, + encoding: 'utf-8', + }, + ); + + // Verify output contains expected sections + assert.ok( + output.includes('Profile Analysis'), + 'Should contain header', + ); + assert.ok( + output.includes('Benchmark Candidates'), + 'Should contain candidates section', + ); + assert.ok( + output.includes('hotFunction') || output.includes('ticks'), + 'Should contain function data', + ); + } finally { + await rm(tmpDir, { force: true, recursive: true }); + } + }, + ); + + node22PlusTest('should support --group-by-file option', async () => { + const tmpDir = await mkdtemp(join(tmpdir(), 'modestbench-profile-')); + + try { + await writeFile( + join(tmpDir, 'package.json'), + JSON.stringify({ name: 'test-project' }), + ); + + const output = execSync( + `node ${join(__dirname, '../../dist/cli/index.js')} analyze "node ${fixtures.profileMultiFunction}" --group-by-file`, + { + cwd: tmpDir, + encoding: 'utf-8', + }, + ); + + assert.ok( + output.includes('Grouped by File'), + 'Should contain grouped by file header', + ); + } finally { + await rm(tmpDir, { force: true, recursive: true }); + } + }); + + node22PlusTest('should respect --min-percent threshold', async () => { + const tmpDir = await mkdtemp(join(tmpdir(), 'modestbench-profile-')); + + try { + await writeFile( + join(tmpDir, 'package.json'), + JSON.stringify({ name: 'test-project' }), + ); + + // Run with high threshold - should filter out most functions + const output = execSync( + `node ${join(__dirname, '../../dist/cli/index.js')} analyze "node ${fixtures.profileHotFunction}" --min-percent 50`, + { + cwd: tmpDir, + encoding: 'utf-8', + }, + ); + + // Should still have header but fewer functions + assert.ok(output.includes('Profile Analysis'), 'Should have header'); + } finally { + await rm(tmpDir, { force: true, recursive: true }); + } + }); }); }); diff --git a/test/unit/profile-runner.test.ts b/test/unit/profile-runner.test.ts index 651568a..8df0047 100644 --- a/test/unit/profile-runner.test.ts +++ b/test/unit/profile-runner.test.ts @@ -1,93 +1,118 @@ -import { expect, expectAsync } from 'bupkis'; -import { mkdtemp, rm } from 'node:fs/promises'; +import { expectAsync } from 'bupkis'; +import { mkdtemp, rm, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { describe, it } from 'node:test'; import { runWithProfiling } from '../../src/services/profiler/profile-runner.js'; +import { isNode20 } from '../util.js'; + +// Node.js v20 doesn't allow --cpu-prof in NODE_OPTIONS for security reasons +// Tests are split into two groups: Node 20 tests and Node 22+ tests +const node20Test = isNode20 ? it : it.skip; +const node22PlusTest = isNode20 ? it.skip : it; describe('ProfileRunner', () => { - it('should run command with --cpu-prof flag and return profile path', async () => { - const tmpDir = await mkdtemp(join(tmpdir(), 'modestbench-test-')); + describe('on Node.js 20', () => { + node20Test( + 'should throw error explaining profiling limitation', + async () => { + await expectAsync( + runWithProfiling('node --eval "console.log(1)"'), + 'to reject with error satisfying', + /CPU profiling requires Node\.js 22 or later/, + ); + }, + ); + }); - try { - // Run a simple command that will generate a profile - const profilePath = await runWithProfiling( - 'node --eval "console.log(1)"', - { - cwd: tmpDir, - }, - ); + describe('on Node.js 22+', () => { + node22PlusTest( + 'should run command with --cpu-prof flag and return profile path', + async () => { + const { expect } = await import('bupkis'); + const tmpDir = await mkdtemp(join(tmpdir(), 'modestbench-test-')); - expect(profilePath, 'to be a string'); - expect(profilePath, 'to match', /\.cpuprofile$/); - expect(profilePath, 'to contain', '.modestbench/profiles'); - expect(profilePath, 'to start with', tmpDir); - } finally { - await rm(tmpDir, { force: true, recursive: true }); - } - }); + try { + // Run a simple command that will generate a profile + const profilePath = await runWithProfiling( + 'node --eval "console.log(1)"', + { + cwd: tmpDir, + }, + ); - it('should handle command failure', async () => { - const tmpDir = await mkdtemp(join(tmpdir(), 'modestbench-test-')); + expect(profilePath, 'to be a string'); + expect(profilePath, 'to match', /\.cpuprofile$/); + expect(profilePath, 'to contain', '.modestbench/profiles'); + expect(profilePath, 'to start with', tmpDir); + } finally { + await rm(tmpDir, { force: true, recursive: true }); + } + }, + ); - try { - await expectAsync( - runWithProfiling('node --eval "process.exit(1)"', { - cwd: tmpDir, - }), - 'to reject with error satisfying', - /Profile command exited with code 1/, - ); - } finally { - await rm(tmpDir, { force: true, recursive: true }); - } - }); + node22PlusTest('should handle command failure', async () => { + const tmpDir = await mkdtemp(join(tmpdir(), 'modestbench-test-')); - it('should respect timeout option', async () => { - const tmpDir = await mkdtemp(join(tmpdir(), 'modestbench-test-')); + try { + await expectAsync( + runWithProfiling('node --eval "process.exit(1)"', { + cwd: tmpDir, + }), + 'to reject with error satisfying', + /Profile command exited with code 1/, + ); + } finally { + await rm(tmpDir, { force: true, recursive: true }); + } + }); - try { - await expectAsync( - runWithProfiling('node --eval "setTimeout(() => {}, 10000)"', { - cwd: tmpDir, - timeout: 100, - }), - 'to reject with error satisfying', - /Profile command timed out/, - ); - } finally { - await rm(tmpDir, { force: true, recursive: true }); - } - }); + node22PlusTest('should respect timeout option', async () => { + const tmpDir = await mkdtemp(join(tmpdir(), 'modestbench-test-')); - it('should profile npm commands', async () => { - const tmpDir = await mkdtemp(join(tmpdir(), 'modestbench-test-')); + try { + await expectAsync( + runWithProfiling('node --eval "setTimeout(() => {}, 10000)"', { + cwd: tmpDir, + timeout: 100, + }), + 'to reject with error satisfying', + /Profile command timed out/, + ); + } finally { + await rm(tmpDir, { force: true, recursive: true }); + } + }); - try { - // Create a simple package.json with a test script - const { writeFile } = await import('node:fs/promises'); - await writeFile( - join(tmpDir, 'package.json'), - JSON.stringify({ - name: 'test', - scripts: { - test: 'node --eval "console.log(1)"', - }, - }), - ); + node22PlusTest('should profile npm commands', async () => { + const { expect } = await import('bupkis'); + const tmpDir = await mkdtemp(join(tmpdir(), 'modestbench-test-')); - // Run npm test with profiling - const profilePath = await runWithProfiling('npm test', { - cwd: tmpDir, - }); + try { + // Create a simple package.json with a test script + await writeFile( + join(tmpDir, 'package.json'), + JSON.stringify({ + name: 'test', + scripts: { + test: 'node --eval "console.log(1)"', + }, + }), + ); + + // Run npm test with profiling + const profilePath = await runWithProfiling('npm test', { + cwd: tmpDir, + }); - expect(profilePath, 'to be a string'); - expect(profilePath, 'to match', /\.cpuprofile$/); - expect(profilePath, 'to contain', '.modestbench/profiles'); - expect(profilePath, 'to start with', tmpDir); - } finally { - await rm(tmpDir, { force: true, recursive: true }); - } + expect(profilePath, 'to be a string'); + expect(profilePath, 'to match', /\.cpuprofile$/); + expect(profilePath, 'to contain', '.modestbench/profiles'); + expect(profilePath, 'to start with', tmpDir); + } finally { + await rm(tmpDir, { force: true, recursive: true }); + } + }); }); }); diff --git a/test/util.ts b/test/util.ts index 9f9838c..4475695 100644 --- a/test/util.ts +++ b/test/util.ts @@ -7,6 +7,19 @@ import { fileURLToPath } from 'node:url'; // Use compiled dist files instead of tsx for faster test execution const cliPath = fileURLToPath(new URL('../dist/cli/index.js', import.meta.url)); +/** + * Node.js major version number + */ +export const nodeMajorVersion = Number(process.versions.node.split('.')[0]); + +/** + * Whether we're running on Node.js v20 + * + * Useful for skipping tests that use features not available in v20, such as + * `--cpu-prof` in `NODE_OPTIONS` (blocked for security reasons in v20). + */ +export const isNode20 = nodeMajorVersion === 20; + /** * Helper function to run CLI commands and capture output */ From be99f09e3a52675ad114073811a535591424b503 Mon Sep 17 00:00:00 2001 From: Christopher Hiller Date: Fri, 9 Jan 2026 13:30:57 -0800 Subject: [PATCH 2/3] chore(ci): tweak concurrency in builds --- .github/workflows/lint.yml | 4 ++++ .github/workflows/nodejs.yml | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index dabcc4b..8c38c94 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -1,6 +1,10 @@ permissions: contents: read +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + jobs: lint: env: diff --git a/.github/workflows/nodejs.yml b/.github/workflows/nodejs.yml index aba431b..3871322 100644 --- a/.github/workflows/nodejs.yml +++ b/.github/workflows/nodejs.yml @@ -2,6 +2,10 @@ name: Node CI on: [push, pull_request] +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + jobs: test: env: From 7433789556cae7ccf6a4edf3eb147986ff499bde Mon Sep 17 00:00:00 2001 From: Christopher Hiller Date: Fri, 9 Jan 2026 14:21:18 -0800 Subject: [PATCH 3/3] chore(scripts): move JSDoc comment after all imports in run-tests-v20.js MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per review feedback, moved the JSDoc module documentation to come after all import statements. Note: kept import ordering per project's perfectionist ESLint config (alphabetical), not the Node.js builtin-first convention suggested by the reviewer. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- scripts/run-tests-v20.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/scripts/run-tests-v20.js b/scripts/run-tests-v20.js index 818041b..1de5087 100644 --- a/scripts/run-tests-v20.js +++ b/scripts/run-tests-v20.js @@ -1,5 +1,7 @@ #!/usr/bin/env node import { globSync } from 'glob'; +import { execFileSync } from 'node:child_process'; + /** * Test runner for Node.js v20 * @@ -7,7 +9,6 @@ import { globSync } from 'glob'; * script uses the `glob` package to find test files and passes them to `node * --test`. */ -import { execFileSync } from 'node:child_process'; const pattern = process.argv[2] || 'test/**/*.test.ts'; const files = globSync(pattern);