Repository navigation
fix(tools): respect .only in run-test runner across Node.js and Bun - #9563
danieljbruce wants to merge 4 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for .only test execution in bin/run-test.cjs by detecting exclusive tests, narrowing down target files, and disabling parallel execution when .only is present. It also adds corresponding unit tests and improves package binary resolution. Feedback on these changes suggests dynamically detecting the active package manager instead of hardcoding npm when compiling stale source files, and wrapping the pnpm fallback resolution logic in a try...catch block to avoid masking the original resolution error.
| if (pkg.scripts && pkg.scripts.compile) { | ||
| const compileRes = spawnSync('npm', ['run', 'compile'], { | ||
| cwd, | ||
| stdio: 'inherit', | ||
| env: {...process.env, RUN_TEST_SKIP_COMPILE: 'true'}, | ||
| }); |
There was a problem hiding this comment.
When invoking the compilation script, it is better to use the active package manager (e.g., pnpm, bun, or yarn) that initiated the test run, rather than hardcoding npm. This ensures that workspace links, lockfiles, and speed optimizations of the active package manager are preserved.
if (pkg.scripts && pkg.scripts.compile) {
const userAgent = process.env.npm_config_user_agent || '';
const pm = userAgent.startsWith('pnpm/')
? 'pnpm'
: userAgent.startsWith('bun/')
? 'bun'
: userAgent.startsWith('yarn/')
? 'yarn'
: 'npm';
const compileRes = spawnSync(pm, ['run', 'compile'], {
cwd,
stdio: 'inherit',
env: {...process.env, RUN_TEST_SKIP_COMPILE: 'true'},
});
| } catch (err) { | ||
| const pkgName = pkgBin.split('/')[0]; | ||
| const pnpmDir = path.join(repoRoot, 'node_modules', '.pnpm'); | ||
| if (fs.existsSync(pnpmDir)) { | ||
| const entry = fs | ||
| .readdirSync(pnpmDir) | ||
| .find(d => d.startsWith(`${pkgName}@`)); | ||
| if (entry) { | ||
| const pnpmCandidate = path.join(pnpmDir, entry, 'node_modules', pkgBin); | ||
| if (fs.existsSync(pnpmCandidate)) { | ||
| return pnpmCandidate; | ||
| } | ||
| } | ||
| } | ||
| throw err; | ||
| } |
There was a problem hiding this comment.
If any error occurs during the pnpm fallback resolution (such as fs.readdirSync throwing due to permission issues or missing directories), it will mask the original require.resolve error. Wrapping the fallback logic in a try...catch block ensures that we gracefully fall back to throwing the original require.resolve error.
} catch (err) {
try {
const pkgName = pkgBin.split('/')[0];
const pnpmDir = path.join(repoRoot, 'node_modules', '.pnpm');
if (fs.existsSync(pnpmDir)) {
const entry = fs
.readdirSync(pnpmDir)
.find(d => d.startsWith(pkgName + '@'));
if (entry) {
const pnpmCandidate = path.join(pnpmDir, entry, 'node_modules', pkgBin);
if (fs.existsSync(pnpmCandidate)) {
return pnpmCandidate;
}
}
}
} catch {}
throw err;
}
… Mocha flags, and stale TypeScript compilation
…ckages and check file stems on stale compile
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for running exclusive tests (using .only) in the test runner script bin/run-test.cjs, along with a comprehensive test suite in .github/scripts/tests/run-test.test.cjs. It parses Mocha arguments, detects .only calls while ignoring comments/strings, automatically compiles stale TypeScript source files, and disables parallel execution when running exclusive tests. The review feedback is highly constructive and identifies several critical areas for improvement: handling Windows platform compatibility and alternative package managers when spawning the compile script, using relative paths in getTestStem to prevent name collisions, avoiding call stack limits by replacing the spread operator with reduce for file arrays, filtering out parallel flags that contain values, and correctly resolving scoped packages in pnpm's virtual store.
| const compileRes = spawnSync('npm', ['run', 'compile'], { | ||
| cwd, | ||
| stdio: 'inherit', | ||
| env: {...process.env, RUN_TEST_SKIP_COMPILE: 'true'}, | ||
| }); |
There was a problem hiding this comment.
On Windows, spawning shell commands like npm directly without shell: true or appending .cmd will throw an ENOENT error. Additionally, in monorepos using pnpm or yarn, running npm run compile can fail or bypass workspace configurations. We should detect the active package manager from process.env.npm_config_user_agent and append .cmd on Windows for non-Bun package managers.
const userAgent = process.env.npm_config_user_agent || '';
let pkgManager = 'npm';
if (userAgent.startsWith('pnpm/')) {
pkgManager = 'pnpm';
} else if (userAgent.startsWith('yarn/')) {
pkgManager = 'yarn';
} else if (userAgent.startsWith('bun/')) {
pkgManager = 'bun';
}
const cmd = process.platform === 'win32' && pkgManager !== 'bun' ? `${pkgManager}.cmd` : pkgManager;
const compileRes = spawnSync(cmd, ['run', 'compile'], {
cwd,
stdio: 'inherit',
env: {...process.env, RUN_TEST_SKIP_COMPILE: 'true'},
});
| function getTestStem(filePath) { | ||
| return path.basename(filePath).replace(/\.[cm]?[jt]s$/, ''); | ||
| } |
There was a problem hiding this comment.
Comparing only the base name of files can lead to false positives or negatives if there are same-named test files in different directories (e.g., test/unit/foo.ts vs test/integration/foo.ts). Using the relative path from the package root (with the build/ prefix stripped) is much more robust.
function getTestStem(filePath, cwd = process.cwd()) {
const rel = path.relative(cwd, filePath).replace(/\\/g, '/');
const cleanRel = rel.replace(/^build\/(?:cjs\/|esm\/)?/, '');
return cleanRel.replace(/\.[cm]?[jt]s$/, '');
}
| const onlyStems = onlyFiles.map(getTestStem).sort().join('\0'); | ||
| const sourceOnlyStems = sourceOnlyFiles.map(getTestStem).sort().join('\0'); |
There was a problem hiding this comment.
Pass the cwd parameter to getTestStem to ensure relative paths are resolved correctly against the package directory.
const onlyStems = onlyFiles.map(f => getTestStem(f, cwd)).sort().join('\\0');
const sourceOnlyStems = sourceOnlyFiles.map(f => getTestStem(f, cwd)).sort().join('\\0');
| const maxSourceOnlyMtime = | ||
| sourceOnlyFiles.length > 0 | ||
| ? Math.max(...sourceOnlyFiles.map(f => fs.statSync(f).mtimeMs)) | ||
| : 0; | ||
| const minTargetMtime = | ||
| onlyFiles.length > 0 | ||
| ? Math.min(...onlyFiles.map(f => fs.statSync(f).mtimeMs)) | ||
| : 0; |
There was a problem hiding this comment.
Using the spread operator (...) on arrays of dynamic/unknown length (like lists of test files) can cause a "Maximum call stack size exceeded" error if the array is very large. Using Array.prototype.reduce is safer and more robust.
const maxSourceOnlyMtime = sourceOnlyFiles.reduce(
(max, f) => Math.max(max, fs.statSync(f).mtimeMs),
0,
);
const minTargetMtime = onlyFiles.reduce(
(min, f) => Math.min(min, fs.statSync(f).mtimeMs),
Infinity,
);
| const filteredOptions = optionArgs.filter( | ||
| a => a !== '--parallel' && a !== '-p', | ||
| ); |
There was a problem hiding this comment.
Users might pass parallel flags with values (e.g., --parallel=true or -p=true). We should filter these out as well to ensure parallel mode is completely disabled when .only is detected.
const filteredOptions = optionArgs.filter(
a => a !== '--parallel' && a !== '-p' && !a.startsWith('--parallel=') && !a.startsWith('-p='),
);
| const pkgName = pkgBin.split('/')[0]; | ||
| const pnpmDir = path.join(repoRoot, 'node_modules', '.pnpm'); | ||
| if (fs.existsSync(pnpmDir)) { | ||
| const entry = fs | ||
| .readdirSync(pnpmDir) | ||
| .find(d => d.startsWith(`${pkgName}@`)); |
There was a problem hiding this comment.
In pnpm, scoped packages are stored in .pnpm with a folder name format of @scope+name@version. The current logic only splits by / and takes the first part, which would be @scope, failing to match the actual folder name. We should handle scoped packages by joining the scope and name with +.
let pkgName = pkgBin.split('/')[0];
if (pkgName.startsWith('@') && pkgBin.split('/').length > 1) {
pkgName = `${pkgName}+${pkgBin.split('/')[1]}`;
}
const pnpmDir = path.join(repoRoot, 'node_modules', '.pnpm');
if (fs.existsSync(pnpmDir)) {
const entry = fs
.readdirSync(pnpmDir)
.find(d => d.startsWith(`${pkgName}@`));
This branch has not been deployed
1 participant
Description
Updates
bin/run-test.cjsso that exclusive Mocha tests (it.only,describe.only,context.only,suite.only,test.only) work out of the box and execute only the exclusive test(s) across both Node.js and Bun.Impact
Previously, adding
.onlyto a unit test caused Mocha to fail withUncaught Error: `.only` is not supported in parallel modewhenever.mocharc.cjs(parallel: true) or--parallelwas active, and runningbun run test(without--bunorJS_RUNTIME=bun) fell back to Node.js parallel execution instead of Bun. In addition, passing an entire test directory when only a single file contained.onlycaused Mocha to load every unrelated test file in the suite. With this change,.onlyis automatically detected, parallel mode is disabled, and execution is narrowed to only the test file(s) containing.only.Changes
bun runinvocations vianpm_config_user_agentinbin/run-test.cjssobun run testautomatically runs under Bun unlessJS_RUNTIME=nodeis explicitly set..only(calls (it.only,describe.only,context.only,suite.only,test.only)..tstest file contains.onlyand is newer than the compiled output inbuild/, automatically run the package'scompilescript before resolving.onlyfiles..onlyis present in one or more test files, setMOCHA_PARALLEL=false, strip--parallel/-p, pass--no-parallel, and narrow the positional test file arguments to only the file(s) containing.only..github/scripts/tests/run-test.test.cjscovering comment/string stripping, argument narrowing, and end-to-end.onlyexecution under both Node.js and Bun.Testing
.github/scripts/tests/run-test.test.cjsverifying.onlydetection and end-to-end execution under both Node.js and Bun..onlyexecution against package test suites with--config ../../.mocharc.cjsacrosspnpm test,bun run test, andbun --bun run test.Alternatives
bin/run-test.cjsor disablingparallel: trueglobally in.mocharc.cjs, but disabling parallelism globally would significantly slow down full unit test suites under Node.js, whereas dynamically disabling parallel mode and narrowing target files only when.onlyis detected preserves full-suite parallelism while making.onlyfast and seamless..onlyto isolate tests when debugging underbin/run-test.cjs.Fixes b/571064199