Skip to content

fix(tests): resolve test-file globs in-process, dropping the ripgrep path #3792

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kwakayama merged 3 commits into from
Aug 17, 2026
Merged

fix(tests): resolve test-file globs in-process, dropping the ripgrep path #3792

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0baea5d
fix(tests): resolve test-file globs in-process only, drop the rg path
kwakayama Aug 16, 2026
131e8d3
fix(tests): prune node_modules in the walk and correct the ignore-tra…
kwakayama Aug 16, 2026
7cb36c8
fix(tests): translate bracket character classes instead of escaping them
kwakayama Aug 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
177 changes: 89 additions & 88 deletions tests/test-file-utils.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,35 @@
import { spawnSync } from "node:child_process";
/**
* Test-file selection.
*
* Patterns are resolved entirely in-process. There is deliberately no ripgrep
* path and no other external resolver — see #3784. `listTestFiles` used to
* shell out to `rg` and walk the tree itself only when `rg` was absent, which
* made the selected set depend on which binaries a machine happened to have.
* #3780 fixed a run of divergences between the two across six review rounds,
* most of them introduced by the fix for the previous one. Exact parity with
* `rg` is not reachable by inspection, and it was never the goal. The goal is
* that the same tests run everywhere; one implementation is how that is met.
*
* The trade is `.gitignore` awareness, which `rg` provided for free. It was
* measured before being given up: with `npm/` built by `deno task build:npm`,
* every gitignored `*.test.*` file in the tree lives under `node_modules/` or
* `npm/node_modules/`, and no pattern any consumer passes reaches either. The
* `rg` (15.2.0) and in-process sets were identical — 1598, 1729 and 1927 files
* for the `test:node`, `test:bun` and Bun-default pattern sets respectively.
*
* `node:fs` `globSync` was evaluated as a third option and rejected: it is
* three implementations, not one. On the same fixtures, Node 25 and Deno 2.7
* return `src` itself for the pattern `src` + globstar while Bun 1.3 does not,
* and Node and Deno match through a hidden directory named in a pattern's
* literal prefix while Bun returns nothing. All three runtimes load this
* module, so that would restore the divergence class removing `rg` closes.
*/
import { readdirSync, statSync } from "node:fs";
import { dirname, relative, resolve, sep } from "node:path";

const TEST_FILE_RE = /\.test\.[cm]?[jt]sx?$/i;
const GLOB_CHARS_RE = /[\*\?\[]/;

let rgAvailable = true;

function toPosixPath(path) {
return path.split(sep).join("/");
}
Expand All @@ -15,22 +38,6 @@ function hasGlob(pattern) {
return GLOB_CHARS_RE.test(pattern);
}

function runRg(args, cwd) {
if (!rgAvailable) return null;
const result = spawnSync("rg", args, { cwd, encoding: "utf8" });
if (result.error) {
if (result.error.code === "ENOENT") {
rgAvailable = false;
}
return null;
}
if (result.status !== 0 && result.status !== 1) return null;
return result.stdout
.split(/\r?\n/)
.map((line) => line.trim())
.filter(Boolean);
}

function globToRegex(glob) {
let re = "^";
for (let i = 0; i < glob.length; i += 1) {
Expand Down Expand Up @@ -69,7 +76,30 @@ function globToRegex(glob) {
re += "[^/]";
continue;
}
if ("+^$.()|{}[]\\".includes(char)) {
if (char === "[") {
// `hasGlob` counts `[` as a glob character, so escaping the brackets here
// made every bracket pattern match nothing — and an empty selection is a
// silently passing test run, not an error. `rg -g 'src/[ab].test.ts'`
// returns a.test.ts and b.test.ts, so the class is translated instead.
const closing = glob.indexOf("]", char === "[" && glob[i + 1] === "]" ? i + 2 : i + 1);
if (closing !== -1) {
let body = glob.slice(i + 1, closing);
// `!` is the glob spelling of a negated class; `^` is the regex one.
const negated = body.startsWith("!") || body.startsWith("^");
if (negated) body = body.slice(1);
// A literal `]` first, a literal `-` last, and `\` all need escaping so
// the class cannot break out into the surrounding expression.
const escaped = body.replace(/\\/g, "\\\\").replace(/\]/g, "\\]");
// Never let a class match a separator: a glob segment cannot span one.
re += negated ? `[^/${escaped}]` : `[${escaped}]`;
i = closing;
continue;
}
// Unterminated `[` is a literal bracket, which is what rg does too.
re += "\\[";
continue;
}
if ("+^$.()|{}]\\".includes(char)) {
re += `\\${char}`;
continue;
}
Expand All @@ -85,14 +115,21 @@ function globToRegex(glob) {
function walk(dir, onFile) {
const entries = readdirSync(dir, { withFileTypes: true });
for (const entry of entries) {
// `rg` is the reference here, because `rg` is what runs when it is
// installed; the fallback exists to reproduce its selection when it is
// not. It treats the two cases differently, verified directly:
// Hidden *directories* are pruned; dot-prefixed *files* are not:
// hidden directory -> skipped (src/.fixtures/x.test.ts is omitted)
// dot-prefixed file -> INCLUDED (src/.smoke.test.ts is returned)
// because `-g/--glob` "always overrides any other ignore logic". Note
// node:fs glob excludes both, so it is the wrong oracle for dot-files.
if (entry.isDirectory() && entry.name.startsWith(".")) continue;
// Inherited from the `rg` behaviour this module used to have to match, and
// kept on purpose so removing `rg` changes no selection. `node:fs` glob
// excludes both, which is one more reason it is not a drop-in here.
// `node_modules` is pruned for the same reason as hidden directories: the
// resolver no longer consults `.gitignore` (see the module doc), and this
// is where every gitignored `*.test.*` in the tree lives — 498 of them,
// under `npm/node_modules/` and `node_modules/.deno/`. Nothing under
// `src/`, `tests/` or `proxy/` contains a `node_modules`, so this cannot
// change what the four consumers select; it makes the guarantee explicit
// rather than incidental, and covers `run-affected-tests.mjs`, which can
// pass the repo root as a directory pattern when a root-level file changes.
if (entry.isDirectory() && isPrunedDirectoryName(entry.name)) continue;
const fullPath = resolve(dir, entry.name);
if (entry.isDirectory()) {
walk(fullPath, onFile);
Expand Down Expand Up @@ -130,6 +167,10 @@ function isMissingPathError(error) {
* Only segments below `cwd` count: a checkout that itself lives under a hidden
* directory is not thereby invisible to its own test runner.
*/
function isPrunedDirectoryName(name) {
return name.startsWith(".") || name === "node_modules";
}

function hasHiddenSegment(target, cwd) {
const relativePath = relative(cwd, target);
// `startsWith("..")` alone would misread a directory *named* `..fixtures` as
Expand All @@ -138,18 +179,30 @@ function hasHiddenSegment(target, cwd) {
const escapesCwd = relativePath === ".." ||
relativePath.startsWith(`..${sep}`) || relativePath.startsWith("../");
if (relativePath === "" || escapesCwd) return false;
return toPosixPath(relativePath).split("/").some((segment) => segment.startsWith("."));
// Covers the base itself, not just its children: `walk` starts *inside* the
// base, so a base that is a hidden directory or `node_modules` would be
// traversed in full while every equivalent child was pruned.
return toPosixPath(relativePath).split("/").some(isPrunedDirectoryName);
}

function listWithFallback(patterns, cwd) {
/**
* Resolve `patterns` — literal files, directories and globs — to absolute test
* file paths.
*
* @param {string[]} patterns
* @param {string} [cwd]
* @returns {string[]}
*/
export function listTestFiles(patterns, cwd = process.cwd()) {

@chatgpt-codex-connector chatgpt-codex-connector Bot Aug 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve bracket-class glob matching

Making the in-process resolver unconditional regresses patterns containing character classes. hasGlob() recognizes [, but globToRegex() escapes both brackets, so a quoted pattern such as src/[ab].test.ts now selects nothing instead of matching a.test.ts and b.test.ts as the former ripgrep path did. When passed to the Node test runner, this empty selection exits successfully without running the requested tests, producing a false green result.

Useful? React with 👍 / 👎.

const files = new Set();
for (const pattern of patterns) {
if (!pattern) continue;
const absolute = resolve(cwd, pattern);
if (!hasGlob(pattern)) {
// Base lookup guarded; the traversal is not, for the same reason as
// `listTestFiles` — a descendant vanishing mid-walk must not be read as
// "this path does not exist".
// Only the base lookup is guarded. Wrapping the traversal too would
// swallow an `ENOENT` raised *inside* `walk` — a descendant removed
// between `readdirSync` calls — and silently drop the directory's whole
// contribution.
let stats;
try {
stats = statSync(absolute);
Expand All @@ -168,10 +221,11 @@ function listWithFallback(patterns, cwd) {
}

const baseDir = getBaseDir(pattern, cwd);
// `rg` prunes hidden directories before the glob is applied, so a pattern
// whose literal prefix descends into one matches nothing at all — verified
// with rg 15: `-g 'src/.fixtures/**/*.test.ts'` returns no files. `walk`
// starts *inside* the base, so the per-entry hidden check never sees it.
// A pattern whose literal prefix descends into a hidden directory matches
// nothing at all, which is what `walk`'s per-entry pruning means applied to
// the base itself — `walk` starts *inside* the base, so the per-entry check
// never sees it. Same as `rg -g 'src/.fixtures/**/*.test.ts'`, verified
// against rg 15.
if (hasHiddenSegment(baseDir, cwd)) continue;
// A glob whose base does not exist contributes nothing, the same as the
// non-glob branch above. This is checked up front rather than by catching
Expand All @@ -196,59 +250,6 @@ function listWithFallback(patterns, cwd) {
return Array.from(files);
}

export function listTestFiles(patterns, cwd = process.cwd()) {
const files = new Set();
for (const pattern of patterns) {
if (!pattern) continue;
const absolute = resolve(cwd, pattern);

if (hasGlob(pattern)) {
const matches = runRg(["--files", "-g", pattern], cwd);
if (matches) {
for (const match of matches) files.add(resolve(cwd, match));
} else {
// No ripgrep (it is absent on the CI runners), so resolve the glob
// in-process. This fallback has to be per-pattern: the whole-result
// fallback at the end only fires when *nothing* matched, so a single
// explicit file listed next to a glob was enough to suppress it and
// drop the glob's entire contribution without a word.
for (const file of listWithFallback([pattern], cwd)) files.add(file);
}
continue;
}

// Only the base lookup is guarded. Wrapping the traversal too would
// re-swallow an `ENOENT` raised *inside* `walk` — a descendant removed
// between `readdirSync` calls — and silently drop the directory's whole
// contribution, which the glob branch above already avoids.
let stats;
try {
stats = statSync(absolute);
} catch (error) {
if (!isMissingPathError(error)) throw error;
continue;
}
if (stats.isFile()) {
if (TEST_FILE_RE.test(absolute)) files.add(absolute);
continue;
}
if (stats.isDirectory()) {
const matches = runRg(["--files", "-g", "*.test.*", absolute], cwd);
if (matches) {
for (const match of matches) files.add(resolve(cwd, match));
} else {
for (const file of listWithFallback([absolute], cwd)) files.add(file);
}
}
}

if (files.size === 0) {
return listWithFallback(patterns, cwd);
}

return Array.from(files);
}

export function splitIntoShards(files, shardCount) {
const total = Math.max(1, Math.min(shardCount, files.length || 1));
const shards = Array.from({ length: total }, () => []);
Expand Down
Loading