diff --git a/deno.json b/deno.json index ef37551e3d..99551b3ba1 100644 --- a/deno.json +++ b/deno.json @@ -557,7 +557,7 @@ "test:tool-search-live": "VF_DISABLE_LRU_INTERVAL=1 deno test --no-check -A tests/agent/verify-tool-search-live.test.ts", "test:cross-runtime": "deno run --allow-all src/platform/compat/cross-runtime.test.ts", "test:node": { - "command": "node ./tests/node/run-tests.mjs 'src/**/*.test.ts' extensions/ext-bundler-esbuild/src/binary.test.ts tests/ensure-npm-links.test.mjs", + "command": "node ./tests/node/run-tests.mjs 'src/**/*.test.ts' extensions/ext-bundler-esbuild/src/binary.test.ts tests/ensure-npm-links.test.mjs tests/test-file-utils.test.mjs", "dependencies": ["build:npm"] }, "test:bun": { diff --git a/tests/test-file-utils.mjs b/tests/test-file-utils.mjs index de3a7f86ba..146b641b5d 100644 --- a/tests/test-file-utils.mjs +++ b/tests/test-file-utils.mjs @@ -1,6 +1,6 @@ import { spawnSync } from "node:child_process"; import { readdirSync, statSync } from "node:fs"; -import { dirname, resolve, sep } from "node:path"; +import { dirname, relative, resolve, sep } from "node:path"; const TEST_FILE_RE = /\.test\.[cm]?[jt]sx?$/i; const GLOB_CHARS_RE = /[\*\?\[]/; @@ -38,11 +38,31 @@ function globToRegex(glob) { if (char === "*") { const next = glob[i + 1]; if (next === "*") { - re += ".*"; - i += 1; - } else { + // `**` only crosses directory boundaries when it is a *complete* + // path segment. Both ripgrep and node:fs `globSync` agree: + // src/**/*.test.ts -> src/a.test.ts AND src/nested/b.test.ts + // src/**.test.ts -> src/a.test.ts only (segment-scoped) + // src/foo**/*.test.ts -> src/foo/a.test.ts only, NOT src/foo.test.ts + // So the globstar translation is gated on both boundaries, and a + // `**` glued to other characters degrades to a single `*`. + const atSegmentStart = i === 0 || glob[i - 1] === "/"; + const after = glob[i + 2]; + if (atSegmentStart && after === "/") { + // Matches zero or more segments, so the depth-1 case is included. + re += "(?:.*\\/)?"; + i += 2; + continue; + } + if (atSegmentStart && after === undefined) { + re += ".*"; + i += 1; + continue; + } re += "[^/]*"; + i += 1; + continue; } + re += "[^/]*"; continue; } if (char === "?") { @@ -65,6 +85,14 @@ 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 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; const fullPath = resolve(dir, entry.name); if (entry.isDirectory()) { walk(fullPath, onFile); @@ -84,28 +112,81 @@ function getBaseDir(pattern, cwd) { return resolve(cwd, base || "."); } +/** + * A base path that does not exist contributes nothing. Anything else — + * `EACCES` on an ancestor, a device error — has to propagate: swallowing it + * would drop the whole pattern, and if that left the selection empty the + * runner would exit 0 having run nothing. That silent-omission failure is the + * reason this module is being fixed. + */ +function isMissingPathError(error) { + const code = error?.code; + return code === "ENOENT" || code === "ENOTDIR"; +} + +/** + * True when `target` sits under a dot-prefixed directory relative to `cwd`. + * + * Only segments below `cwd` count: a checkout that itself lives under a hidden + * directory is not thereby invisible to its own test runner. + */ +function hasHiddenSegment(target, cwd) { + const relativePath = relative(cwd, target); + // `startsWith("..")` alone would misread a directory *named* `..fixtures` as + // a parent path and skip the hidden check entirely. Only an exact `..` or a + // `..` followed by a separator escapes `cwd`. + const escapesCwd = relativePath === ".." || + relativePath.startsWith(`..${sep}`) || relativePath.startsWith("../"); + if (relativePath === "" || escapesCwd) return false; + return toPosixPath(relativePath).split("/").some((segment) => segment.startsWith(".")); +} + function listWithFallback(patterns, cwd) { 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". + let stats; try { - const stats = statSync(absolute); - if (stats.isDirectory()) { - walk(absolute, (file) => { - if (TEST_FILE_RE.test(file)) files.add(file); - }); - } else if (stats.isFile() && TEST_FILE_RE.test(absolute)) { - files.add(absolute); - } - } catch { - // Ignore missing paths. + stats = statSync(absolute); + } catch (error) { + if (!isMissingPathError(error)) throw error; + continue; + } + if (stats.isDirectory()) { + walk(absolute, (file) => { + if (TEST_FILE_RE.test(file)) files.add(file); + }); + } else if (stats.isFile() && TEST_FILE_RE.test(absolute)) { + files.add(absolute); } continue; } 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. + 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 + // around the walk: a failure *inside* the traversal (an unreadable + // subdirectory, a file removed mid-walk) would otherwise be swallowed + // after `walk` had already accumulated part of the tree, and the runner + // would execute a partial selection and report success — which is the + // exact silent-omission failure this module is being fixed for. Those + // errors propagate. + try { + if (!statSync(baseDir).isDirectory()) continue; + } catch (error) { + if (!isMissingPathError(error)) throw error; + continue; + } const matcher = globToRegex(toPosixPath(pattern)); walk(baseDir, (file) => { const rel = toPosixPath(file.startsWith(cwd) ? file.slice(cwd.length + 1) : file); @@ -125,26 +206,39 @@ export function listTestFiles(patterns, cwd = process.cwd()) { const matches = runRg(["--files", "-g", pattern], cwd); if (matches) { for (const match of matches) files.add(resolve(cwd, match)); - continue; + } 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 { - const stats = statSync(absolute); - 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); - } + 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); } - } catch { - // Ignore missing paths or stat failures. } } diff --git a/tests/test-file-utils.test.mjs b/tests/test-file-utils.test.mjs new file mode 100644 index 0000000000..8b2f554af2 --- /dev/null +++ b/tests/test-file-utils.test.mjs @@ -0,0 +1,402 @@ +import { deepStrictEqual, ok } from "node:assert/strict"; +import { spawnSync } from "node:child_process"; +import { + chmodSync, + globSync, + mkdirSync, + mkdtempSync, + rmSync, + statSync, + writeFileSync, +} from "node:fs"; +import { tmpdir } from "node:os"; +import { dirname, join, resolve, sep } from "node:path"; +import { describe, it } from "node:test"; + +const utilsUrl = new URL("./test-file-utils.mjs", import.meta.url).href; + +const BASE_TREE = [ + "src/a.test.ts", + "src/nested/b.test.ts", + "src/nested/deep/c.test.ts", + "src/not-a-test.ts", + // Hidden directory: both `rg` (without --hidden) and node:fs glob skip these, + // so the in-process fallback must too or selection depends on whether + // ripgrep happens to be installed. + "src/.fixtures/hidden.test.ts", + "extra/explicit.test.mjs", +]; + +const GLOBSTAR_TREE = [ + "src/b.test.ts", + "src/foo.test.ts", + "src/foo/a.test.ts", + "src/foo/deep/d.test.ts", +]; + +/** + * Build a throwaway tree, hand it to `run`, then remove it. + * + * Teardown is per-test rather than a `node:test` `after` hook: `tests/` is also + * swept by `deno test` in the integration lane, and Deno's `node:test` shim + * does not implement `after` — it fails the whole file with an uncaught + * "Not implemented: test.after". `describe`/`it` are supported in both, which + * is why the sibling `ensure-npm-links.test.mjs` sticks to them. + */ +function withFixture(relativePaths, run) { + const root = mkdtempSync(join(tmpdir(), "vf-test-file-utils-")); + try { + for (const relative of relativePaths) { + const absolute = join(root, relative); + mkdirSync(dirname(absolute), { recursive: true }); + writeFileSync(absolute, "// fixture\n"); + } + run(root); + } finally { + rmSync(root, { recursive: true, force: true }); + } +} + +/** + * Resolve patterns with `rg` guaranteed absent. + * + * `rgAvailable` is module-level state latched on the first ENOENT, so each + * scenario runs in its own child process with an empty PATH instead of trying + * to reset it in-process. An empty PATH is what makes `spawnSync("rg", ...)` + * fail with ENOENT, which is the branch this suite is about — and the branch + * the CI runners actually take. + */ +function runListTestFilesProbe(patterns, cwd) { + // The probe goes to a real file rather than `-e`. This suite runs in both + // the Node lane and the Deno integration lane (which sweeps all of `tests/`), + // and `process.execPath` is whichever runtime is hosting — so a Node-only + // `--input-type=module -e` invocation fails under Deno with "await is only + // valid in async functions and the top level bodies of modules". + const probeDir = mkdtempSync(join(tmpdir(), "vf-test-file-utils-probe-")); + const probePath = join(probeDir, "probe.mjs"); + writeFileSync( + probePath, + `import { listTestFiles } from ${JSON.stringify(utilsUrl)};\n` + + `process.stdout.write(JSON.stringify(listTestFiles(${JSON.stringify(patterns)}, ${ + JSON.stringify(cwd) + })));\n`, + ); + // Deno needs its permissions named; Node takes the script path alone. + const args = typeof globalThis.Deno === "undefined" + ? [probePath] + : ["run", "--allow-read", "--allow-env", "--allow-run", probePath]; + try { + return spawnSync(process.execPath, args, { + encoding: "utf8", + env: { ...process.env, PATH: "" }, + }); + } finally { + rmSync(probeDir, { recursive: true, force: true }); + } +} + +function listTestFilesWithoutRipgrep(patterns, cwd) { + const result = runListTestFilesProbe(patterns, cwd); + ok( + result.status === 0, + `child listTestFiles failed (status ${result.status}): ${result.stderr}`, + ); + return JSON.parse(result.stdout); +} + +function relativeSorted(files, root) { + return files + .map((file) => resolve(file).slice(resolve(root).length + 1).split(sep).join("/")) + .sort(); +} + +/** + * `globSync` yields directory entries as well as files (`src/**` includes + * `src/foo`), while `listTestFiles` only ever returns files. Compare like with + * like so the reference stays meaningful for directory-matching patterns. + */ +function globFilesSorted(pattern, root) { + return globSync(pattern, { cwd: root }) + .filter((entry) => statSync(join(root, entry)).isFile()) + .map((entry) => entry.split(sep).join("/")) + .sort(); +} + +describe("listTestFiles without ripgrep", () => { + it("resolves a glob pattern when no other pattern matched", () => { + withFixture(BASE_TREE, (root) => { + deepStrictEqual( + relativeSorted(listTestFilesWithoutRipgrep(["src/**/*.test.ts"], root), root), + [ + "src/a.test.ts", + "src/nested/b.test.ts", + "src/nested/deep/c.test.ts", + ], + ); + }); + }); + + it("resolves a glob pattern alongside an explicit file pattern", () => { + withFixture(BASE_TREE, (root) => { + // The regression: the explicit file makes the result set non-empty, so + // the whole-result-set fallback never fires and the glob silently + // contributes nothing. Selection collapses to the explicit file alone. + const files = listTestFilesWithoutRipgrep( + ["src/**/*.test.ts", "extra/explicit.test.mjs"], + root, + ); + deepStrictEqual(relativeSorted(files, root), [ + "extra/explicit.test.mjs", + "src/a.test.ts", + "src/nested/b.test.ts", + "src/nested/deep/c.test.ts", + ]); + }); + }); + + it("resolves a glob pattern alongside a directory pattern", () => { + withFixture(BASE_TREE, (root) => { + const files = listTestFilesWithoutRipgrep(["src/**/*.test.ts", "extra"], root); + deepStrictEqual(relativeSorted(files, root), [ + "extra/explicit.test.mjs", + "src/a.test.ts", + "src/nested/b.test.ts", + "src/nested/deep/c.test.ts", + ]); + }); + }); + + it("does not invent matches for a glob that matches nothing", () => { + withFixture(BASE_TREE, (root) => { + const files = listTestFilesWithoutRipgrep( + ["does-not-exist/**/*.test.ts", "extra/explicit.test.mjs"], + root, + ); + deepStrictEqual(relativeSorted(files, root), ["extra/explicit.test.mjs"]); + }); + }); + + it("keeps a glob's contribution identical with and without an extra pattern", () => { + withFixture(BASE_TREE, (root) => { + const globOnly = relativeSorted( + listTestFilesWithoutRipgrep(["src/**/*.test.ts"], root), + root, + ); + const withExtra = relativeSorted( + listTestFilesWithoutRipgrep(["src/**/*.test.ts", "extra/explicit.test.mjs"], root), + root, + ).filter((file) => file.startsWith("src/")); + deepStrictEqual(withExtra, globOnly); + }); + }); +}); + +describe("listTestFiles treats ** as a globstar only as a complete segment", () => { + // Raised in review on #3780. `**` glued to other characters is segment-scoped + // in both ripgrep and node:fs glob, so the zero-segment translation has to be + // gated on both boundaries or `src/foo**/` wrongly selects `src/foo.test.ts`. + for ( + const pattern of [ + "src/foo**/*.test.ts", + "src/**.test.ts", + "src/**/*.test.ts", + "src/**", + "**/*.test.ts", + "src/foo/**/*.test.ts", + ] + ) { + it(`resolves ${pattern} the way node:fs glob does`, () => { + withFixture(GLOBSTAR_TREE, (root) => { + deepStrictEqual( + relativeSorted(listTestFilesWithoutRipgrep([pattern], root), root), + globFilesSorted(pattern, root), + ); + }); + }); + } +}); + +describe("listTestFiles matches ripgrep on dot-prefixed entries", () => { + // `rg` is the oracle for this case, not node:fs glob, because `rg` is what + // runs when it is installed — the fallback exists to reproduce its + // selection. Verified directly against rg 14: + // rg --files -g "*.test.*" src -> includes src/.smoke.test.ts + // rg --files -g "src/**/*.test.ts" -> includes src/.smoke.test.ts + // rg --files -g "src/**/*.test.ts" -> OMITS src/.fixtures/x.test.ts + // `-g/--glob` "always overrides any other ignore logic", so a dot-prefixed + // *file* is matched while a hidden *directory* is still pruned. node:fs glob + // excludes both, so the globSync-pinned suite below cannot cover this. + const DOTFILE_TREE = [ + "src/a.test.ts", + "src/.smoke.test.ts", + "src/.fixtures/skipped.test.ts", + "src/nested/b.test.ts", + ]; + + it("keeps a dot-prefixed file matched by a glob pattern", () => { + withFixture(DOTFILE_TREE, (root) => { + deepStrictEqual( + relativeSorted(listTestFilesWithoutRipgrep(["src/**/*.test.ts"], root), root), + ["src/.smoke.test.ts", "src/a.test.ts", "src/nested/b.test.ts"], + ); + }); + }); + + it("keeps a dot-prefixed file under a directory pattern", () => { + withFixture(DOTFILE_TREE, (root) => { + deepStrictEqual( + relativeSorted(listTestFilesWithoutRipgrep(["src"], root), root), + ["src/.smoke.test.ts", "src/a.test.ts", "src/nested/b.test.ts"], + ); + }); + }); + + it("matches nothing when the glob's literal prefix is a hidden directory", () => { + // rg prunes the hidden directory before applying the glob, so this pattern + // returns no files at all. `walk` starts inside the base, so the per-entry + // check never sees `.fixtures` — the base itself has to be rejected. + withFixture(DOTFILE_TREE, (root) => { + deepStrictEqual( + relativeSorted(listTestFilesWithoutRipgrep(["src/.fixtures/**/*.test.ts"], root), root), + [], + ); + }); + }); + + it("treats a directory named ..something as hidden, not as a parent path", () => { + // `relative()` returns `..fixtures` here, which a bare startsWith("..") + // check reads as "outside cwd" and waves through. rg 15 returns nothing + // for this pattern. + withFixture(["..fixtures/a.test.ts", "src/a.test.ts"], (root) => { + deepStrictEqual( + relativeSorted(listTestFilesWithoutRipgrep(["..fixtures/**/*.test.ts"], root), root), + [], + ); + }); + }); + + it("still prunes a hidden directory", () => { + withFixture(DOTFILE_TREE, (root) => { + const selected = relativeSorted( + listTestFilesWithoutRipgrep(["src/**/*.test.ts"], root), + root, + ); + deepStrictEqual(selected.includes("src/.fixtures/skipped.test.ts"), false); + }); + }); +}); + +describe("listTestFiles agrees with the platform glob", () => { + // `node:fs` globSync is the reference implementation: ripgrep's `-g` returns + // the same set for these patterns, and pinning against a built-in keeps the + // assertion deterministic on machines where `rg` is absent. + for ( + const pattern of [ + "src/**/*.test.ts", + "src/**/*.test.*", + "**/*.test.ts", + "src/*.test.ts", + "src/nested/**/*.test.ts", + ] + ) { + it(`resolves ${pattern} the way node:fs glob does`, () => { + withFixture(BASE_TREE, (root) => { + deepStrictEqual( + relativeSorted(listTestFilesWithoutRipgrep([pattern], root), root), + globFilesSorted(pattern, root), + ); + }); + }); + } +}); + +describe("listTestFiles does not hide a failed traversal", () => { + // Raised in review on #3780. Catching around the whole walk swallowed errors + // raised *during* traversal after files had already been collected, so the + // runner could execute a partial selection and report success — the same + // silent-omission failure this module is being fixed for. + it("propagates an unreadable descendant of a directory pattern", () => { + if (typeof process.getuid === "function" && process.getuid() === 0) return; + if (process.platform === "win32") return; + withFixture(BASE_TREE, (root) => { + // A *directory* pattern routes through `listWithFallback` from inside + // `listTestFiles`'s own try. A broad catch there swallowed the rethrow, + // so the directory's tests were dropped and only the other explicit + // pattern survived — reported as a clean pass. + const blocked = join(root, "src", "nested"); + chmodSync(blocked, 0o000); + try { + const result = runListTestFilesProbe(["src", "extra/explicit.test.mjs"], root); + ok( + result.status !== 0, + `expected a non-zero exit, got ${result.status} with stdout: ${result.stdout}`, + ); + ok( + /EACCES|EPERM/.test(result.stderr), + `expected a permission error to surface, got: ${result.stderr}`, + ); + } finally { + chmodSync(blocked, 0o755); + } + }); + }); + + it("propagates an unreadable glob base instead of dropping the pattern", () => { + if (typeof process.getuid === "function" && process.getuid() === 0) return; + if (process.platform === "win32") return; + withFixture(BASE_TREE, (root) => { + // The glob's base is `src/`, reached through an ancestor we cannot + // search. `statSync` raises EACCES rather than ENOENT, and treating that + // as "missing" would drop the whole pattern — leaving an empty selection + // that the runner reports as a clean pass. + const gate = join(root, "src"); + chmodSync(gate, 0o000); + try { + const result = runListTestFilesProbe(["src/nested/**/*.test.ts"], root); + ok( + result.status !== 0, + `expected a non-zero exit, got ${result.status} with stdout: ${result.stdout}`, + ); + ok( + /EACCES|EPERM/.test(result.stderr), + `expected a permission error to surface, got: ${result.stderr}`, + ); + } finally { + chmodSync(gate, 0o755); + } + }); + }); + + it("propagates an unreadable subdirectory instead of returning a partial set", () => { + if (typeof process.getuid === "function" && process.getuid() === 0) { + // root ignores the mode bits, so the error cannot be provoked. + return; + } + if (process.platform === "win32") { + // `chmod 000` does not deny directory traversal on Windows, so the child + // would succeed and the assertion below would fail for the wrong reason. + // No CI runner is Windows today; this is for local runs. + return; + } + withFixture(BASE_TREE, (root) => { + const blocked = join(root, "src", "blocked"); + mkdirSync(blocked, { recursive: true }); + writeFileSync(join(blocked, "hidden.test.ts"), "// fixture\n"); + chmodSync(blocked, 0o000); + try { + const result = runListTestFilesProbe(["src/**/*.test.ts"], root); + ok( + result.status !== 0, + `expected a non-zero exit, got ${result.status} with stdout: ${result.stdout}`, + ); + ok( + /EACCES|EPERM/.test(result.stderr), + `expected a permission error to surface, got: ${result.stderr}`, + ); + } finally { + // Restore before teardown, or the recursive remove cannot descend. + chmodSync(blocked, 0o755); + } + }); + }); +});