Repository navigation
Conversation
|
Updated 11:36 PM PT - Oct 15th, 2026
❌ @robobun, your commit f20d19d has 2 failures in
🧪 To try this PR locally: bunx bun-pr 45447That installs a local version of the PR into your bun-45447 --bun |
|
How to reproduce on a release build:
This PR is the CI at f20d19d (build 126380) is finished: 179 jobs passed and 2 failed. Neither failure is in code that this PR changes, and
One decision is open for a maintainer: is the case-insensitive match wanted as the default? The PR body lists who it affects. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe ChangesTest-name matching
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to
|
There was a problem hiding this comment.
Beyond the inline findings, I also checked for sibling sites compiling the name pattern: RegularExpression::init at src/runtime/cli/Arguments.rs:1939 is the only compile of -t, and parallel workers receive the raw test_filter_pattern (src/runtime/cli/test/parallel/runner.rs:315) and re-parse it through the same path, so they inherit the same flag — no bunfig or programmatic site needs a matching change.
Extended reasoning...
The diff is a one-flag change (RegexFlags::None to IgnoreCase) in the -t/--test-name-pattern parser, plus two doc edits and one new test; it touches no security-sensitive surface. Inline findings already cover the default-behavior change for existing users and the one-directional (?-i:...) assertion, so a human should weigh the compat decision; this body only records the sibling-site check that was ruled out.
Findings marked 🟡 are optional suggestions and need no follow-up push.
There was a problem hiding this comment.
I re-reviewed after the latest commit and found no bugs; the help text, completions, and the positive (?-i:...) test variant now cover what I raised earlier. Since this flips the default selection behavior of a shipped flag (-t/--test-name-pattern/--grep) for every existing caller, a maintainer should still decide whether that break is acceptable.
What was reviewed:
- Flag plumbing:
IgnoreCase = 1 << 2in src/jsc/RegularExpression.rs is passed verbatim intoOptionSet<Yarr::Flags>in src/jsc/bindings/RegularExpression.cpp, and this is the only compile site; test_command.rs and the parallel runner's-tforwarding both consume it. - Existing suite: grepped every
-t/--test-name-patternuse in test/ — none asserts case-sensitive filtering, so no stale assertions. - One consumer worth a maintainer's eye: the VS Code extension (packages/bun-vscode/src/features/tests/bun-test-controller.ts,
buildTestNamePattern) builds anchored^ ?name$patterns from exact test names and does not wrap them in(?-i:...), so "Run this test" will now also run a sibling whose name differs only by case.
Extended reasoning...
The change is a one-constant edit in src/runtime/cli/Arguments.rs (RegexFlags::None to RegexFlags::IgnoreCase) plus help text, shell completions, two docs pages, and one new test in test/cli/test/bun-test.test.ts. It touches no security-sensitive surface; the regex is compiled from a CLI argument with the same JSC engine as before. The follow-up commit addressed both prior inline comments, the flag bit maps correctly to Yarr, and no existing test depends on the old case-sensitive behavior. Deferring because the PR changes the default behavior of a shipped user-facing flag (including the Mocha-style --grep alias) with no opt-in, which is a product decision for a maintainer rather than a correctness question.
|
Thank you. I added the VS Code extension to Downsides in the PR body: |
Jest compiles testNamePattern with the i flag, so -t foo runs a test named Foo. bun compiled the pattern with no flags and filtered that test out. Compile the pattern with IgnoreCase. A (?-i:...) group in the pattern matches case again.
The default for -t changed, so the places that describe the flag say so. The test now also checks that a (?-i:...) group selects a test when the case is right.
6218c37 to
f20d19d
Compare
Problem
bun test -t 'mixedcase target'does not run a test namedMixedCase Target. It printserror: regex "mixedcase target" matched 0 tests. Jest runs that test.RegexFlags::Nonewhere the pattern is compiled (src/runtime/cli/Arguments.rs:1941). jest-circus 30.5.2 compilesnew RegExp(testNamePattern, 'i').Fix
RegexFlags::IgnoreCase.(?-i:...)group in the pattern matches case again.--help, the shell completions and the docs now say that it ignores case.test/cli/test/bun-test.test.ts, test-t matches test names case-insensitively. Release 1.4.3-canary.1 fails it and this branch passes it.Background
-t/--test-name-patternfilters tests by name. Bun matches the regex against the test name prefixed with the labels of itsdescribeblocks.Downsides
-t APIalso runs a test namedcapital letters. No flag restores the old match. A user rewrites the pattern as-t '(?-i:API)'.^ ?name$patterns from exact test names (buildTestNamePatterninpackages/bun-vscode). "Run this test" now also runs a test whose name differs only in case. This PR does not change the extension.-t, each name match folds case: +156 instructions (8,230 to 8,386) for a 68-character name with no match. Without-t, Bun compiles no pattern.Notes
Source of the ask: item (c) of a test-runner audit against jest 30.4.1.
The alternative to the default change is an opt-in flag. I did not add one, because the ask was Jest's behavior by default and a new flag is new public surface.
Vitest and Mocha's
--grepmatch case. Bun's--grepalias now differs from Mocha's.The extension could wrap its pattern in
(?-i:...). I did not do that here: the extension runs the bun that the user has installed, and I have not checked which released versions accept a modifier group.Cost of the
iflag, in instructions for one call ofYarr__RegularExpression__matcheswith-t 'mixedcase target'. Release build of main (af5743d). gdb sets the flags argument ofYarr__RegularExpression__initto 0 or toIgnoreCase, then single-steps each call. 3 runs give the same counts.perfandvalgrindare not installed here.suite mixedcase targetsuite MixedCase Targetsuite another testbun-test.test.tson the local debug build: 103 pass, 6 todo, 2 fail. The two failures areinstances of a class nested too deeply to print are a RangeErrorandthe data of events nested too deeply to print are a RangeError. Both time out after 5000ms, and neither passes-t.