Repository navigation
Add tests for returned promises by node:fs/promises - #22198
sosukesuzuki wants to merge 4 commits into
Conversation
|
Updated 1:58 AM PT - Sep 5th, 2025
❌ @Jarred-Sumner, your commit da66a89 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 22198That installs a local version of the PR into your bun-22198 --bun |
9e8e1b3 to
108fb8e
Compare
WalkthroughAdds a new test verifying Node’s fs.promises APIs return native Promise instances and expected shapes/results across file, directory, and descriptor operations, including negative cases and a regression for rm. No exported/public declarations are modified. Changes
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
test/js/node/fs/fs-promises-returns-promise.test.ts (7)
37-41: Future-proof deprecation: gate rmdir.fs.promises.rmdir is deprecated and may disappear. Only include this case if it exists.
Apply this diff:
// Operations that may fail but should still return Promise { name: "rm", fn: () => promises.rm(nonExistentFile), shouldFail: true }, - { name: "rmdir", fn: () => promises.rmdir(nonExistentFile), shouldFail: true }, + ...(typeof promises.rmdir === "function" + ? [{ name: "rmdir", fn: () => promises.rmdir(nonExistentFile), shouldFail: true }] + : []), { name: "readlink", fn: () => promises.readlink(testFile), shouldFail: true }, // Not a symlink
47-53: Make Promise checks robust across realms.constructor.name can be brittle; prefer tag-based checks that remain valid cross-realm.
Apply this diff at each occurrence:
- expect(result.constructor.name).toBe("Promise"); + expect(Object.prototype.toString.call(result)).toBe("[object Promise]");- expect(openResult.constructor.name).toBe("Promise"); + expect(Object.prototype.toString.call(openResult)).toBe("[object Promise]");- expect(existsResult1.constructor.name).toBe("Promise"); + expect(Object.prototype.toString.call(existsResult1)).toBe("[object Promise]");- expect(existsResult2.constructor.name).toBe("Promise"); + expect(Object.prototype.toString.call(existsResult2)).toBe("[object Promise]");- expect(rmResult.constructor.name).toBe("Promise"); + expect(Object.prototype.toString.call(rmResult)).toBe("[object Promise]");Also applies to: 74-77, 97-99, 103-105, 150-153
60-62: Preserve original error for debugging.Keep the original stack via Error.cause.
Apply this diff:
- } catch (error) { - throw new Error(`Test for ${test.name} failed: ${error}`); + } catch (error) { + throw new Error(`Test for ${test.name} failed`, { cause: error });
87-106: Optionally skip exists() on older environments.If exists is absent (older Node compatibility layers), skip the test to avoid false failures.
Apply this diff at the start of the test body:
it("should return Promise for exists() function", async () => { + if (typeof (promises as any).exists !== "function") { + // Not supported; skip + return; + }
118-125: Avoid fchown on Windows.fchown is typically unsupported on win32. Include it conditionally.
Apply this diff:
const fdTests = [ { name: "fstat", fn: () => promises.fstat(fileHandle.fd) }, { name: "fchmod", fn: () => promises.fchmod(fileHandle.fd, 0o644) }, - { name: "fchown", fn: () => promises.fchown(fileHandle.fd, process.getuid?.() ?? 0, process.getgid?.() ?? 0) }, + ...(process.platform === "win32" + ? [] + : [{ name: "fchown", fn: () => promises.fchown(fileHandle.fd, process.getuid?.() ?? 0, process.getgid?.() ?? 0) }]), { name: "fsync", fn: () => promises.fsync(fileHandle.fd) }, { name: "fdatasync", fn: () => promises.fdatasync(fileHandle.fd) }, { name: "ftruncate", fn: () => promises.ftruncate(fileHandle.fd, 5) },
127-136: Silence console noise in tests.Avoid console.warn in passing tests; it pollutes CI logs. Swallow expected failures.
Apply this diff:
- } catch (error) { - // Some operations might fail due to permissions, but should still return Promise - console.warn(`${test.name} failed (expected on some systems): ${error}`); - } + } catch { + // Some operations may fail depending on environment; acceptable. + }
143-158: Minor: avoid __dirname in ESM.For maximum ESM portability, prefer a tmp path (e.g., os.tmpdir()) for the nonexistent target instead of __dirname.
If desired, change:
const nonExistentFile = path.join(os.tmpdir(), `definitely-does-not-exist-${Date.now().toString(36)}-${Math.random().toString(16).slice(2)}`);(Requires: import os from "node:os")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- 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/node/fs/fs-promises-returns-promise.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Place tests in test/ and ensure filenames end with .test.ts or .test.tsx
Always use port: 0 in tests; never hardcode port numbers or use custom random-port functions
Prefer snapshot tests using normalizeBunSnapshot(...).toMatchInlineSnapshot(...) over exact string comparisons
Never write tests that assert absence of crashes (e.g., no 'panic' or 'uncaught exception' in output)
Avoid shell commands like find/grep in tests; use Bun.Glob and built-in tools instead
Files:
test/js/node/fs/fs-promises-returns-promise.test.ts
test/js/node/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Put Node.js compatibility tests under test/js/node/
Files:
test/js/node/fs/fs-promises-returns-promise.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Format JavaScript/TypeScript with Prettier (bun run prettier)
Files:
test/js/node/fs/fs-promises-returns-promise.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Use bun:test in files that end in *.test.ts
Prefer async/await over callbacks in tests
For single-callback flows in tests, use Promise.withResolvers()
Do not set timeouts on tests; rely on Bun’s built-in timeouts
Use tempDirWithFiles from harness to create temporary directories/files in tests
Organize tests with describe blocks
Always assert exit codes and error scenarios in tests (e.g., proc.exited, expect(...).not.toBe(0), toThrow())
Use describe.each for parameterized tests; toMatchSnapshot for snapshots; and beforeAll/afterEach/beforeEach for setup/teardown
Avoid flaky tests: never wait for arbitrary time; wait for conditions to be met
Files:
test/js/node/fs/fs-promises-returns-promise.test.ts
test/{**/*.test.ts,**/*-fixture.ts}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/{**/*.test.ts,**/*-fixture.ts}: When spawning Bun processes, use bunExe() and bunEnv from harness
Use using or await using for Bun APIs (e.g., Bun.spawn, Bun.listen, Bun.connect, Bun.serve) to ensure cleanup
Never hardcode port numbers; use port: 0 to get a random port
Use Buffer.alloc(count, fill).toString() instead of "A".repeat(count) for large/repetitive strings
Import testing utilities from harness (bunExe, bunEnv, tempDirWithFiles, tmpdirSync, isMacOS, isWindows, isPosix, gcTick, withoutAggressiveGC)
Files:
test/js/node/fs/fs-promises-returns-promise.test.ts
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/js/node/fs/fs-promises-returns-promise.test.ts
test/js/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place JavaScript and TypeScript tests under test/js/
Files:
test/js/node/fs/fs-promises-returns-promise.test.ts
test/js/node/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place Node.js module compatibility tests under test/js/node/, separated by module (e.g., test/js/node/assert/)
Files:
test/js/node/fs/fs-promises-returns-promise.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/js/node/fs/fs-promises-returns-promise.test.ts
🧠 Learnings (18)
📓 Common learnings
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/js/bun/**/*.{js,ts} : Place Bun API tests under test/js/bun/, separated by category (e.g., test/js/bun/glob/)
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/{dev/*.test.ts,dev-and-prod.ts} : Do not use node:fs APIs in tests; mutate files via dev.write, dev.patch, and dev.delete
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/js/node/**/*.{js,ts} : Place Node.js module compatibility tests under test/js/node/, separated by module (e.g., test/js/node/assert/)
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-30T00:05:38.001Z
Learning: Applies to test/js/node/**/*.test.{ts,tsx} : Put Node.js compatibility tests under test/js/node/
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-30T00:05:38.001Z
Learning: Applies to test/**/*.test.{ts,tsx} : Avoid shell commands like find/grep in tests; use Bun.Glob and built-in tools instead
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/{dev/*.test.ts,dev-and-prod.ts} : Do not use node:fs APIs in tests; mutate files via dev.write, dev.patch, and dev.delete
Applied to files:
test/js/node/fs/fs-promises-returns-promise.test.ts
📚 Learning: 2025-08-30T00:05:38.001Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-30T00:05:38.001Z
Learning: Applies to test/js/node/**/*.test.{ts,tsx} : Put Node.js compatibility tests under test/js/node/
Applied to files:
test/js/node/fs/fs-promises-returns-promise.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/js/node/**/*.{js,ts} : Place Node.js module compatibility tests under test/js/node/, separated by module (e.g., test/js/node/assert/)
Applied to files:
test/js/node/fs/fs-promises-returns-promise.test.ts
📚 Learning: 2025-08-30T00:07:13.641Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-08-30T00:07:13.641Z
Learning: Applies to test/**/*.test.ts : Use tempDirWithFiles from harness to create temporary directories/files in tests
Applied to files:
test/js/node/fs/fs-promises-returns-promise.test.ts
📚 Learning: 2025-08-30T00:05:38.001Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-30T00:05:38.001Z
Learning: Applies to test/regression/issue/**/*.test.{ts,tsx} : Create regression tests under test/regression/issue/ (one per bug fix)
Applied to files:
test/js/node/fs/fs-promises-returns-promise.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/js/third_party/**/*.{js,ts} : Place third-party npm package tests under test/js/third_party/
Applied to files:
test/js/node/fs/fs-promises-returns-promise.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/**/*.{js,ts} : Use shared utilities from test/harness.ts where applicable
Applied to files:
test/js/node/fs/fs-promises-returns-promise.test.ts
📚 Learning: 2025-08-30T00:07:13.641Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-08-30T00:07:13.641Z
Learning: Applies to test/**/*-fixture.ts : Name spawned script files with the suffix -fixture.ts
Applied to files:
test/js/node/fs/fs-promises-returns-promise.test.ts
📚 Learning: 2025-08-30T00:07:13.641Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-08-30T00:07:13.641Z
Learning: Applies to test/**/*.test.ts : Prefer async/await over callbacks in tests
Applied to files:
test/js/node/fs/fs-promises-returns-promise.test.ts
📚 Learning: 2025-08-30T00:07:13.641Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-08-30T00:07:13.641Z
Learning: Applies to test/{**/*.test.ts,**/*-fixture.ts} : Import testing utilities from harness (bunExe, bunEnv, tempDirWithFiles, tmpdirSync, isMacOS, isWindows, isPosix, gcTick, withoutAggressiveGC)
Applied to files:
test/js/node/fs/fs-promises-returns-promise.test.ts
📚 Learning: 2025-08-30T00:07:13.641Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-08-30T00:07:13.641Z
Learning: Applies to test/{**/*.test.ts,**/*-fixture.ts} : Use using or await using for Bun APIs (e.g., Bun.spawn, Bun.listen, Bun.connect, Bun.serve) to ensure cleanup
Applied to files:
test/js/node/fs/fs-promises-returns-promise.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/cli/**/*.{js,ts} : When testing Bun as a CLI, use spawn with bunExe() and bunEnv from harness, and capture stdout/stderr via pipes
Applied to files:
test/js/node/fs/fs-promises-returns-promise.test.ts
📚 Learning: 2025-08-30T00:05:38.001Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-30T00:05:38.001Z
Learning: Applies to test/**/*.test.{ts,tsx} : Avoid shell commands like find/grep in tests; use Bun.Glob and built-in tools instead
Applied to files:
test/js/node/fs/fs-promises-returns-promise.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/**/*.{js,ts} : Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Applied to files:
test/js/node/fs/fs-promises-returns-promise.test.ts
📚 Learning: 2025-08-30T00:07:13.641Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-08-30T00:07:13.641Z
Learning: Applies to test/{**/*.test.ts,**/*-fixture.ts} : When spawning Bun processes, use bunExe() and bunEnv from harness
Applied to files:
test/js/node/fs/fs-promises-returns-promise.test.ts
📚 Learning: 2025-08-30T00:05:38.001Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-30T00:05:38.001Z
Learning: Applies to test/js/bun/**/*.test.{ts,tsx} : Put Bun-specific API tests under test/js/bun/
Applied to files:
test/js/node/fs/fs-promises-returns-promise.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/js/bun/**/*.{js,ts} : Place Bun API tests under test/js/bun/, separated by category (e.g., test/js/bun/glob/)
Applied to files:
test/js/node/fs/fs-promises-returns-promise.test.ts
🧬 Code graph analysis (1)
test/js/node/fs/fs-promises-returns-promise.test.ts (1)
test/harness.ts (1)
tempDirWithFiles(258-265)
🔇 Additional comments (3)
test/js/node/fs/fs-promises-returns-promise.test.ts (3)
1-5: Good placement and harness usage.Node-compat tests are correctly under test/js/node/fs/, use bun:test, and leverage tempDirWithFiles. Formatting looks consistent.
160-187: Function-name assertions look good.Reasonable guard to prevent leaking internal names like defaultAsync.
1-189: Manual verification required: confirm fs-promises-returns-promise test passes under Bun
The provided script cannot execute here (bun: command not found). Please runbun test test/js/node/fs/fs-promises-returns-promise.test.tsin an environment with Bun installed to ensure the test passes.
This branch has not been deployed
3 participants
What does this PR do?
Add tests for oven-sh/WebKit#106
How did you verify your code works?
N/A