-
Notifications
You must be signed in to change notification settings - Fork 3.8k
fix: $.cwd('.') no longer throws "undefined" path error
#25588
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
base: main
Are you sure you want to change the base?
Conversation
The `defaultCwd` variable was `undefined`, causing `.cwd('.')` to pass
`undefined` to `setCwd()`, which the Zig code converted to the literal
string "undefined" and appended to the path.
Fix by removing the unused `defaultCwd` variable and using empty string
instead. Empty string is handled correctly by the Zig code - it joins
with the current cwd, effectively meaning "use process.cwd()".
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
|
Updated 7:39 PM PT - Dec 18th, 2025
@dylan-conway, your commit 7756307 is building: |
$\...\.cwd('.') no longer throws "undefined" path error$\pwd\.cwd('.') no longer throws "undefined" path error
$\pwd\.cwd('.') no longer throws "undefined" path error$pwd.cwd('.') no longer throws "undefined" path error
$pwd.cwd('.') no longer throws "undefined" path error$pwd.cwd('.') no longer throws "undefined" path error
$pwd.cwd('.') no longer throws "undefined" path error$.cwd('.') no longer throws "undefined" path error
WalkthroughShell cwd handling changed to treat undefined, ".", "", and "./" as an empty-string cwd (current directory) and removed the initial defaultCwd. Tests were added to verify cwd behavior, resets, and interactions with process.cwd(). Changes
Suggested reviewers
Pre-merge checks✅ Passed checks (4 passed)
Comment |
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.
Actionable comments posted: 1
📜 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/js/builtins/shell.ts(2 hunks)test/js/bun/shell/bunshell.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
test/**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts,jsx,tsx}: Write tests as JavaScript and TypeScript files using Jest-style APIs (test,describe,expect) and import frombun:test
Usetest.eachand data-driven tests to reduce boilerplate when testing multiple similar cases
Files:
test/js/bun/shell/bunshell.test.ts
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/shell/bunshell.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/shell/bunshell.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/shell/bunshell.test.ts
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}: Use.$call()and.$apply()instead of.call()and.apply()to prevent user tampering with function invocation
Use string literalrequire()statements only; dynamic requires are not permitted
Export modules usingexport default { ... }syntax; modules are NOT ES modules
Use JSC intrinsics (prefixed with$) such as$Array.from(),$isCallable(), and$newArrayWithSize()for performance-critical operations
Use private globals and methods with$prefix (e.g.,$Array,map.$set()) instead of public JavaScript globals
Use$debug()for debug logging and$assert()for assertions; both are stripped in release builds
Validate function arguments using validators frominternal/validatorsand throw$ERR_*error codes for invalid arguments
Useprocess.platformandprocess.archfor platform detection; these values are inlined and dead-code eliminated at build time
Files:
src/js/builtins/shell.ts
src/js/{builtins,node,bun,thirdparty,internal}/**/*.ts
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
Builtin functions must include
thisparameter typing in TypeScript to enable direct method binding in C++
Files:
src/js/builtins/shell.ts
🧠 Learnings (24)
📚 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/shell/bunshell.test.ts
📚 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/shell/bunshell.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/shell/bunshell.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} : 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:
test/js/bun/shell/bunshell.test.tssrc/js/builtins/shell.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) : Avoid shell commands like `find` or `grep` in tests - use Bun's Glob and built-in tools instead
Applied to files:
test/js/bun/shell/bunshell.test.ts
📚 Learning: 2025-10-19T02:44:46.354Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/context-propagation.test.ts:1-1
Timestamp: 2025-10-19T02:44:46.354Z
Learning: In the Bun repository, standalone packages under packages/ (e.g., bun-vscode, bun-inspector-protocol, bun-plugin-yaml, bun-plugin-svelte, bun-debug-adapter-protocol, bun-otel) co-locate their tests with package source code using *.test.ts files. This follows standard npm/monorepo patterns. The test/ directory hierarchy (test/js/bun/, test/cli/, test/js/node/) is reserved for testing Bun's core runtime APIs and built-in functionality, not standalone packages.
Applied to files:
test/js/bun/shell/bunshell.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/shell/bunshell.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/shell/bunshell.test.ts
📚 Learning: 2025-11-24T18:35:50.422Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:50.422Z
Learning: Applies to test/cli/**/*.{js,ts,jsx,tsx} : When testing Bun as a CLI, use the `spawn` API from `bun` with the `bunExe()` and `bunEnv` from `harness` to execute Bun commands and validate exit codes, stdout, and stderr
Applied to files:
test/js/bun/shell/bunshell.test.tssrc/js/builtins/shell.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) : Verify tests fail with `USE_SYSTEM_BUN=1 bun test <file>` and pass with `bun bd test <file>` - tests are invalid if they pass with USE_SYSTEM_BUN=1
Applied to files:
test/js/bun/shell/bunshell.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) : For single-file tests, prefer `-e` flag over `tempDir`
Applied to files:
test/js/bun/shell/bunshell.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 `tempDir` from `harness` to create temporary directories - do not use `tmpdirSync` or `fs.mkdtempSync`
Applied to files:
test/js/bun/shell/bunshell.test.ts
📚 Learning: 2025-11-24T18:35:50.422Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:50.422Z
Learning: Tests should be located in the `test/` directory, organized by category: `cli/` for CLI tests, `js/` for JavaScript/TypeScript tests (further subdivided into `bun/`, `node/`, `web/`, `third_party/`), `napi/` for N-API tests, `v8/` for V8 C++ API tests, `bundler/` for bundler/transpiler tests, and `regression/issue/[number]` for regression tests
Applied to files:
test/js/bun/shell/bunshell.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) : When spawning processes in tests, expect stdout before expecting exit code for more useful error messages on test failure
Applied to files:
test/js/bun/shell/bunshell.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} : Always use `await using` or `using` to ensure proper resource cleanup in tests for APIs like Bun.listen, Bun.connect, Bun.spawn, Bun.serve, etc
Applied to files:
test/js/bun/shell/bunshell.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} : Always check exit codes and test error scenarios in error tests
Applied to files:
test/js/bun/shell/bunshell.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: Unit tests for specific features are organized by module (e.g., `/test/js/bun/`, `/test/js/node/`)
Applied to files:
test/js/bun/shell/bunshell.test.ts
📚 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: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Export modules using `export default { ... }` syntax; modules are NOT ES modules
Applied to files:
src/js/builtins/shell.ts
📚 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: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use private globals and methods with `$` prefix (e.g., `$Array`, `map.$set()`) instead of public JavaScript globals
Applied to files:
src/js/builtins/shell.ts
📚 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: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use `process.platform` and `process.arch` for platform detection; these values are inlined and dead-code eliminated at build time
Applied to files:
src/js/builtins/shell.ts
📚 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: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use `.$call()` and `.$apply()` instead of `.call()` and `.apply()` to prevent user tampering with function invocation
Applied to files:
src/js/builtins/shell.ts
📚 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/js/builtins/shell.ts
📚 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/js/builtins/shell.ts
📚 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/js/builtins/shell.ts
🧬 Code graph analysis (1)
test/js/bun/shell/bunshell.test.ts (2)
scripts/utils.mjs (1)
$(143-161)test/js/node/util/parse_args/default-args.test.mjs (1)
temp_dir(9-9)
🔇 Additional comments (5)
src/js/builtins/shell.ts (2)
149-156: LGTM! Clear normalization logic for current directory representations.The normalization of
.,./,"", andundefinedto an empty string is correct and addresses the issue where these values previously resulted in "undefined" path errors. The empty string is then passed to the Zig layer viasetCwd(""), which correctly interprets it as "use process.cwd()".
276-288: Verify consistent behavior between instance and global cwd handling.The normalization logic here is correct, but note that when
$.cwd('.')is called, it setscwdSymbolto"". Later, when creating a shell command (lines 310, 330), the checkif (cwd)evaluates to false (empty string is falsy), sosetCwdis NOT called.This differs from the instance method
ShellPromise.cwd()which explicitly callssetCwd(""). Both code paths should work correctly if:
- Not calling
setCwddefaults toprocess.cwd()- Calling
setCwd("")explicitly usesprocess.cwd()The tests verify this behavior works, including the reset scenario (
$.cwd(temp_dir)then$.cwd('.')to reset). However, this subtle difference in implementation could be confusing.Consider adding a comment explaining that empty string in
cwdSymbolis intentionally falsy to skip thesetCwdcall, relying on default behavior.Based on learnings, uses
process.platformfor platform detection which gets inlined at build time.test/js/bun/shell/bunshell.test.ts (3)
810-816: LGTM! Good test coverage for cwd reset behavior.This test correctly verifies that calling
.cwd('.')after.cwd(temp_dir)resets to the current working directory. This is an important edge case that confirms the fix works for chained calls.
818-822: LGTM! Verifies global cwd setter behavior.This test correctly verifies that
$.cwd('.')sets the default cwd to the current working directory for subsequent commands.
824-837: LGTM! Proper cleanup of global state.This test correctly verifies the reset behavior when using the global
$.cwd()setter. The test properly cleans up by resetting toprocess.cwd()at the end (lines 834-836), which prevents interference with other tests.Since this test modifies global state (
$.cwd()), it's important that it doesn't run concurrently with other tests. This appears safe as the surroundingdescribe("cd & pwd")block doesn't usedescribe.concurrent.As per coding guidelines, when tests modify shared state, avoid
test.concurrentunless the tests are properly isolated.
| test(".cwd('.') uses current working directory", async () => { | ||
| const result = await $`pwd`.cwd(".").text(); | ||
| expect(result.trim()).toBe(process.cwd()); | ||
| }); | ||
|
|
||
| test(".cwd('') uses current working directory", async () => { | ||
| const result = await $`pwd`.cwd("").text(); | ||
| expect(result.trim()).toBe(process.cwd()); | ||
| }); | ||
|
|
||
| test(".cwd('./') uses current working directory", async () => { | ||
| const result = await $`pwd`.cwd("./").text(); | ||
| expect(result.trim()).toBe(process.cwd()); | ||
| }); | ||
|
|
||
| test(".cwd(undefined) uses current working directory", async () => { | ||
| const result = await $`pwd`.cwd(undefined).text(); | ||
| expect(result.trim()).toBe(process.cwd()); | ||
| }); |
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.
🧹 Nitpick | 🔵 Trivial
Consider using test.each to reduce test duplication.
These four tests follow an identical pattern and could be consolidated using test.each for better maintainability:
Suggested refactor
- test(".cwd('.') uses current working directory", async () => {
- const result = await $`pwd`.cwd(".").text();
- expect(result.trim()).toBe(process.cwd());
- });
-
- test(".cwd('') uses current working directory", async () => {
- const result = await $`pwd`.cwd("").text();
- expect(result.trim()).toBe(process.cwd());
- });
-
- test(".cwd('./') uses current working directory", async () => {
- const result = await $`pwd`.cwd("./").text();
- expect(result.trim()).toBe(process.cwd());
- });
-
- test(".cwd(undefined) uses current working directory", async () => {
- const result = await $`pwd`.cwd(undefined).text();
- expect(result.trim()).toBe(process.cwd());
- });
+ test.each([
+ { value: ".", description: "'.'" },
+ { value: "", description: "''" },
+ { value: "./", description: "'./'" },
+ { value: undefined, description: "undefined" },
+ ])(".cwd($description) uses current working directory", async ({ value }) => {
+ const result = await $`pwd`.cwd(value).text();
+ expect(result.trim()).toBe(process.cwd());
+ });As per coding guidelines, use test.each and data-driven tests to reduce boilerplate when testing multiple similar cases.
📝 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.
| test(".cwd('.') uses current working directory", async () => { | |
| const result = await $`pwd`.cwd(".").text(); | |
| expect(result.trim()).toBe(process.cwd()); | |
| }); | |
| test(".cwd('') uses current working directory", async () => { | |
| const result = await $`pwd`.cwd("").text(); | |
| expect(result.trim()).toBe(process.cwd()); | |
| }); | |
| test(".cwd('./') uses current working directory", async () => { | |
| const result = await $`pwd`.cwd("./").text(); | |
| expect(result.trim()).toBe(process.cwd()); | |
| }); | |
| test(".cwd(undefined) uses current working directory", async () => { | |
| const result = await $`pwd`.cwd(undefined).text(); | |
| expect(result.trim()).toBe(process.cwd()); | |
| }); | |
| test.each([ | |
| { value: ".", description: "'.'" }, | |
| { value: "", description: "''" }, | |
| { value: "./", description: "'./'" }, | |
| { value: undefined, description: "undefined" }, | |
| ])(".cwd($description) uses current working directory", async ({ value }) => { | |
| const result = await $`pwd`.cwd(value).text(); | |
| expect(result.trim()).toBe(process.cwd()); | |
| }); |
🤖 Prompt for AI Agents
In test/js/bun/shell/bunshell.test.ts around lines 790 to 808, four nearly
identical tests verify that different cwd inputs resolve to the current working
directory; replace them with a single data-driven test using test.each with the
inputs [".", "", "./", undefined] so the same assertion runs for each value,
calling $`pwd`.cwd(input).text(), trimming the result and expecting it to equal
process.cwd(); ensure the test name includes the parameter for clarity and
maintain existing async handling.
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.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/js/bun/shell/bunshell.test.ts (1)
790-809: Consider usingtest.eachto reduce test duplication.These four tests follow an identical pattern and could be consolidated using
test.eachfor better maintainability, as suggested in the previous review.As per coding guidelines, use
test.eachand data-driven tests to reduce boilerplate when testing multiple similar cases.
📜 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 (1)
test/js/bun/shell/bunshell.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
test/**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts,jsx,tsx}: Write tests as JavaScript and TypeScript files using Jest-style APIs (test,describe,expect) and import frombun:test
Usetest.eachand data-driven tests to reduce boilerplate when testing multiple similar cases
Files:
test/js/bun/shell/bunshell.test.ts
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/shell/bunshell.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/shell/bunshell.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/shell/bunshell.test.ts
🧠 Learnings (21)
📓 Common learnings
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
📚 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/shell/bunshell.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) : For multi-file tests, prefer `tempDir` and `Bun.spawn` over single-file tests
Applied to files:
test/js/bun/shell/bunshell.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} : 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:
test/js/bun/shell/bunshell.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/shell/bunshell.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) : Avoid shell commands like `find` or `grep` in tests - use Bun's Glob and built-in tools instead
Applied to files:
test/js/bun/shell/bunshell.test.ts
📚 Learning: 2025-10-19T02:44:46.354Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/context-propagation.test.ts:1-1
Timestamp: 2025-10-19T02:44:46.354Z
Learning: In the Bun repository, standalone packages under packages/ (e.g., bun-vscode, bun-inspector-protocol, bun-plugin-yaml, bun-plugin-svelte, bun-debug-adapter-protocol, bun-otel) co-locate their tests with package source code using *.test.ts files. This follows standard npm/monorepo patterns. The test/ directory hierarchy (test/js/bun/, test/cli/, test/js/node/) is reserved for testing Bun's core runtime APIs and built-in functionality, not standalone packages.
Applied to files:
test/js/bun/shell/bunshell.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/shell/bunshell.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/shell/bunshell.test.ts
📚 Learning: 2025-11-24T18:35:50.422Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:50.422Z
Learning: Applies to test/cli/**/*.{js,ts,jsx,tsx} : When testing Bun as a CLI, use the `spawn` API from `bun` with the `bunExe()` and `bunEnv` from `harness` to execute Bun commands and validate exit codes, stdout, and stderr
Applied to files:
test/js/bun/shell/bunshell.test.ts
📚 Learning: 2025-11-24T18:35:39.205Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-11-24T18:35:39.205Z
Learning: Add tests for new Bun runtime functionality
Applied to files:
test/js/bun/shell/bunshell.test.ts
📚 Learning: 2025-11-24T18:35:50.422Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:50.422Z
Learning: Applies to test/**/*.{js,ts,jsx,tsx} : Use `test.each` and data-driven tests to reduce boilerplate when testing multiple similar cases
Applied to files:
test/js/bun/shell/bunshell.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} : Always use `await using` or `using` to ensure proper resource cleanup in tests for APIs like Bun.listen, Bun.connect, Bun.spawn, Bun.serve, etc
Applied to files:
test/js/bun/shell/bunshell.test.ts
📚 Learning: 2025-11-06T00:58:23.965Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 24417
File: test/js/bun/spawn/spawn.test.ts:903-918
Timestamp: 2025-11-06T00:58:23.965Z
Learning: In Bun test files, `await using` with spawn() is appropriate for long-running processes that need guaranteed cleanup on scope exit or when explicitly testing disposal behavior. For short-lived processes that exit naturally (e.g., console.log scripts), the pattern `const proc = spawn(...); await proc.exited;` is standard and more common, as evidenced by 24 instances vs 4 `await using` instances in test/js/bun/spawn/spawn.test.ts.
Applied to files:
test/js/bun/shell/bunshell.test.ts
📚 Learning: 2025-11-07T17:28:51.204Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 24423
File: test/js/node/path/browserify.test.js:472-475
Timestamp: 2025-11-07T17:28:51.204Z
Learning: In test files under test/js/node/path/, markovejnovic prefers functional programming patterns (e.g., reduce, map, filter chains) over imperative loops for clarity and readability.
Applied to files:
test/js/bun/shell/bunshell.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) : For single-file tests, prefer `-e` flag over `tempDir`
Applied to files:
test/js/bun/shell/bunshell.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 `tempDir` from `harness` to create temporary directories - do not use `tmpdirSync` or `fs.mkdtempSync`
Applied to files:
test/js/bun/shell/bunshell.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) : When spawning processes in tests, expect stdout before expecting exit code for more useful error messages on test failure
Applied to files:
test/js/bun/shell/bunshell.test.ts
📚 Learning: 2025-11-24T18:35:50.422Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:50.422Z
Learning: Tests should be located in the `test/` directory, organized by category: `cli/` for CLI tests, `js/` for JavaScript/TypeScript tests (further subdivided into `bun/`, `node/`, `web/`, `third_party/`), `napi/` for N-API tests, `v8/` for V8 C++ API tests, `bundler/` for bundler/transpiler tests, and `regression/issue/[number]` for regression tests
Applied to files:
test/js/bun/shell/bunshell.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} : Always check exit codes and test error scenarios in error tests
Applied to files:
test/js/bun/shell/bunshell.test.ts
📚 Learning: 2025-11-24T18:35:08.612Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:08.612Z
Learning: Applies to test/bake/dev/esm.test.ts : Organize ESM tests in esm.test.ts for tests about various ESM features in development mode
Applied to files:
test/js/bun/shell/bunshell.test.ts
🧬 Code graph analysis (1)
test/js/bun/shell/bunshell.test.ts (2)
scripts/utils.mjs (1)
$(143-161)test/js/node/util/parse_args/default-args.test.mjs (1)
temp_dir(9-9)
🔇 Additional comments (2)
test/js/bun/shell/bunshell.test.ts (2)
811-817: LGTM!The chaining behavior test correctly verifies that
.cwd(temp_dir).cwd('.')resets to the current working directory.
825-838: LGTM!Excellent test with proper cleanup. The test correctly verifies the reset behavior and includes explicit cleanup to maintain test isolation, with a final assertion confirming the reset worked.
| test("$.cwd('.') sets default cwd to current working directory", async () => { | ||
| $.cwd("."); | ||
| const result = await $`pwd`.text(); | ||
| expect(result.trim()).toBe(process.cwd()); | ||
| }); |
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.
Potential test isolation issue: Global state not reset.
This test modifies the global $.cwd('.') but doesn't restore it afterward. If other tests depend on the initial $.cwd(process.cwd().replaceAll("\\", "https://github.com/")) set at line 34, they may fail when run after this test.
Consider adding cleanup to restore the original cwd state after the assertion.
🔎 Suggested fix
test("$.cwd('.') sets default cwd to current working directory", async () => {
$.cwd(".");
const result = await $`pwd`.text();
expect(result.trim()).toBe(process.cwd());
+ // Reset to original cwd for test isolation
+ $.cwd(process.cwd().replaceAll("\\", "https://github.com/"));
});📝 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.
| test("$.cwd('.') sets default cwd to current working directory", async () => { | |
| $.cwd("."); | |
| const result = await $`pwd`.text(); | |
| expect(result.trim()).toBe(process.cwd()); | |
| }); | |
| test("$.cwd('.') sets default cwd to current working directory", async () => { | |
| $.cwd("."); | |
| const result = await $`pwd`.text(); | |
| expect(result.trim()).toBe(process.cwd()); | |
| // Reset to original cwd for test isolation | |
| $.cwd(process.cwd().replaceAll("\\", "https://github.com/")); | |
| }); |
🤖 Prompt for AI Agents
In test/js/bun/shell/bunshell.test.ts around lines 819 to 823, the test mutates
global $.cwd('.') without restoring it; save the current value (the original
$.cwd or process.cwd().replaceAll("\\", "https://github.com/") as used at line 34) before calling
$.cwd("."), run the assertion, then restore the saved value in a finally block
(or use afterEach to reset) so global cwd state is returned to its original
value for subsequent tests.
Summary
Fixes #25579
.cwd('.'),.cwd(''),.cwd('./'), and.cwd(undefined)now correctly useprocess.cwd()instead of throwingNo such file or directory: .../undefineddefaultCwdvariable that was alwaysundefinedTest plan
.cwd('.'),.cwd(''),.cwd('./'),.cwd(undefined).cwd(temp_dir).cwd('.')to reset to current directory$.cwd('.')and$.cwd(temp_dir)then$.cwd('.')resetUSE_SYSTEM_BUN=1and pass withbun bd test🤖 Generated with Claude Code