Repository navigation
Conversation
previously, brace patterns like '{apps/api,packages/{utils,core}}/**' would fail because the walker couldn't determine which directories to descend into w/o expanding the braces first
this matches how node fs.glob() handles them
WalkthroughThe changes implement brace-expansion support in the GlobWalker module. A new optional field tracks expanded patterns, initialization pre-processes patterns to compute effective variants, walk logic handles multiple expanded patterns, and a helper function detects when brace expressions require expansion across path components. Changes
Suggested reviewers
Pre-merge checks✅ Passed checks (2 passed)
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 |
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/glob/GlobWalker.zigtest/js/bun/glob/scan.test.ts
🧰 Additional context used
📓 Path-based instructions (5)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun:testwith files that end in*.test.{ts,js,jsx,tsx,mjs,cjs}
Do not write flaky tests. Never wait for time to pass in tests; always wait for the condition to be met instead of using an arbitrary amount of time
Never use hardcoded port numbers in tests. Always useport: 0to get a random port
Prefer concurrent tests over sequential tests usingtest.concurrentordescribe.concurrentwhen multiple tests spawn processes or write files, unless it's very difficult to make them concurrent
When spawning Bun processes in tests, usebunExeandbunEnvfromharnessto ensure the same build of Bun is used and debug logging is silenced
Use-eflag for single-file tests when spawning Bun processes
UsetempDir()from harness to create temporary directories with files for multi-file tests instead of creating files manually
Prefer async/await over callbacks in tests
When callbacks must be used and it's just a single callback, usePromise.withResolversto create a promise that can be resolved or rejected from a callback
Do not set a timeout on tests. Bun already has timeouts
UseBuffer.alloc(count, fill).toString()instead of'A'.repeat(count)to create repetitive strings in tests, as ''.repeat is very slow in debug JavaScriptCore builds
Usedescribeblocks for grouping related tests
Always useawait usingorusingto ensure proper resource cleanup in tests for APIs like Bun.listen, Bun.connect, Bun.spawn, Bun.serve, etc
Always check exit codes and test error scenarios in error tests
Usedescribe.each()for parameterized tests
UsetoMatchSnapshot()for snapshot testing
UsebeforeAll(),afterEach(),beforeEach()for setup/teardown in tests
Track resources (servers, clients) in arrays for cleanup inafterEach()
Files:
test/js/bun/glob/scan.test.ts
**/*.test.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.ts?(x): Never usebun testdirectly - always usebun bd testto run tests with debug build changes
For single-file tests, prefer-eflag overtempDir
For multi-file tests, prefertempDirandBun.spawnover single-file tests
UsenormalizeBunSnapshotto normalize snapshot output of tests
Never write tests that check for 'panic', 'uncaught exception', or similar strings in test output
UsetempDirfromharnessto create temporary directories - do not usetmpdirSyncorfs.mkdtempSync
When spawning processes in tests, expect stdout before expecting exit code for more useful error messages on test failure
Do not write flaky tests - do not usesetTimeoutin tests; instead await the condition to be met
Verify tests fail withUSE_SYSTEM_BUN=1 bun test <file>and pass withbun bd test <file>- tests are invalid if they pass with USE_SYSTEM_BUN=1
Test files must end with.test.tsor.test.tsx
Avoid shell commands likefindorgrepin tests - use Bun's Glob and built-in tools instead
Files:
test/js/bun/glob/scan.test.ts
test/**/*.test.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
Always use
port: 0in tests - do not hardcode ports or use custom random port number functions
Files:
test/js/bun/glob/scan.test.ts
src/**/*.zig
📄 CodeRabbit inference engine (src/CLAUDE.md)
src/**/*.zig: Private fields in Zig are fully supported using the#prefix:struct { #foo: u32 };
Use decl literals in Zig for declaration initialization:const decl: Decl = .{ .binding = 0, .value = 0 };
Prefer@importat the bottom of the file (auto formatter will move them automatically)
Files:
src/glob/GlobWalker.zig
**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, be careful with allocators and use defer for cleanup
Files:
src/glob/GlobWalker.zig
🧠 Learnings (20)
📓 Common learnings
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 22946
File: test/js/sql/sql.test.ts:195-202
Timestamp: 2025-09-25T22:07:13.851Z
Learning: PR oven-sh/bun#22946: JSON/JSONB result parsing updates (e.g., returning parsed arrays instead of legacy strings) are out of scope for this PR; tests keep current expectations with a TODO. Handle parsing fixes in a separate PR.
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Avoid shell commands like `find` or `grep` in tests - use Bun's Glob and built-in tools instead
Applied to files:
test/js/bun/glob/scan.test.tssrc/glob/GlobWalker.zig
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : For multi-file tests, prefer `tempDir` and `Bun.spawn` over single-file tests
Applied to files:
test/js/bun/glob/scan.test.ts
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun:test` with files that end in `*.test.{ts,js,jsx,tsx,mjs,cjs}`
Applied to files:
test/js/bun/glob/scan.test.tssrc/glob/GlobWalker.zig
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output
Applied to files:
test/js/bun/glob/scan.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Use `normalizeBunSnapshot` to normalize snapshot output of tests
Applied to files:
test/js/bun/glob/scan.test.ts
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `tempDir()` from harness to create temporary directories with files for multi-file tests instead of creating files manually
Applied to files:
test/js/bun/glob/scan.test.ts
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Prefer async/await over callbacks in tests
Applied to files:
test/js/bun/glob/scan.test.ts
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `describe` blocks for grouping related tests
Applied to files:
test/js/bun/glob/scan.test.ts
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Prefer concurrent tests over sequential tests using `test.concurrent` or `describe.concurrent` when multiple tests spawn processes or write files, unless it's very difficult to make them concurrent
Applied to files:
test/js/bun/glob/scan.test.ts
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `-e` flag for single-file tests when spawning Bun processes
Applied to files:
test/js/bun/glob/scan.test.ts
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `describe.each()` for parameterized tests
Applied to files:
test/js/bun/glob/scan.test.ts
📚 Learning: 2025-10-08T13:56:00.875Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 23373
File: src/bun.js/api/BunObject.zig:2514-2521
Timestamp: 2025-10-08T13:56:00.875Z
Learning: For Bun codebase: prefer using `bun.path` utilities (e.g., `bun.path.joinAbsStringBuf`, `bun.path.join`) over `std.fs.path` functions for path operations.
Applied to files:
src/glob/GlobWalker.zig
📚 Learning: 2025-11-24T18:37:11.466Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Write JS builtins for Bun's Node.js compatibility and APIs, and run `bun bd` after changes
Applied to files:
src/glob/GlobWalker.zig
📚 Learning: 2025-10-01T22:13:08.081Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23169
File: src/codegen/bindgenv2/internal/base.ts:120-125
Timestamp: 2025-10-01T22:13:08.081Z
Learning: Iterator.some() and other Iterator helper methods (map, filter, etc.) are supported in Bun and modern Node.js (22+) runtimes and can be safely used on string iterators and other iterator objects.
Applied to files:
src/glob/GlobWalker.zig
📚 Learning: 2025-09-12T18:16:50.754Z
Learnt from: RiskyMH
Repo: oven-sh/bun PR: 22606
File: src/glob/GlobWalker.zig:449-452
Timestamp: 2025-09-12T18:16:50.754Z
Learning: For Bun codebase: prefer using `std.fs.path.sep` over manual platform separator detection, and use `bun.strings.lastIndexOfChar` instead of `std.mem.lastIndexOfScalar` for string operations.
Applied to files:
src/glob/GlobWalker.zig
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : When spawning Bun processes in tests, use `bunExe` and `bunEnv` from `harness` to ensure the same build of Bun is used and debug logging is silenced
Applied to files:
src/glob/GlobWalker.zig
📚 Learning: 2025-11-24T18:37:47.899Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/AGENTS.md:0-0
Timestamp: 2025-11-24T18:37:47.899Z
Learning: Applies to src/bun.js/bindings/v8/**/<UNKNOWN> : <UNKNOWN>
Applied to files:
src/glob/GlobWalker.zig
📚 Learning: 2025-10-26T01:32:04.844Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24082
File: test/cli/test/coverage.test.ts:60-112
Timestamp: 2025-10-26T01:32:04.844Z
Learning: In the Bun repository test files (test/cli/test/*.test.ts), when spawning Bun CLI commands with Bun.spawnSync for testing, prefer using stdio: ["inherit", "inherit", "inherit"] to inherit stdio streams rather than piping them.
Applied to files:
src/glob/GlobWalker.zig
📚 Learning: 2025-08-30T09:09:18.384Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 22231
File: src/bundler/bundle_v2.zig:48-48
Timestamp: 2025-08-30T09:09:18.384Z
Learning: In Zig, when a module exports a top-level struct, import("./Module.zig") directly returns that struct type and can be used as a type alias without needing to access a field within the module. This is a common pattern in the Bun codebase.
Applied to files:
src/glob/GlobWalker.zig
🧬 Code graph analysis (1)
test/js/bun/glob/scan.test.ts (3)
test/harness.ts (1)
tempDirWithFiles(259-266)scripts/glob-sources.mjs (1)
glob(11-11)packages/bun-types/bun.d.ts (1)
Glob(6544-6599)
🔇 Additional comments (5)
src/glob/GlobWalker.zig (5)
310-313: LGTM! Proper memory management via arena allocator.The
expanded_patternsfield is correctly allocated using the arena allocator, so it will be automatically freed when the arena is deinitialized. The optional type and documentation are appropriate.
1114-1159: LGTM! Proper handling of multiple expanded patterns.The implementation correctly:
- Walks the first pattern (set up during init)
- Iterates over remaining expanded patterns
- Rebuilds pattern components for each variant
- Clears the work buffer between patterns to avoid state leakage
The fail-fast behavior on errors (returning immediately if any pattern walk fails) is appropriate for this use case.
1162-1182: LGTM! Clean extraction for pattern walking.The
walkSinglePatternfunction is a well-structured extraction from the originalwalkimplementation, enabling the new multi-pattern expansion feature while maintaining the existing walking logic.
1803-1803: LGTM! Appropriate reuse of existing brace expansion module.Importing the Braces module from the shell package is a good choice, avoiding code duplication and leveraging existing, tested brace expansion logic.
1062-1079: No action needed. TheBraces.expandBracesAlloc()function handles errors internally and never fails: on tokenization errors, allocation failures, or expansion issues, it catches the exception and returns the original input as a single-element list. The check forexpanded.items.len > 0is therefore always true and the code is correct as written.Likely an incorrect or invalid review comment.
| /// checks if brace alternation spans multiple path components (contains `/` inside braces). | ||
| /// multi-component alternation like `{a/b,c/d}` can't be resolved during traversal since | ||
| /// the walker needs to know which directories to descend into upfront. | ||
| /// single-component alternation like `{a,b}` is handled by normal pattern matching. | ||
| fn needsBraceExpansionForWalk(pattern: []const u8) bool { | ||
| var brace_depth: u32 = 0; | ||
| var in_brackets = false; | ||
| var i: usize = 0; | ||
|
|
||
| while (i < pattern.len) : (i += 1) { | ||
| const c = pattern[i]; | ||
| switch (c) { | ||
| '\\' => { | ||
| if (comptime !isWindows) { | ||
| // skip escaped character on non-windows | ||
| i += 1; | ||
| } else { | ||
| // on windows, backslash is a path separator | ||
| if (brace_depth > 0 and !in_brackets) return true; | ||
| } | ||
| }, | ||
| '[' => { | ||
| if (!in_brackets) in_brackets = true; | ||
| }, | ||
| ']' => { | ||
| in_brackets = false; | ||
| }, | ||
| '{' => { | ||
| if (!in_brackets) { | ||
| brace_depth += 1; | ||
| } | ||
| }, | ||
| '}' => { | ||
| if (!in_brackets and brace_depth > 0) { | ||
| brace_depth -= 1; | ||
| } | ||
| }, | ||
| '/' => { | ||
| // path separator inside braces means we need expansion | ||
| if (brace_depth > 0 and !in_brackets) return true; | ||
| }, | ||
| else => {}, | ||
| } | ||
| } | ||
| // only expand if we saw a path separator inside braces | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Logic is correct but escape handling could be clearer.
The function correctly detects whether brace patterns contain path separators and need expansion. The escape handling is correct but could be more explicit.
📝 Optional clarification for escape handling
Consider adding a comment to clarify the escape handling logic:
'\\' => {
if (comptime !isWindows) {
- // skip escaped character on non-windows
+ // skip escaped character on non-windows (i += 1 here, then continue expression adds 1)
i += 1;
} else {
// on windows, backslash is a path separator
if (brace_depth > 0 and !in_brackets) return true;
}
},This makes it clearer that the loop's continue expression (i += 1) will skip the escaped character.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// checks if brace alternation spans multiple path components (contains `/` inside braces). | |
| /// multi-component alternation like `{a/b,c/d}` can't be resolved during traversal since | |
| /// the walker needs to know which directories to descend into upfront. | |
| /// single-component alternation like `{a,b}` is handled by normal pattern matching. | |
| fn needsBraceExpansionForWalk(pattern: []const u8) bool { | |
| var brace_depth: u32 = 0; | |
| var in_brackets = false; | |
| var i: usize = 0; | |
| while (i < pattern.len) : (i += 1) { | |
| const c = pattern[i]; | |
| switch (c) { | |
| '\\' => { | |
| if (comptime !isWindows) { | |
| // skip escaped character on non-windows | |
| i += 1; | |
| } else { | |
| // on windows, backslash is a path separator | |
| if (brace_depth > 0 and !in_brackets) return true; | |
| } | |
| }, | |
| '[' => { | |
| if (!in_brackets) in_brackets = true; | |
| }, | |
| ']' => { | |
| in_brackets = false; | |
| }, | |
| '{' => { | |
| if (!in_brackets) { | |
| brace_depth += 1; | |
| } | |
| }, | |
| '}' => { | |
| if (!in_brackets and brace_depth > 0) { | |
| brace_depth -= 1; | |
| } | |
| }, | |
| '/' => { | |
| // path separator inside braces means we need expansion | |
| if (brace_depth > 0 and !in_brackets) return true; | |
| }, | |
| else => {}, | |
| } | |
| } | |
| // only expand if we saw a path separator inside braces | |
| return false; | |
| } | |
| /// checks if brace alternation spans multiple path components (contains `/` inside braces). | |
| /// multi-component alternation like `{a/b,c/d}` can't be resolved during traversal since | |
| /// the walker needs to know which directories to descend into upfront. | |
| /// single-component alternation like `{a,b}` is handled by normal pattern matching. | |
| fn needsBraceExpansionForWalk(pattern: []const u8) bool { | |
| var brace_depth: u32 = 0; | |
| var in_brackets = false; | |
| var i: usize = 0; | |
| while (i < pattern.len) : (i += 1) { | |
| const c = pattern[i]; | |
| switch (c) { | |
| '\\' => { | |
| if (comptime !isWindows) { | |
| // skip escaped character on non-windows (i += 1 here, then continue expression adds 1) | |
| i += 1; | |
| } else { | |
| // on windows, backslash is a path separator | |
| if (brace_depth > 0 and !in_brackets) return true; | |
| } | |
| }, | |
| '[' => { | |
| if (!in_brackets) in_brackets = true; | |
| }, | |
| ']' => { | |
| in_brackets = false; | |
| }, | |
| '{' => { | |
| if (!in_brackets) { | |
| brace_depth += 1; | |
| } | |
| }, | |
| '}' => { | |
| if (!in_brackets and brace_depth > 0) { | |
| brace_depth -= 1; | |
| } | |
| }, | |
| '/' => { | |
| // path separator inside braces means we need expansion | |
| if (brace_depth > 0 and !in_brackets) return true; | |
| }, | |
| else => {}, | |
| } | |
| } | |
| // only expand if we saw a path separator inside braces | |
| return false; | |
| } |
🤖 Prompt for AI Agents
In src/glob/GlobWalker.zig around lines 1581 to 1627, the escape handling in the
switch is correct but unclear: add a concise comment explaining that on
non-Windows the code intends to skip the character after a backslash by relying
on the loop's post-increment `(i += 1)`, and on Windows a backslash is treated
as a path separator so encountering one inside braces should force expansion;
update the backslash case with these two short explanatory comments so future
readers understand why we manually increment `i` on non-Windows and why we
return true on Windows when inside braces.
| // brace alternation without path separators (handled by normal matching) | ||
| describe("brace alternation (basic)", () => { | ||
| test("braces in first segment", async () => { | ||
| const tempdir = tempDirWithFiles("glob-brace-first", { | ||
| apps: { | ||
| "a.txt": "content", | ||
| }, | ||
| packages: { | ||
| "b.txt": "content", | ||
| }, | ||
| other: { | ||
| "c.txt": "content", | ||
| }, | ||
| }); | ||
|
|
||
| const glob = new Glob("{apps,packages}/**"); | ||
| const entries = await Array.fromAsync(glob.scan({ cwd: tempdir, onlyFiles: false })); | ||
|
|
||
| expect(entries.length).toBeGreaterThan(0); | ||
| // should find both apps and packages directories/files | ||
| expect(entries.some(e => e.startsWith("apps"))).toBe(true); | ||
| expect(entries.some(e => e.startsWith("packages"))).toBe(true); | ||
| // should NOT find the "other" directory | ||
| expect(entries.some(e => e.startsWith("other"))).toBe(false); | ||
| }); | ||
|
|
||
| test("braces in first segment (sync)", () => { | ||
| const tempdir = tempDirWithFiles("glob-brace-first-sync", { | ||
| apps: { | ||
| "a.txt": "content", | ||
| }, | ||
| packages: { | ||
| "b.txt": "content", | ||
| }, | ||
| other: { | ||
| "c.txt": "content", | ||
| }, | ||
| }); | ||
|
|
||
| const glob = new Glob("{apps,packages}/**"); | ||
| const entries = Array.from(glob.scanSync({ cwd: tempdir, onlyFiles: false })); | ||
|
|
||
| expect(entries.length).toBeGreaterThan(0); | ||
| expect(entries.some(e => e.startsWith("apps"))).toBe(true); | ||
| expect(entries.some(e => e.startsWith("packages"))).toBe(true); | ||
| expect(entries.some(e => e.startsWith("other"))).toBe(false); | ||
| }); | ||
|
|
||
| test("braces with only files option", async () => { | ||
| const tempdir = tempDirWithFiles("glob-brace-onlyfiles", { | ||
| src: { | ||
| "index.ts": "content", | ||
| lib: { | ||
| "helper.ts": "content", | ||
| }, | ||
| }, | ||
| test: { | ||
| "index.test.ts": "content", | ||
| }, | ||
| }); | ||
|
|
||
| const glob = new Glob("{src,test}/**/*.ts"); | ||
| const entries = await Array.fromAsync(glob.scan({ cwd: tempdir, onlyFiles: true })); | ||
|
|
||
| expect(entries.length).toBe(3); | ||
| expect(entries.sort()).toEqual( | ||
| [`src${path.sep}index.ts`, `src${path.sep}lib${path.sep}helper.ts`, `test${path.sep}index.test.ts`].sort(), | ||
| ); | ||
| }); | ||
|
|
||
| test("single alternative in braces (edge case)", async () => { | ||
| const tempdir = tempDirWithFiles("glob-brace-single", { | ||
| apps: { | ||
| "a.txt": "content", | ||
| }, | ||
| packages: { | ||
| "b.txt": "content", | ||
| }, | ||
| }); | ||
|
|
||
| // single alternative should still work | ||
| const glob = new Glob("{apps}/**"); | ||
| const entries = await Array.fromAsync(glob.scan({ cwd: tempdir, onlyFiles: false })); | ||
|
|
||
| expect(entries.length).toBeGreaterThan(0); | ||
| expect(entries.some(e => e.startsWith("apps"))).toBe(true); | ||
| expect(entries.some(e => e.startsWith("packages"))).toBe(false); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider making these tests concurrent.
These tests create separate temporary directories and don't share state, so they can safely run concurrently. Using test.concurrent would improve test suite performance.
🔎 Proposed refactor to use concurrent tests
// brace alternation without path separators (handled by normal matching)
describe("brace alternation (basic)", () => {
- test("braces in first segment", async () => {
+ test.concurrent("braces in first segment", async () => {
const tempdir = tempDirWithFiles("glob-brace-first", {
apps: {
"a.txt": "content",
},
packages: {
"b.txt": "content",
},
other: {
"c.txt": "content",
},
});
const glob = new Glob("{apps,packages}/**");
const entries = await Array.fromAsync(glob.scan({ cwd: tempdir, onlyFiles: false }));
expect(entries.length).toBeGreaterThan(0);
// should find both apps and packages directories/files
expect(entries.some(e => e.startsWith("apps"))).toBe(true);
expect(entries.some(e => e.startsWith("packages"))).toBe(true);
// should NOT find the "other" directory
expect(entries.some(e => e.startsWith("other"))).toBe(false);
});
- test("braces in first segment (sync)", () => {
+ test.concurrent("braces in first segment (sync)", () => {
const tempdir = tempDirWithFiles("glob-brace-first-sync", {
apps: {
"a.txt": "content",
},
packages: {
"b.txt": "content",
},
other: {
"c.txt": "content",
},
});
const glob = new Glob("{apps,packages}/**");
const entries = Array.from(glob.scanSync({ cwd: tempdir, onlyFiles: false }));
expect(entries.length).toBeGreaterThan(0);
expect(entries.some(e => e.startsWith("apps"))).toBe(true);
expect(entries.some(e => e.startsWith("packages"))).toBe(true);
expect(entries.some(e => e.startsWith("other"))).toBe(false);
});
- test("braces with only files option", async () => {
+ test.concurrent("braces with only files option", async () => {
const tempdir = tempDirWithFiles("glob-brace-onlyfiles", {
src: {
"index.ts": "content",
lib: {
"helper.ts": "content",
},
},
test: {
"index.test.ts": "content",
},
});
const glob = new Glob("{src,test}/**/*.ts");
const entries = await Array.fromAsync(glob.scan({ cwd: tempdir, onlyFiles: true }));
expect(entries.length).toBe(3);
expect(entries.sort()).toEqual(
[`src${path.sep}index.ts`, `src${path.sep}lib${path.sep}helper.ts`, `test${path.sep}index.test.ts`].sort(),
);
});
- test("single alternative in braces (edge case)", async () => {
+ test.concurrent("single alternative in braces (edge case)", async () => {
const tempdir = tempDirWithFiles("glob-brace-single", {
apps: {
"a.txt": "content",
},
packages: {
"b.txt": "content",
},
});
// single alternative should still work
const glob = new Glob("{apps}/**");
const entries = await Array.fromAsync(glob.scan({ cwd: tempdir, onlyFiles: false }));
expect(entries.length).toBeGreaterThan(0);
expect(entries.some(e => e.startsWith("apps"))).toBe(true);
expect(entries.some(e => e.startsWith("packages"))).toBe(false);
});
});Based on coding guidelines: "Prefer concurrent tests over sequential tests using test.concurrent or describe.concurrent when multiple tests spawn processes or write files."
🤖 Prompt for AI Agents
test/js/bun/glob/scan.test.ts lines 818-906: these tests are independent (each
creates its own temp dir) and should be run in parallel to speed up the suite;
change the block to run concurrently by either converting the describe to
describe.concurrent("brace alternation (basic)", ...) or prefixing each test
with test.concurrent instead of test, and keep async tests returning promises
as-is; verify the sync test remains safe to run concurrently (it only uses its
own temp dir) before committing.
| describe("brace alternation with path separators", () => { | ||
| test("nested braces with path separators", async () => { | ||
| const tempdir = tempDirWithFiles("glob-brace-nested", { | ||
| apps: { | ||
| api: { | ||
| "config.yaml": "content", | ||
| }, | ||
| other: { | ||
| "skip.txt": "content", | ||
| }, | ||
| }, | ||
| packages: { | ||
| utils: { | ||
| "index.ts": "content", | ||
| }, | ||
| common: { | ||
| "helpers.ts": "content", | ||
| }, | ||
| core: { | ||
| "types.ts": "content", | ||
| }, | ||
| unrelated: { | ||
| "skip.txt": "content", | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| const glob = new Glob("{apps/api,packages/{utils,common,core}}/**"); | ||
| const entries = await Array.fromAsync(glob.scan({ cwd: tempdir, onlyFiles: false })); | ||
|
|
||
| expect(entries.length).toBeGreaterThan(0); | ||
| // should find files in the specified paths | ||
| expect(entries.some(e => e.startsWith(`apps${path.sep}api`))).toBe(true); | ||
| expect(entries.some(e => e.startsWith(`packages${path.sep}utils`))).toBe(true); | ||
| expect(entries.some(e => e.startsWith(`packages${path.sep}common`))).toBe(true); | ||
| expect(entries.some(e => e.startsWith(`packages${path.sep}core`))).toBe(true); | ||
| // should NOT find files in unrelated paths | ||
| expect(entries.some(e => e.startsWith(`apps${path.sep}other`))).toBe(false); | ||
| expect(entries.some(e => e.startsWith(`packages${path.sep}unrelated`))).toBe(false); | ||
| }); | ||
|
|
||
| test("nested braces with path separators (sync)", () => { | ||
| const tempdir = tempDirWithFiles("glob-brace-nested-sync", { | ||
| apps: { | ||
| api: { | ||
| "config.yaml": "content", | ||
| }, | ||
| other: { | ||
| "skip.txt": "content", | ||
| }, | ||
| }, | ||
| packages: { | ||
| utils: { | ||
| "index.ts": "content", | ||
| }, | ||
| common: { | ||
| "helpers.ts": "content", | ||
| }, | ||
| core: { | ||
| "types.ts": "content", | ||
| }, | ||
| unrelated: { | ||
| "skip.txt": "content", | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| const glob = new Glob("{apps/api,packages/{utils,common,core}}/**"); | ||
| const entries = Array.from(glob.scanSync({ cwd: tempdir, onlyFiles: false })); | ||
|
|
||
| expect(entries.length).toBeGreaterThan(0); | ||
| expect(entries.some(e => e.startsWith(`apps${path.sep}api`))).toBe(true); | ||
| expect(entries.some(e => e.startsWith(`packages${path.sep}utils`))).toBe(true); | ||
| expect(entries.some(e => e.startsWith(`packages${path.sep}common`))).toBe(true); | ||
| expect(entries.some(e => e.startsWith(`packages${path.sep}core`))).toBe(true); | ||
| expect(entries.some(e => e.startsWith(`apps${path.sep}other`))).toBe(false); | ||
| expect(entries.some(e => e.startsWith(`packages${path.sep}unrelated`))).toBe(false); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider making these tests concurrent.
Similar to the previous test suite, these tests create separate temporary directories and can safely run concurrently.
🔎 Proposed refactor to use concurrent tests
// brace alternation with path separators inside braces (requires expansion)
describe("brace alternation with path separators", () => {
- test("nested braces with path separators", async () => {
+ test.concurrent("nested braces with path separators", async () => {
const tempdir = tempDirWithFiles("glob-brace-nested", {
apps: {
api: {
"config.yaml": "content",
},
other: {
"skip.txt": "content",
},
},
packages: {
utils: {
"index.ts": "content",
},
common: {
"helpers.ts": "content",
},
core: {
"types.ts": "content",
},
unrelated: {
"skip.txt": "content",
},
},
});
const glob = new Glob("{apps/api,packages/{utils,common,core}}/**");
const entries = await Array.fromAsync(glob.scan({ cwd: tempdir, onlyFiles: false }));
expect(entries.length).toBeGreaterThan(0);
// should find files in the specified paths
expect(entries.some(e => e.startsWith(`apps${path.sep}api`))).toBe(true);
expect(entries.some(e => e.startsWith(`packages${path.sep}utils`))).toBe(true);
expect(entries.some(e => e.startsWith(`packages${path.sep}common`))).toBe(true);
expect(entries.some(e => e.startsWith(`packages${path.sep}core`))).toBe(true);
// should NOT find files in unrelated paths
expect(entries.some(e => e.startsWith(`apps${path.sep}other`))).toBe(false);
expect(entries.some(e => e.startsWith(`packages${path.sep}unrelated`))).toBe(false);
});
- test("nested braces with path separators (sync)", () => {
+ test.concurrent("nested braces with path separators (sync)", () => {
const tempdir = tempDirWithFiles("glob-brace-nested-sync", {
apps: {
api: {
"config.yaml": "content",
},
other: {
"skip.txt": "content",
},
},
packages: {
utils: {
"index.ts": "content",
},
common: {
"helpers.ts": "content",
},
core: {
"types.ts": "content",
},
unrelated: {
"skip.txt": "content",
},
},
});
const glob = new Glob("{apps/api,packages/{utils,common,core}}/**");
const entries = Array.from(glob.scanSync({ cwd: tempdir, onlyFiles: false }));
expect(entries.length).toBeGreaterThan(0);
expect(entries.some(e => e.startsWith(`apps${path.sep}api`))).toBe(true);
expect(entries.some(e => e.startsWith(`packages${path.sep}utils`))).toBe(true);
expect(entries.some(e => e.startsWith(`packages${path.sep}common`))).toBe(true);
expect(entries.some(e => e.startsWith(`packages${path.sep}core`))).toBe(true);
expect(entries.some(e => e.startsWith(`apps${path.sep}other`))).toBe(false);
expect(entries.some(e => e.startsWith(`packages${path.sep}unrelated`))).toBe(false);
});
});Based on coding guidelines: "Prefer concurrent tests over sequential tests using test.concurrent or describe.concurrent when multiple tests spawn processes or write files."
🤖 Prompt for AI Agents
In test/js/bun/glob/scan.test.ts around lines 909 to 987, the two tests creating
temp directories run sequentially but are safe to run in parallel; change them
to concurrent tests by either converting the describe to describe.concurrent or
prefixing each test with test.concurrent (e.g., test.concurrent("nested
braces...", ...) and test.concurrent("nested braces... (sync)", ...)); ensure no
shared mutable state remains between tests (they already use independent temp
directories) and run the suite to verify no race conditions.
|
Closing as stale: this PR predates the Rust rewrite. Every If the underlying change is still wanted, it will need to be redone against the current Rust/C++ tree. Apologies for the churn, and thank you for the contribution. |
previously, brace patterns like '{apps/api,packages/{utils,core}}/**' would fail because the walker couldn't determine which directories to descend into w/o expanding the braces first
this matches how node fs.glob() handles them
What does this PR do?
expands brace patterns that include path separators and then walks the resulting concrete patterns. simple brace alternation like
{a,b}/**is unchanged.How did you verify your code works?
added some tests for nested brace patterns with path separators, regression tests for simple braces, etc
ran this in an internal repo:
use this if you wanna try locally:
the 4 difference comes from existing directory inclusion differences between node and bun (node includes some directory entries that bun filters); lmk if you wanted to do something about this