fix(tests): resolve bare-basename, brace and literal-bracket patterns - #3858
Conversation
Closes #3857. Three pattern shapes selected ZERO files where ripgrep 15.2.0 returns real matches. Zero is the dangerous answer: run-tests.mjs turns an empty selection into process.exit(0), so the command reports success having run nothing. pattern before rg *.test.ts 0 4 src/{a,b}.test.ts 0 2 src/routes/[id] 0 1 Bare basename globs were anchored to the root. `rg` matches a slash-free glob against the basename at any depth; `globToRegex` anchored it at `cwd`, so `*.test.ts` matched nothing unless a test sat directly beside `cwd`. Patterns containing a separator stay anchored — pinned by a test, since that is the half a basename rule is most likely to break. Brace alternation was not recognised at all: `{` was absent from GLOB_CHARS_RE, so `src/{a,b}.test.ts` was treated as a literal path, ENOENT'd, and was dropped without a word. `expandBraces` expands the outermost group and recurses, so nested and multi-segment branches work; an unbalanced `{` stays literal, as in `rg`. A real directory containing brackets was unreachable. `[id]` is both a valid character class and a legal directory name, and this repo has such directories (src/discovery/__fixtures__/autodiscovery/resources/users/[userId]/). An existing path now wins over a glob reading of the same string — the same way `rg` resolves that ambiguity for an explicit path argument. Selection is unchanged for every consumer, verified against origin/main: test:node 1618, test:bun 1749, affected 1948 — identical on both sides. Red first: 3 of the 5 new tests fail on origin/main, the two guard cases pass. Green: 36/36 under Node, 8 passed (36 steps) under Deno.
|
Warning Review limit reached
Next review available in: 40 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
📦 Client bundle boundary
A server module in a client graph aborts hydration in the browser. New leaks fail CI; known leaks are tracked in |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a87e906be
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
`expandBraces` scanned the raw pattern with no notion of `[...]` classes, so
`src/[{}].test.ts` was rewritten to `src/[].test.ts` before `globToRegex` could
parse the class — and that selects nothing. Empty is the failure this whole
change set exists to remove: `tests/node/run-tests.mjs` turns an empty
selection into `process.exit(0)`, so the lane reports success having run no
tests at all.
Verified against rg 15.2.0 and `node:fs` globSync on a fixture holding files
literally named `{.test.ts` and `}.test.ts`; both return the two files, and
`filterTestFiles` — which calls `globToRegex` without `expandBraces` — already
did too. The translator was never wrong; only the scan that ran ahead of it.
Every scan in `expandBraces` (opening brace, its match, the branch commas) now
steps over classes, and the class-extent logic is shared with `globToRegex` via
`findClassEnd` so the two cannot drift apart again — two scanners over one
grammar is how this bug arose.
Backslash escapes are left alone on purpose. `globToRegex` does not honour them
either, so `rg -g 'src/\{a\}.test.ts'` matches where this module does not;
skipping `\{` in the scan would only hand the translator a pattern it still
cannot match. That is a separate gap in the translator.
Consumer selections are unchanged: 1618 / 1749 / 1948 for the `test:node`,
`test:bun` and affected-test pattern sets.
2 participants
Closes #3857.
Three pattern shapes selected nothing
rg 15.2.0*.test.tssrc/{a,b}.test.tssrc/routes/[id]Zero is the dangerous answer.
tests/node/run-tests.mjsturns an empty selection intoprocess.exit(0), so on main today:That is the silent-green failure mode #3780 and #3784 existed to eliminate, still reachable — and invisible to anyone whose machine has ripgrep, because it only bites once the in-process resolver is the one deciding.
Causes
Bare basename globs were anchored to the root.
rgmatches a slash-free glob against the basename at any depth;globToRegexanchored atcwd. Note the half most likely to break in fixing this:src/*.test.tsmust stay anchored to depth-1 undersrc/, not become "any a.test.ts anywhere". There is a test for that specifically.Brace alternation was not a glob at all.
{was absent fromGLOB_CHARS_RE, sosrc/{a,b}.test.tswas read as a literal path,statSyncENOENT'd, and the pattern was dropped silently.A real directory containing brackets was unreachable.
[id]is both a valid character class and a legal directory name — and this repo already has them (src/discovery/__fixtures__/autodiscovery/resources/users/[userId]/). Classifying the string as a class meant it could never match the directory literally named[id]. An existing path now wins, the same wayrgresolves that ambiguity for an explicit path argument.Selection is unchanged
The risk in touching this module is moving what the lanes run. Verified against
origin/main, same fixture, both implementations:Verification
Red first — 3 of 5 new tests fail on
origin/main; the two guard cases (anchored patterns, bracket classes) pass before and after, which is what stops the fix over-reaching.Green: 36/36 under Node, 8 passed (36 steps) under Deno.
tests/runtime-test-filters.test.tsunchanged.All four shapes now match
rgexactly, verified side by side.