Repository navigation
fix(tests): stop listTestFiles silently dropping glob patterns #3780
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.
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
Changes from 1 commit
5d9fb30
51078d6
08da949
76bf50a
49fe41b
98336d3
f88aab9
810e0d3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,8 +38,18 @@ function globToRegex(glob) { | |
| if (char === "*") { | ||
| const next = glob[i + 1]; | ||
| if (next === "*") { | ||
| re += ".*"; | ||
| i += 1; | ||
| // `**` crosses directory boundaries. Written as its own segment | ||
| // (`src/**/*.test.ts`) it must also match *zero* segments, so | ||
| // `src/a.test.ts` is selected — this is what both ripgrep and | ||
| // node:fs `globSync` do. Translating it to `.*` alone requires a | ||
| // trailing separator and silently drops every depth-1 match. | ||
| if (glob[i + 2] === "/") { | ||
| re += "(?:.*\\/)?"; | ||
| i += 2; | ||
| } else { | ||
| re += ".*"; | ||
| i += 1; | ||
| } | ||
| } else { | ||
| re += "[^/]*"; | ||
| } | ||
|
|
@@ -107,10 +117,16 @@ function listWithFallback(patterns, cwd) { | |
|
|
||
| const baseDir = getBaseDir(pattern, cwd); | ||
| const matcher = globToRegex(toPosixPath(pattern)); | ||
| walk(baseDir, (file) => { | ||
| const rel = toPosixPath(file.startsWith(cwd) ? file.slice(cwd.length + 1) : file); | ||
| if (matcher.test(rel)) files.add(file); | ||
| }); | ||
| try { | ||
| walk(baseDir, (file) => { | ||
| const rel = toPosixPath(file.startsWith(cwd) ? file.slice(cwd.length + 1) : file); | ||
| if (matcher.test(rel)) files.add(file); | ||
| }); | ||
| } catch { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When the base exists but a descendant cannot be read, or disappears during traversal, this catch suppresses that error after Useful? React with 👍 / 👎. |
||
| // A glob whose base directory does not exist contributes nothing, the | ||
| // same as the non-glob branch above. Left unguarded this throws out of | ||
| // the runner entirely instead of yielding an empty selection. | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| } | ||
| return Array.from(files); | ||
| } | ||
|
|
@@ -125,8 +141,15 @@ 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); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When ripgrep is unavailable and the glob base contains a hidden directory, this new per-pattern fallback traverses it unconditionally. For example, Useful? React with 👍 / 👎. |
||
| } | ||
| continue; | ||
| } | ||
|
|
||
| try { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,158 @@ | ||
| import { deepStrictEqual, ok } from "node:assert/strict"; | ||
| import { spawnSync } from "node:child_process"; | ||
| import { globSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; | ||
| import { tmpdir } from "node:os"; | ||
| import { dirname, join, resolve, sep } from "node:path"; | ||
| import { fileURLToPath } from "node:url"; | ||
| import { after, describe, it } from "node:test"; | ||
|
|
||
| const utilsUrl = new URL("./test-file-utils.mjs", import.meta.url).href; | ||
| const fixtures = []; | ||
|
|
||
| /** | ||
| * The suite exercises the branch taken when `rg` is not on PATH, which is the | ||
| * case on the CI runners. `rgAvailable` is module-level state latched on the | ||
| * first ENOENT, so each scenario runs in its own child process with an empty | ||
| * PATH rather than trying to reset it in-process. | ||
| */ | ||
| function createFixture() { | ||
| const root = mkdtempSync(join(tmpdir(), "vf-test-file-utils-")); | ||
| fixtures.push(root); | ||
| for ( | ||
| const relative of [ | ||
| "src/a.test.ts", | ||
| "src/nested/b.test.ts", | ||
| "src/nested/deep/c.test.ts", | ||
| "src/not-a-test.ts", | ||
| "extra/explicit.test.mjs", | ||
| ] | ||
| ) { | ||
| const absolute = join(root, relative); | ||
| mkdirSync(dirname(absolute), { recursive: true }); | ||
| writeFileSync(absolute, "// fixture\n"); | ||
| } | ||
| return root; | ||
| } | ||
|
|
||
| function listTestFilesWithoutRipgrep(patterns, cwd) { | ||
| const script = ` | ||
| const { listTestFiles } = await import(${JSON.stringify(utilsUrl)}); | ||
| const files = listTestFiles(${JSON.stringify(patterns)}, ${JSON.stringify(cwd)}); | ||
| process.stdout.write(JSON.stringify(files)); | ||
| `; | ||
| const result = spawnSync(process.execPath, ["--input-type=module", "-e", script], { | ||
| encoding: "utf8", | ||
| // An empty PATH is what makes spawnSync("rg", ...) fail with ENOENT, which | ||
| // is the condition this suite is about. Everything else is inherited. | ||
| env: { ...process.env, PATH: "" }, | ||
| }); | ||
| 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(); | ||
| } | ||
|
|
||
| after(() => { | ||
| for (const root of fixtures) rmSync(root, { recursive: true, force: true }); | ||
| }); | ||
|
|
||
| describe("listTestFiles without ripgrep", () => { | ||
| it("resolves a glob pattern when no other pattern matched", () => { | ||
| const root = createFixture(); | ||
| const files = listTestFilesWithoutRipgrep(["src/**/*.test.ts"], root); | ||
| deepStrictEqual(relativeSorted(files, 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", () => { | ||
| const root = createFixture(); | ||
| // 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", () => { | ||
| const root = createFixture(); | ||
| 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", () => { | ||
| const root = createFixture(); | ||
| 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", () => { | ||
| const root = createFixture(); | ||
| 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 agrees with the platform glob", () => { | ||
| // `node:fs` globSync is the reference implementation here: 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`, () => { | ||
| const root = createFixture(); | ||
| const expected = globSync(pattern, { cwd: root }) | ||
| .map((file) => file.split(sep).join("/")) | ||
| .sort(); | ||
| deepStrictEqual(relativeSorted(listTestFilesWithoutRipgrep([pattern], root), root), expected); | ||
| }); | ||
| } | ||
|
|
||
| it("matches the in-process path, whatever ripgrep availability is here", async () => { | ||
| const root = createFixture(); | ||
| const patterns = ["src/**/*.test.ts", "extra/explicit.test.mjs"]; | ||
| const { listTestFiles } = await import( | ||
| fileURLToPath(new URL("./test-file-utils.mjs", import.meta.url)) | ||
| ); | ||
| deepStrictEqual( | ||
| relativeSorted(listTestFiles(patterns, root), root), | ||
| relativeSorted(listTestFilesWithoutRipgrep(patterns, root), root), | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When
**/is preceded by non-separator characters, this branch incorrectly makes the slash optional. For example, without ripgrep,src/foo**/*.test.tsselects bothsrc/foo/a.test.tsandsrc/foo.test.ts, while bothnode:fsglobSyncandrg --files -gselect only the nested file. Check that**starts at the beginning of a path segment before applying the zero-segment translation.Useful? React with 👍 / 👎.