chore(tests): consolidate regression tests into appropriate test files#26176
Closed
robobun wants to merge 1 commit into
Closed
chore(tests): consolidate regression tests into appropriate test files#26176robobun wants to merge 1 commit into
robobun wants to merge 1 commit into
Conversation
This commit reorganizes ~128 regression tests from test/regression/issue/ into their appropriate existing test files based on what they test: - HTTP server tests → test/js/bun/http/serve.test.ts - child_process tests → test/js/node/child_process/child_process.test.ts - fetch/body tests → test/js/web/fetch/body.test.ts - expect matchers → test/js/bun/test/expect.test.js - jest hooks → test/js/bun/test/jest-hooks.test.ts - CSS tests → test/js/bun/css/css.test.ts - S3 tests → test/js/bun/s3/s3.test.ts - Shell tests → test/js/bun/shell/bunshell.test.ts - TTY tests → test/js/node/tty.test.ts - Mock tests → test/js/bun/test/mock-fn.test.js - WebSocket tests → test/js/bun/websocket/websocket-server.test.ts - Bundler tests → test/bundler/*.test.ts - And many more... The remaining 86 tests in test/regression/issue/ are kept because they: - Test very specific edge cases worth keeping isolated - Don't have a clear existing test file to merge into - Were explicitly marked to keep during analysis All moved tests retain comments referencing their original issue numbers for traceability (e.g., "// Regression test for #XXXXX"). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Collaborator
Author
Contributor
WalkthroughThis pull request consolidates and expands regression test coverage across the Bun test suite. It adds extensive new regression tests for bundler operations, CLI commands, HTTP/WebSocket functionality, CSS handling, Node.js API compatibility, and web standards APIs. Concurrently, it removes individual issue-specific test files from the regression directory, consolidating them into their corresponding feature-specific test files. Changes
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 63
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/cli/install/bunx.test.ts (1)
728-764: PrefertempDirovertmpdirSyncfor the new tarball workspace.Line 728 introduces
tmpdirSync()for test temp dirs, which the repo guidance discourages. UsetempDir()withusingfor cleanup and consistency.♻️ Proposed fix
- const tempDirPath = tmpdirSync(); - const packageDir = join(tempDirPath, "package"); + using tempDirPath = tempDir("multi-tool-pkg", {}); + const packageDir = join(String(tempDirPath), "package");As per coding guidelines, prefer
tempDirfromharnessfor temporary test directories.
🤖 Fix all issues with AI agents
In `@test/bundler/bun-build-api.test.ts`:
- Around line 1380-1477: The tests currently spawn the compiler (compileProc)
but don't assert it succeeded; capture the compile exit code from
compileProc.exited after awaiting compilation and assert it's 0 before running
the produced binary (e.g., check const compileExit = await compileProc.exited;
expect(compileExit).toBe(0)); if non-zero, include compileProc.stderr.text() in
the failure message or log to aid debugging; apply this check in both places
where compileProc is created (the "22157-basic" and "22157-args" tests) so the
runProc only starts when compileProc succeeded.
In `@test/bundler/bundler_edgecase.test.ts`:
- Around line 2389-2405: The test "bun build --target bun should support
non-ascii source" sets global $.throws(true) which can leak into other tests;
wrap the body that calls bunExe() in a try/finally and restore $.throws to its
previous/default value (e.g., false) in the finally block so $.throws is reset
even on failures—modify the test around the $.throws(true) call and the awaits
for bunExe() and result retrieval to ensure cleanup.
In `@test/bundler/bundler_minify.test.ts`:
- Around line 1218-1225: The inline snapshot is using raw output from
api.readFile("/out.js") in the onAfterBundle(api) callback; update the test to
normalize output before snapshot comparison by calling normalizeBunSnapshot on
the string returned from api.readFile("/out.js") (keep the existing minifySyntax
and minifyWhitespace options), then pass the normalized value into
expect(...).toMatchInlineSnapshot so the snapshot is stable across platforms;
locate this change in the onAfterBundle function where api.readFile("/out.js")
is read and the expect is performed.
In `@test/cli/install/bun-update.test.ts`:
- Around line 491-705: Replace uses of tempDirWithFiles with the disposable
tempDir helper and wrap each Bun.spawn call in an await using block so the
temporary directory and spawned process are auto-cleaned; locate the tests that
create dirs via tempDirWithFiles (e.g., variables named dir) and where Bun.spawn
is called with cmd: [bunExe(), "update", ...], env, stdin/stdout/stderr, then
change to create the tempDir (tempDir(...)) and use "await using (const proc =
Bun.spawn(...))" (or the repo's canonical await-using pattern) to ensure both
the temp directory and the spawned process are disposed after the test.
- Around line 519-704: Tests currently assert absence of "panic"/"segfault"
strings in stderr (e.g., expect(stderr).not.toContain("panic") / "segfault")
which is brittle; remove those negative string checks and instead assert the
command completed successfully by checking the Bun.spawn result (e.g., assert
result.exitCode === 0 or use expect(result.exitCode).toBe(0)) and/or assert
specific expected output in stdout/stderr; update each test case that uses
Bun.spawn (the blocks creating result via Bun.spawn and reading stderr) to drop
the panic/segfault assertions and replace them with a stable success assertion
against result.exitCode (and any domain-specific expected text) while keeping
the other underflow/overflow checks if still desired.
In `@test/cli/install/bun-workspaces.test.ts`:
- Around line 1954-1989: The two Bun.spawnSync invocations in the "11806" test
(the variables result1 and result2 created inside the test that calls
Bun.spawnSync with cmd/bunExe and cwd set to dir + "/apps/api") omit the harness
environment, making installs non-hermetic; update both Bun.spawnSync calls to
pass the harness environment by adding the env: bunEnv option so the test uses
the isolated cache/tmp/registry settings provided by the test harness (keep
tempDirWithFiles and other args unchanged).
In `@test/cli/install/bunx.test.ts`:
- Around line 921-973: The assertions in the spawned-process tests check
exitCode before stdout, making failures harder to debug; in the tests that
create `proc` (the two blocks using Bun.spawn invoking `bun run` and `bunx`),
swap the order so you first await/inspect `stdout` and assert on its parsed
contents (the JSON.parse(stdout.trim()) expectations) and only after that assert
exitCode (variables: `proc`, `stdout`, `stderr`, `exitCode`, and use of
`Bun.spawn`), applying the same change to the other referenced block (around
lines with expectations for [""] and ["hello", "", "world"] and the similar
block at 1030-1039).
In `@test/cli/install/semver.test.ts`:
- Around line 740-744: Remove the duplicate standalone test "semver with
multiple tags work properly" that calls satisfies("3.4.5", ">=3.3.0-beta.1
<3.4.0-beta.3") since the same assertion already exists in the "range excludes"
section (the array case with [">=3.3.0-beta.1 <3.4.0-beta.3", "3.4.5"]) — delete
the test block containing that assertion to avoid redundancy and keep the single
existing test that references issue `#8040`.
In `@test/cli/run/run_command.test.ts`:
- Around line 96-106: The test "bun run sets cwd for script, matching npm"
mutates the global $.cwd and doesn't guarantee reset if an assertion throws;
wrap the body that changes $.cwd (calls to $.cwd(issue10132Dir) and
$.cwd(currentPwd2)) in a try/finally so that the original cwd (e.g., capture
const originalCwd = process.cwd() or prior $.cwd value) is restored in the
finally block via $.cwd(originalCwd) to avoid leaking state across tests; ensure
references to $.cwd and bunExe() remain unchanged and only the control flow
around them is updated.
- Around line 108-126: The test races because $.cwd(...) mutates shared state
inside the async helper run; replace the concurrent Promise.all([...].map(run))
call with a sequential iteration (e.g. a for/for-of loop) that awaits each
run(...) in order so $.cwd(dir) is not called concurrently; update the test
block containing the run function and the invocation that currently uses
Promise.all to iterate sequentially to ensure each invocation of $.cwd and the
subsequent $`${bunExe()} ...` calls run in the intended working directory.
In `@test/js/bun/cookie/cookie.test.ts`:
- Around line 463-465: The test passes a millisecond timestamp to Bun.Cookie
which expects seconds; update the expires argument used when constructing
futureCookie so it represents a Unix timestamp in seconds (e.g., current time in
seconds plus 86400 for one day) rather than Date.now() in milliseconds, ensuring
the Bun.Cookie("test","value", { expires: ... }) call and the subsequent
futureCookie.isExpired() assertion reflect a 1-day future expiry.
In `@test/js/bun/css/color.test.ts`:
- Around line 223-256: The test "CSS system colors in color-mix should not
crash" currently asserts on panic-style substrings in error.message; update the
assertions to avoid matching "panic"/"unreachable" strings by either asserting
that cssInternals._test(css, css) does not throw (use expect(() =>
cssInternals._test(...)).not.toThrow()) or, if a parse failure is acceptable,
assert on structured parse error metadata (e.g., error.name, error.code or an
instance check against the parser's ParseError type) instead of checking
error.message; locate the call to cssInternals._test and the try/catch block in
this test and replace the panic-string checks with one of these approaches.
In `@test/js/bun/css/css.test.ts`:
- Around line 7529-7533: Remove the brittle panic/unreachable string assertions:
delete the two expectations that check stderr for "panic" and "integer part of
floating point value out of bounds" and rely on the existing exit code assertion
(expect(exitCode).toBe(0)) or structured error metadata instead; update the same
pattern at the other occurrence around the expect-block at 7649-7656 so both
test blocks only assert exitCode (or structured error fields) and no longer
assert stderr string contents.
- Around line 7469-7516: Replace non-disposable tempDirWithFiles usage with the
disposable tempDir pattern so the temp directory is automatically cleaned up:
change the declaration using the symbol dir from "const dir =
tempDirWithFiles(...)" to use the disposable form "using dir = tempDir(...)"
(and provide the same fixture files to tempDir), and apply the same replacement
for the other fixture instance referenced at the second occurrence; ensure you
import/enable the using/tempDir helper if required so the test cleans up after
itself.
- Around line 7537-7583: Import normalizeBunSnapshot from the test harness and
wrap the normalized CSS before asserting: change the snapshot calls to use
normalizeBunSnapshot(normalizeCSSOutput(outputContent)) instead of just
normalizeCSSOutput(outputContent), and update both places where the inline
snapshot is asserted; ensure the harness import (normalizeBunSnapshot) is added
at the top of the file and referenced alongside the existing normalizeCSSOutput
helper.
In `@test/js/bun/http/bun-serve-static.test.ts`:
- Around line 204-210: The test defines a "/dynamic" route that is never
asserted; either remove the unused "/dynamic" entry from the routes object or
add assertions that exercise it (e.g., fetch "/dynamic" and assert the
status/body/headers match expectations and compare Unicode handling to the
"/static" route). Locate the routes definition (the routes: { "/dynamic": () =>
new Response("▲"), ... } block) and either delete the "/dynamic" property or add
test steps that issue an HTTP request to "/dynamic" and verify the response body
contains the expected character(s) and matches the static route behavior.
In `@test/js/bun/http/serve.test.ts`:
- Around line 2250-2258: Replace the weak assertion expect(true).toBe(true) with
the project-standard pass-through assertion expect().pass() in this test; locate
the assertion inside the try block (the section that says "If we get here
without crashing, the fix worked") and change it to expect().pass(), leaving the
surrounding finally cleanup that calls server.stop() intact.
- Around line 2261-2278: The test manually creates and stops the server with
Bun.serve(...) and try/finally; replace this with the file's established "using
server" pattern for automatic cleanup: create the server using the using server
= Bun.serve({...}) pattern, remove the try/finally and server.stop() call, and
keep the same assertions (fetch(server.url), response.text(), status check) so
the test behavior is unchanged while matching other tests' resource-management
style.
In `@test/js/bun/plugin/plugins.test.ts`:
- Around line 674-748: In both tests inside the "TypeScript syntax with 'ts'
loader in BunPlugin" block (the it blocks titled "issue `#12548`: TypeScript
syntax should work with 'ts' loader in BunPlugin" and "issue `#12548`: TypeScript
type imports work with 'ts' loader"), swap the assertion order so stdout is
asserted before exitCode: keep the Promise.all call that destructures [stdout,
stderr, exitCode] from proc.stdout.text(), proc.stderr.text(), and proc.exited,
but change the expectations to check stdout (e.g.,
expect(stdout).toContain(...)/expect(stdout).toBe(...)) and stderr first, then
assert expect(exitCode).toBe(0) last; update both test blocks that reference
proc, stdout, stderr, and exitCode accordingly.
- Around line 564-672: The assertions check result.exitCode before result.stdout
making failures harder to debug; for each test case in the "onResolve returning
undefined/null" describe block replace the two expects that reference the
spawnSync result so that you assert result.stdout (or result.stderr where
appropriate) first and then assert result.exitCode, i.e., locate the local
variable result from each Bun.spawnSync call and swap the order of the
expect(result.exitCode)... and expect(result.stdout/stderr)... assertions so
stdout/stderr is asserted before exitCode.
In `@test/js/bun/s3/s3.test.ts`:
- Around line 1553-1573: The test "S3 presigned URL should have correct query
parameter order" contains a stray console.log causing noisy output; remove the
console.log("Query parameters order:", params) statement inside that test
(relating to the s3.presign call and subsequent URL(url) and params extraction)
so the test runs quietly—if desired, replace it with a simple assertion on
params order or leave it out entirely.
In `@test/js/bun/shell/bunshell.test.ts`:
- Around line 10-20: Replace any use of tmpdirSync with the harness-provided
tempDir or tempDirWithFiles helpers: remove tmpdirSync from the import list and
import/use tempDir or tempDirWithFiles (from the existing harness import) to
create and manage temporary directories in the tests; update the test
setup/teardown code that calls tmpdirSync() to instead call tempDir() (or
tempDirWithFiles(...) if pre-populated files are needed) and use the returned
handle/path according to the harness API so the harness manages cleanup
consistently (search for tmpdirSync usage and update those callsites and the
import).
- Around line 2543-2564: The test "issue `#22650` - shell crash with && operator
followed by external command" spawns an external "node --version" which triggers
the regression; update the command passed to Bun.spawn so the external command
uses bunExe() instead of the literal "node" (i.e., replace the "node --version"
substring in the cmd argument for the proc created in this test with bunExe() +
" --version") so the test uses Bun's binary for the second command and remains
self-contained; keep the rest of the proc invocation (proc variable,
stdout/stderr checks, expectations) unchanged.
In `@test/js/bun/spawn/spawn.test.ts`:
- Around line 1054-1055: Remove the debug console.log lines that print the deep
path(s) and rely on the existing assertions instead: delete the
console.log(`Created deep path (length: ${deepPath.length}): ${deepPath}`) and
the similar log at the later occurrence (the one printing the other deep path),
keeping the expect(deepPath.length).toBeGreaterThanOrEqual(260) assertions
intact; this removes noisy test output while preserving the path length
verification.
- Around line 1057-1072: The test's intent is to accept either a successful
spawn or a graceful error, but the current assertion always expects an Error;
update the assertion logic around the Bun.spawn call (the `err` variable set in
the try/catch) so it accepts both outcomes — i.e., assert that either `err` is
undefined (spawn succeeded) or `err` is an instance of Error (spawn failed
gracefully). Locate the try/catch around `Bun.spawn({...}).exited`, the `err`
binding, and the `expect(err).toBeInstanceOf(Error)` assertion and replace it
with a check that validates `err === undefined || err instanceof Error`.
- Around line 1094-1117: The test currently expects errors but on Windows with
long path support the operations (Bun.write and Bun.spawn) can succeed; update
the assertions to verify no panic instead of forcing failure: for the Bun.write
block (the try/catch around Bun.write(join(deepPath, "test.js"), ...)) replace
expect(err).toBeInstanceOf(Error) with an assertion that err is undefined (e.g.,
expect(err).toBeUndefined()), and for the Bun.spawn block (the code creating
proc from Bun.spawn and awaiting proc.exited) similarly assert that err is
undefined after the try/catch (or assert exitCode === 0 when not caught) so the
test passes whether the operation succeeds or fails gracefully without
panicking; reference Bun.write, Bun.spawn, proc.exited, deepPath, and the err
variable when making the change.
In `@test/js/bun/test/bun-test.test.ts`:
- Around line 80-82: The test currently asserts exitCode before checking stdout
(see expect(exitCode) and normalizeBunSnapshot(stdout)); flip the assertions so
the stdout assertion
(expect(normalizeBunSnapshot(stdout)).toMatchInlineSnapshot(...)) runs before
expect(exitCode).toBe(0) to ensure stdout failures surface with better
diagnostics.
- Around line 39-52: Remove the manual hang-detection logic: delete the `let
timeout = false;` declaration, the `const timer = setTimeout(... 3000)` block
(including the `proc.kill()` call), the `clearTimeout(timer);` line, and the
`expect(timeout).toBeFalse();` assertion; keep the existing awaits for
`proc.stdout.text()`, `proc.stderr.text()`, and `proc.exited` so the test relies
on Bun's built-in test timeout instead of using `setTimeout` or manually killing
`proc`.
In `@test/js/bun/test/describe.test.ts`:
- Around line 278-314: Reorder the assertions so the normalized stderr snapshot
is asserted before checking the exit code: after spawning (result), first await
and store stderr via await result.stderr.text(), then call
expect(normalizeBunSnapshot(stderr)).toMatchInlineSnapshot(...) and only
afterwards await result.exited into exitCode and assert
expect(exitCode).toBe(0); update references to the existing variables result,
stderr, exitCode, and normalizeBunSnapshot in the test "issue `#19875`:
describe.only with nested describe.todo".
In `@test/js/bun/test/expect-extend.test.js`:
- Around line 397-406: Replace the inline assertion that checks
expect.toBeInTheDocument against undefined with a more readable matcher by using
.toBeDefined(); specifically, in the test block labeled "expect extended with
jest-dom matchers" (after importing matchers and calling expect.extend), change
the assertion on expect.toBeInTheDocument to use
expect(expect.toBeInTheDocument).toBeDefined() so the test reads more
idiomatically.
In `@test/js/bun/test/jest-each.test.ts`:
- Around line 64-76: The test currently awaits exitCode before checking
stderr/stdout which hides snapshot diffs on unexpected exits; reorder the
assertions in the "#11793 - test.each with nested empty array" test to read and
assert stderr (and stdout if needed) using the result.stdout.text() and
result.stderr.text() values (and normalizeBunSnapshot(stderr)) before awaiting
or asserting exitCode, keeping the same variables (result, stdout, stderr,
exitCode) and the normalizeBunSnapshot call so snapshot failures show diffs even
when the process exit is nonzero.
In `@test/js/bun/test/mock-fn.test.js`:
- Around line 1018-1072: The second describe block that calls mock.clearAllMocks
should be skipped when running under Jest/Vitest: guard the entire
describe("mock.clearAllMocks", ...) block with if (isBun) so it only runs when
isBun is truthy; this prevents calling mock.clearAllMocks when test-interop sets
mock to null (causing a TypeError). Locate the describe block that references
mock and mock.clearAllMocks and wrap it with an if (isBun) conditional (using
the existing isBun symbol from test setup).
In `@test/js/bun/test/mock/mock-module.test.ts`:
- Around line 16-58: The test currently calls bunRun(join(dir, "dir/a.ts")) but
doesn't assert its result; update the test to capture the return value (result =
await bunRun(...)) and assert that result.exitCode === 0 and that result.stdout
contains expected output (e.g., an empty line or a specific marker printed by
dir/a.ts) so failures show stdout for debugging; modify the test body around the
bunRun call in the "mock.module does not segfault with path aliases" test
(referencing tempDirWithFiles and bunRun) to await the process result and add
assertions on result.stdout before checking result.exitCode.
In `@test/js/bun/test/nested-describes.test.ts`:
- Around line 41-49: Replace the plain Bun.spawn usage with an "await using"
scoped resource so the spawned process is disposed even if assertions fail:
change the declaration of the spawn result (currently "const result =
Bun.spawn(...)") to use "await using" with the same variable name (result) when
calling Bun.spawn, then keep awaiting result.exited and reading
result.stdout.text() as before; this ensures Bun.spawn's resource is
automatically cleaned up if the test throws.
In `@test/js/bun/test/spyMatchers.test.ts`:
- Around line 1333-1336: The method a() contains an unreachable statement after
a throw; remove the trailing `return 1;` from the a() function (the method named
"a") so the throw is the last statement and the Biome `noUnreachable` lint error
is resolved.
In `@test/js/bun/test/test-only.test.ts`:
- Around line 28-50: Move the snapshot assertion for
normalizeBunSnapshot(stderr) to occur before the exit-code assertion so test
output diffs are shown first; specifically, in the "20092" test using variables
result, exitCode, stdout, stderr, call
expect(normalizeBunSnapshot(stderr)).toMatchInlineSnapshot(...) before
expect(exitCode).toBe(0) (preserve awaiting result.exited and reading
stdout/stderr as-is).
In `@test/js/bun/util/inspect-error.test.js`:
- Around line 214-218: The test currently asserts exitCode before checking
process output; change the assertion order in this test (and the other listed
blocks) to check stdout/stderr contents first and only then assert exitCode,
i.e., use the existing variables stdout, stderr and expect(...) calls so that
expect(stdout).toContain(...)/expect(stdout).not.toContain(...) and any stderr
assertions run before expect(exitCode).toBe(0); apply the same swap in the other
affected blocks referenced (around lines with similar
Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]) usage).
In `@test/js/bun/yaml/yaml.test.ts`:
- Around line 2760-2782: In the "YAML import with double-quoted strings
containing ... - issue `#23489`" test, move the expectations that inspect process
output (expect(stderr).not.toContain(...) and expect(stdout.trim()).toBe(...))
to run before the exit code assertion; specifically reorder the assertions that
reference stderr, stdout, and exitCode (variables proc, stderr, stdout,
exitCode) so you assert stderr and stdout first and then call
expect(exitCode).toBe(0), ensuring test output is checked before validating the
process exit status.
- Around line 2784-2807: The RSS-based leak assertion in the test "YAML.parse
shouldn't leak memory" is flaky; perform a warm-up parse and GC before capturing
initialMemory (e.g., call YAML.parse(yaml) once and Bun.gc(true) to stabilize
allocator) or increase the memory threshold; update the test around YAML.parse,
Bun.gc(true), initialMemory, finalMemory, and memoryIncreaseMB to use the
warm-up sequence (or a larger <50 MB limit) so the RSS comparison is less prone
to false positives across platforms.
In `@test/js/node/async_hooks/AsyncLocalStorage.test.ts`:
- Around line 565-590: Make the test await server.listen and server.close and
remove the arbitrary setTimeout: replace the server.listen callback-style usage
with an awaitable pattern (e.g., await new Promise(resolve => server.listen(0,
resolve))) and wrap the fetch/assert/resolve logic in try/finally so you always
await server.close() (e.g., await new Promise for listen, try { fetch + expect;
} finally { await new Promise(resolve => server.close(resolve)); resolve(); }).
In the request handler replace await new Promise(resolve => setTimeout(resolve,
10)) with a deterministic microtask/yield such as await new Promise(r =>
setImmediate(r)) or await Promise.resolve() so the AsyncLocalStorage nested
als.run() still yields without using arbitrary timeouts; keep references to
server.listen, server.close, the request handler, and als.run when making the
changes.
- Around line 571-574: The test contains an unnecessary await new Promise with
setTimeout inside the als.run callback which delays the test for 10ms even
though the counter is incremented synchronously; remove the setTimeout-based
delay from the async callback in als.run (the block that computes out with
++als.getStore().counter and later calls res.end(out)) and, if you need an
event-loop yield for semantics, replace that delay with a microtask yield (e.g.,
await a resolved promise) rather than a timed setTimeout so the test remains
deterministic.
In `@test/js/node/child_process/child_process.test.ts`:
- Around line 549-571: The test creates promise and merges it with childProcess
into subprocess but never awaits it, which can let the child process leak; make
the test function async and await the merged promise (await subprocess) before
finishing so the spawned process exits and cleanup runs; modify the
"tinyspawn-like library usage should work" test to declare async and add "await
subprocess" (or return the promise) after the existing assertions to ensure
childProcess/Promise created by spawn and promise are resolved.
- Around line 514-529: Rename the redundant test title inside the
describe("child process stdio properties should be enumerable for
Object.assign()") block to something more specific (e.g., "stdio properties have
enumerable: true") and ensure the spawned child (variable child created via
spawn(process.execPath, ["-e", 'console.log("hello")'])) is properly
awaited/cleaned up before the test finishes by awaiting the child's exit (e.g.,
return or await a Promise that resolves on child.on("exit" or "close") or
otherwise ensure child is killed and waited for) so the process does not become
orphaned.
- Around line 531-547: The test "Object.assign should copy child process stdio
properties" spawns a child process (spawn) but never waits for it to exit;
update the test to await the child process termination for proper cleanup by
creating and awaiting a Promise that resolves on the child's 'close' (or 'exit')
event (e.g., using child.on('close', ...) or the events.once helper) after you
finish assertions on merged, referencing the local variables child and merged so
the test waits for the process to finish before returning.
- Around line 484-491: The test spawns a child with spawn(...) but never waits
for or cleans it up; modify the test to capture the spawned child (e.g., const
child = spawn(bunExe(), { stdio: [input, input, input] })) and then wait for it
to exit (await new Promise(r => child.on('close', r)) or await
child.wait()/child.status if available) and/or kill it if needed, and ensure
stdout and stderr streams are closed/destroyed after the wait (call
stdout.destroy()/stderr.destroy() or similar) so the process is not left
orphaned and resources are released.
In `@test/js/node/dns/node-dns.test.js`:
- Around line 551-580: Replace the done-callback style tests for "dns.resolve
callback parameters", "dns.resolve with A record type callback parameters", and
"dns.resolve with AAAA record type callback parameters" with async tests that
use Promise.withResolvers: create a resolvers pair, call dns.resolve (or
dns.resolve with type "A"/"AAAA") and resolve/reject the promise inside the dns
callback based on error/addresses, then await the promise and assert the same
conditions (args length, null error, addresses array and string entries) instead
of calling done; update the test declarations to be async and remove the done
parameter so failures propagate as promise rejections.
In `@test/js/node/net/node-net.test.ts`:
- Around line 879-883: The tests contain redundant type assertions for
socket._handle.fd (e.g., both expect(socket._handle.fd).toBeTypeOf("number") and
expect(typeof socket._handle.fd).toBe("number")); remove the duplicate
typeof-based checks and keep a single type assertion per fd (or vice‑versa) and
similarly remove the duplicated assertions at the later block (lines around
898-900) so each fd is only type-checked once before assigning serverFd.
In `@test/js/node/process/process.test.js`:
- Around line 1205-1226: The test "console.log should not panic on broken pipe"
currently only checks stderr for "panic" but doesn't assert the child process
exit code; update the child.on("exit", ...) handler to capture the exit code and
assert it equals 0 (e.g., child.on("exit", (code) => { expect(code).toBe(0);
resolve(); })), and make the same change for the other similar test (the block
around lines 1267-1309) so both tests assert the child process exited
successfully; reference the spawned child variable (child) and the test
descriptions to locate each case.
- Around line 1186-1203: The test is flaky because the spawned process does an
immediate process.stdout.write which can beat the parent.destroy call; change
the spawned command so the child delays its write (e.g., use setImmediate or
setTimeout(..., 0) around process.stdout.write) to ensure child.stdout.destroy()
runs first; update the code that constructs the child (spawn(bunExe(), [...],
{env: bunEnv, stdio: [...]}) and the inline script passed to bunExe()) to
perform the delayed write, leaving the rest (child.stdout.destroy(), exitCode
promise, and expect(exitCode).not.toBe(0)) intact.
In `@test/js/node/timers/node-timers.test.ts`:
- Around line 298-303: The test "timers created at different times have
different _idleStart values" currently includes an unnecessary fixed wait (await
Bun.sleep(10)); remove that line and its accompanying comment so the test simply
creates timer1 and timer2 with setTimeout and relies on the existing assertion,
avoiding time-based waits and making the test faster and deterministic.
In `@test/js/node/tty.test.ts`:
- Around line 67-135: Replace the ad-hoc temp file with the test tempDir helper
(create a temp dir and write the script into it instead of
path.join(os.tmpdir(), ...)) so artifacts are isolated (replace tempFile
creation and Bun.write accordingly), and reorder the spawn result
awaits/assertions to read stdout/stderr before checking exitCode: await
proc.stdout.text() and proc.stderr.text() (and check NO_TTY and expect(stderr) /
expect(normalizeBunSnapshot(stdout)) first) and only then await proc.exited to
assert exitCode; update references to tempFile, Bun.write, Bun.spawn,
proc.stdout.text(), proc.stderr.text(), proc.exited, normalizeBunSnapshot, and
the test name test("tty.ReadStream ref/unref should behave like Node.js") when
making the changes.
- Around line 481-511: The test currently writes the non‑TTY fixture into the
global OS temp dir (tempFile using os.tmpdir()), which may leak files; change it
to create a test‑scoped temp directory with fs.mkdtempSync (e.g. const tmpDir =
fs.mkdtempSync(path.join(os.tmpdir(), "node-tty-test-"))), build the fixture
inside that directory instead of os.tmpdir(), and then ensure cleanup removes
both the file (fs.unlinkSync) and the temp directory (fs.rmdirSync or fs.rmSync)
in the finally block; update references to tempFile and add tmpDir creation and
removal around the existing fd/open/destroy logic in the "tty.ReadStream handles
non-TTY file descriptors correctly" test.
In `@test/js/node/util/bun-inspect.test.ts`:
- Around line 94-103: The test "Set is properly formatted in Bun.inspect()"
should normalize multiline inspect output to avoid CRLF/platform diffs: wrap the
actual formatted output and the expected snapshot string with
normalizeBunSnapshot before asserting; update the expect call that currently
compares Bun.inspect({ set }) directly to use
expect(normalizeBunSnapshot(formatted)).toBe(normalizeBunSnapshot(`...`));
ensure you import or reference normalizeBunSnapshot where needed and keep the
same expected content string used with Bun.inspect.
In `@test/js/node/vm/vm.test.ts`:
- Around line 60-71: The test registers a persistent listener on the global
process via process.on inside the VM code which pollutes subsequent tests;
update the test "issue `#9778`" so the listener is removed after the emission by
either using process.once("poop") in the VM code or by ensuring cleanup after
runInNewContext and the expect call (e.g., call
process.removeAllListeners("poop") or remove the specific listener in a finally
block). Locate the test function named "issue `#9778`" that calls
runInNewContext(...) and change the listener handling accordingly to prevent
test pollution.
In `@test/js/web/atomics.test.ts`:
- Around line 311-316: Add the missing issue number to the regression test
comment in the waitAsync tests: update the existing comment above the
describe("waitAsync regression tests") block (the lines describing the
Atomics.waitAsync UAF bug and DispatchTimer/WTFTimer interaction) to include the
original GitHub issue reference (e.g., change the top comment to something like
"// Regression test for #<actual-issue-number>"), ensuring the
test.todoIf("Atomics.waitAsync with setTimeout does not crash (UAF bug)") entry
remains unchanged.
In `@test/js/web/fetch/body.test.ts`:
- Around line 712-753: The tests assert fetch(req) throws synchronously but
fetch returns a rejected Promise; update each of the four tests (the ones
creating Request(...) and using Response(req.body).arrayBuffer(), .text(),
req.body.getReader().read(), and req.body.getReader()) to use async rejection
assertions: replace expect(() => fetch(req)).toThrow() with await
expect(fetch(req)).rejects.toThrow() (or equivalent await/then/reject handling),
keeping the existing Request, Response, req.body.getReader() calls and the final
expect(req.bodyUsed).toBe(true).
In `@test/js/web/fetch/fetch-gzip.test.ts`:
- Around line 521-526: Remove the brittle byte-check asserting the first byte
isn't 0x78 in the raw-deflate test: delete the line with
expect(compressed[0]).not.toBe(0x78); and keep the actual end-to-end validation
that uses deflateRawSync(compressed)/deflateTestData and the final assertion
expect(text).toBe(deflateTestData.toString()); this ensures the test verifies
decompression correctness without making fragile assumptions about the
compressor output.
In `@test/js/web/fetch/fetch.test.ts`:
- Around line 2517-2531: The test parses and asserts on bunStdout after checking
bunExitCode, which hides stdout diagnostics when the spawned process fails;
reorder the checks in the test so you await and parse bunStdout (the Promise.all
result producing bunStdout from new Response(bunProc.stdout).text()) and run the
JSON.parse and expect(bunResult).toEqual(...) assertions before asserting
expect(bunExitCode).toBe(0) (keep the same bunProc / bunStdout / bunExitCode
variables and the existing expected object, just move the exit-code assertion to
after the stdout parsing and expectations).
In `@test/js/web/html/html-rewriter-doctype.test.ts`:
- Around line 139-167: The test currently calls rewriter2.transform(new
Response(...)) but never consumes the Response body, so the element handler on
HTMLRewriter (rewriter2) may not run before the assertion; make the test async,
await the transformed Response.body consumption (e.g., await result.text() or
await result.arrayBuffer()) after calling rewriter2.transform(...) to ensure the
element handler runs and increments normalCallCount before asserting.
- Around line 261-273: The test name "HTMLRewriter concurrent usage should work
correctly" is misleading because it only calls rewriter.transform sequentially;
rename the test to something like "HTMLRewriter sequential reuse should work
correctly" or "HTMLRewriter multiple transforms should work correctly" to match
behavior, or alternatively change the test to actually run transforms in
parallel (e.g., use async transform calls with Promise.all) to assert true
concurrency; update the test title string and, if choosing the concurrency
route, adjust the body to call HTMLRewriter().transform(...) in parallel (using
Promise.all) and assert no throw. Ensure references to the rewriter instance and
the transform method (rewriter.transform) are updated accordingly.
- Around line 218-238: Update the test "HTMLRewriter memory management - no
leaks on selector parse errors" to assert memory stability: record memory with
process.memoryUsage() (or Bun.memoryUsage()) before the for-loop that repeatedly
constructs new HTMLRewriter and calls rewriter.on("div[incomplete", ...) and
then record memory after the loop; compute the relevant metric (e.g., heapUsed)
delta and add an assertion that the increase is within an acceptable threshold
(small constant) to catch real leaks instead of using the no-op
expect(true).toBe(true).
In `@test/js/web/request/request.test.ts`:
- Around line 79-87: The test "Request cache option is set correctly" uses
Request(...) with cacheValues including "only-if-cached", which per the Fetch
spec is only valid when mode is "same-origin"; update the test so the Request
constructor supplies mode: "same-origin" for that case (e.g., when iterating
cacheValues, if cache === "only-if-cached" call new Request(url, { cache, mode:
"same-origin" }), otherwise call new Request(url, { cache })) so the test stays
spec-compliant while still asserting request.cache matches the input.
In `@test/js/web/websocket/websocket.test.js`:
- Around line 869-898: The test never closes the underlying HTTP server: add a
deterministic shutdown for httpServer by invoking httpServer.close() and waiting
for its close callback before finishing the test (similar to how
wss.close()/ws.close() are used); update the test around the
WebSocket/WebSocketServer teardown (references: httpServer.listen, ws.close,
wss.close, httpServer.close) to call httpServer.close() and await its completion
(e.g., via a Promise resolver) so the server socket is released.
Comment on lines
+740
to
+744
|
|
||
| // Regression test for #8040 | ||
| test("semver with multiple tags work properly", () => { | ||
| expect(satisfies("3.4.5", ">=3.3.0-beta.1 <3.4.0-beta.3")).toBeFalse(); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Duplicate test case — this assertion already exists at line 729.
The test at line 729 within the "range excludes" section already covers this exact case:
[">=3.3.0-beta.1 <3.4.0-beta.3", "3.4.5"]
with a comment referencing issue #8040 at line 728. This standalone test is redundant.
Consider removing lines 740-744 since the regression is already covered.
Suggested removal
test("pre-release snapshot", () => {
expect(unsortedPrereleases.sort(Bun.semver.order)).toMatchSnapshot();
});
-
- // Regression test for `#8040`
- test("semver with multiple tags work properly", () => {
- expect(satisfies("3.4.5", ">=3.3.0-beta.1 <3.4.0-beta.3")).toBeFalse();
- });
});📝 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.
Suggested change
| // Regression test for #8040 | |
| test("semver with multiple tags work properly", () => { | |
| expect(satisfies("3.4.5", ">=3.3.0-beta.1 <3.4.0-beta.3")).toBeFalse(); | |
| }); |
🤖 Prompt for AI Agents
In `@test/cli/install/semver.test.ts` around lines 740 - 744, Remove the duplicate
standalone test "semver with multiple tags work properly" that calls
satisfies("3.4.5", ">=3.3.0-beta.1 <3.4.0-beta.3") since the same assertion
already exists in the "range excludes" section (the array case with
[">=3.3.0-beta.1 <3.4.0-beta.3", "3.4.5"]) — delete the test block containing
that assertion to avoid redundancy and keep the single existing test that
references issue `#8040`.
Comment on lines
+96
to
+106
| test("bun run sets cwd for script, matching npm", async () => { | ||
| $.cwd(issue10132Dir); | ||
| const currentPwd = (await $`${bunExe()} run get-pwd`.text()).trim(); | ||
| expect(currentPwd).toBe(issue10132Dir); | ||
|
|
||
| const currentPwd2 = join(currentPwd, "subdir", "one"); | ||
| $.cwd(currentPwd2); | ||
| expect((await $`${bunExe()} run get-pwd`.text()).trim()).toBe(issue10132Dir); | ||
|
|
||
| $.cwd(process.cwd()); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the test file and check its existence and content
find . -type f -name "run_command.test.ts" | head -5Repository: oven-sh/bun
Length of output: 89
🏁 Script executed:
# If found, read the specific lines mentioned (96-106)
cat -n test/cli/run/run_command.test.ts | sed -n '90,110p'Repository: oven-sh/bun
Length of output: 888
🏁 Script executed:
# Search for other $.cwd() usage patterns in test files to understand conventions
rg -t ts -t js '\$\.cwd\(' test/ -B 2 -A 5 | head -100Repository: oven-sh/bun
Length of output: 6940
Guard $.cwd(...) reset with finally to avoid state leakage.
If an assertion throws at line 99 or 103, the global $ cwd remains altered at line 105 and can taint later tests.
💡 Proposed fix
test("bun run sets cwd for script, matching npm", async () => {
- $.cwd(issue10132Dir);
- const currentPwd = (await $`${bunExe()} run get-pwd`.text()).trim();
- expect(currentPwd).toBe(issue10132Dir);
-
- const currentPwd2 = join(currentPwd, "subdir", "one");
- $.cwd(currentPwd2);
- expect((await $`${bunExe()} run get-pwd`.text()).trim()).toBe(issue10132Dir);
-
- $.cwd(process.cwd());
+ const originalCwd = process.cwd();
+ try {
+ $.cwd(issue10132Dir);
+ const currentPwd = (await $`${bunExe()} run get-pwd`.text()).trim();
+ expect(currentPwd).toBe(issue10132Dir);
+
+ const currentPwd2 = join(currentPwd, "subdir", "one");
+ $.cwd(currentPwd2);
+ expect((await $`${bunExe()} run get-pwd`.text()).trim()).toBe(issue10132Dir);
+ } finally {
+ $.cwd(originalCwd);
+ }
});🤖 Prompt for AI Agents
In `@test/cli/run/run_command.test.ts` around lines 96 - 106, The test "bun run
sets cwd for script, matching npm" mutates the global $.cwd and doesn't
guarantee reset if an assertion throws; wrap the body that changes $.cwd (calls
to $.cwd(issue10132Dir) and $.cwd(currentPwd2)) in a try/finally so that the
original cwd (e.g., capture const originalCwd = process.cwd() or prior $.cwd
value) is restored in the finally block via $.cwd(originalCwd) to avoid leaking
state across tests; ensure references to $.cwd and bunExe() remain unchanged and
only the control flow around them is updated.
Comment on lines
+108
to
+126
| test("issue #10132, bun run sets PATH", async () => { | ||
| async function run(dir: string) { | ||
| $.cwd(dir); | ||
| const [first, second] = await Promise.all([$`${bunExe()} bun-hello`.quiet(), $`${bunExe()} run bun-hello`.quiet()]); | ||
|
|
||
| expect(first.text().trim()).toBe("My name is bun-hello"); | ||
| expect(second.text().trim()).toBe("My name is bun-hello"); | ||
| } | ||
|
|
||
| await Promise.all( | ||
| [ | ||
| issue10132Dir, | ||
| join(issue10132Dir, "subdir"), | ||
| join(issue10132Dir, "subdir", "one"), | ||
| join(issue10132Dir, "subdir", "one", "two"), | ||
| join(issue10132Dir, "subdir", "one", "two", "three"), | ||
| ].map(run), | ||
| ); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the test file
cat -n test/cli/run/run_command.test.ts | sed -n '100,130p'Repository: oven-sh/bun
Length of output: 1056
🏁 Script executed:
# Let's search for how $.cwd() is defined and used in the codebase
rg "\.cwd\(" -A 2 -B 2 --type ts --type js | head -100Repository: oven-sh/bun
Length of output: 4990
🏁 Script executed:
# Check if this test file imports anything related to shell/zsh DSL
head -30 test/cli/run/run_command.test.tsRepository: oven-sh/bun
Length of output: 1032
🏁 Script executed:
# Search for other uses of $.cwd in test files to see patterns
rg '\$\.cwd\(' test/ -A 3 -B 1 --type ts --type js | head -150Repository: oven-sh/bun
Length of output: 7577
🏁 Script executed:
# Check if there are any existing test utilities for handling cwd in tests
rg 'originalCwd\|restoreCwd\|withCwd' test/ --type ts --type js | head -50Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Look at the Bun shell implementation to understand $.cwd() behavior
fd -t f -e ts -e js 'shell' src/ | head -20Repository: oven-sh/bun
Length of output: 155
🏁 Script executed:
# Let me look at how $.cwd() is implemented to understand if it's truly global
cat -n src/js/builtins/shell.ts | head -200Repository: oven-sh/bun
Length of output: 6894
🏁 Script executed:
# Check if there's a chainable API for $.cwd() that could avoid the race condition
rg 'cwd.*function\|\.cwd.*=.*function' src/ -A 5 --type ts | head -100Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Look for similar concurrent patterns in tests and see how they handle cwd
rg 'Promise\.all.*map.*cwd|Promise\.race.*cwd' test/ --type ts --type jsRepository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Let me see the rest of the shell.ts file to find the global $ object definition
cat -n src/js/builtins/shell.ts | sed -n '200,400p'Repository: oven-sh/bun
Length of output: 5769
🏁 Script executed:
# Search for where the global $ is defined/exported
rg 'export.*\$\s*=|export const \$' src/js/builtins/ -A 10Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Let me verify the actual test pattern one more time to confirm the race condition
cat -n test/cli/run/run_command.test.ts | sed -n '95,130p'Repository: oven-sh/bun
Length of output: 1293
Avoid concurrent $.cwd(...) mutations in the PATH test.
$.cwd() modifies global state on the $ object. Promise.all([...].map(run)) races across directories, causing commands to execute in the wrong working directory. Sequentialize with a for loop:
Proposed fix
test("issue `#10132`, bun run sets PATH", async () => {
async function run(dir: string) {
$.cwd(dir);
const [first, second] = await Promise.all([$`${bunExe()} bun-hello`.quiet(), $`${bunExe()} run bun-hello`.quiet()]);
expect(first.text().trim()).toBe("My name is bun-hello");
expect(second.text().trim()).toBe("My name is bun-hello");
}
- await Promise.all(
- [
- issue10132Dir,
- join(issue10132Dir, "subdir"),
- join(issue10132Dir, "subdir", "one"),
- join(issue10132Dir, "subdir", "one", "two"),
- join(issue10132Dir, "subdir", "one", "two", "three"),
- ].map(run),
- );
+ const originalCwd = process.cwd();
+ try {
+ for (const dir of [
+ issue10132Dir,
+ join(issue10132Dir, "subdir"),
+ join(issue10132Dir, "subdir", "one"),
+ join(issue10132Dir, "subdir", "one", "two"),
+ join(issue10132Dir, "subdir", "one", "two", "three"),
+ ]) {
+ await run(dir);
+ }
+ } finally {
+ $.cwd(originalCwd);
+ }
});📝 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.
Suggested change
| test("issue #10132, bun run sets PATH", async () => { | |
| async function run(dir: string) { | |
| $.cwd(dir); | |
| const [first, second] = await Promise.all([$`${bunExe()} bun-hello`.quiet(), $`${bunExe()} run bun-hello`.quiet()]); | |
| expect(first.text().trim()).toBe("My name is bun-hello"); | |
| expect(second.text().trim()).toBe("My name is bun-hello"); | |
| } | |
| await Promise.all( | |
| [ | |
| issue10132Dir, | |
| join(issue10132Dir, "subdir"), | |
| join(issue10132Dir, "subdir", "one"), | |
| join(issue10132Dir, "subdir", "one", "two"), | |
| join(issue10132Dir, "subdir", "one", "two", "three"), | |
| ].map(run), | |
| ); | |
| }); | |
| test("issue `#10132`, bun run sets PATH", async () => { | |
| async function run(dir: string) { | |
| $.cwd(dir); | |
| const [first, second] = await Promise.all([$`${bunExe()} bun-hello`.quiet(), $`${bunExe()} run bun-hello`.quiet()]); | |
| expect(first.text().trim()).toBe("My name is bun-hello"); | |
| expect(second.text().trim()).toBe("My name is bun-hello"); | |
| } | |
| const originalCwd = process.cwd(); | |
| try { | |
| for (const dir of [ | |
| issue10132Dir, | |
| join(issue10132Dir, "subdir"), | |
| join(issue10132Dir, "subdir", "one"), | |
| join(issue10132Dir, "subdir", "one", "two"), | |
| join(issue10132Dir, "subdir", "one", "two", "three"), | |
| ]) { | |
| await run(dir); | |
| } | |
| } finally { | |
| $.cwd(originalCwd); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In `@test/cli/run/run_command.test.ts` around lines 108 - 126, The test races
because $.cwd(...) mutates shared state inside the async helper run; replace the
concurrent Promise.all([...].map(run)) call with a sequential iteration (e.g. a
for/for-of loop) that awaits each run(...) in order so $.cwd(dir) is not called
concurrently; update the test block containing the run function and the
invocation that currently uses Promise.all to iterate sequentially to ensure
each invocation of $.cwd and the subsequent $`${bunExe()} ...` calls run in the
intended working directory.
Comment on lines
+463
to
+465
| // Test future date - should not be expired | ||
| const futureCookie = new Bun.Cookie("test", "value", { expires: Date.now() + 86400000 }); | ||
| expect(futureCookie.isExpired()).toBe(false); |
Contributor
There was a problem hiding this comment.
Potential issue: expires value may be interpreted as seconds, not milliseconds.
Based on lines 14-16 in this file, the Bun.Cookie constructor interprets numeric expires values as seconds (Unix timestamp), not milliseconds. The value Date.now() + 86400000 is in milliseconds and would result in a timestamp far in the future (year ~2297).
The test still passes because the cookie won't be expired, but the intent seems to be 1 day in the future rather than ~86 million days.
Suggested fix
// Test future date - should not be expired
- const futureCookie = new Bun.Cookie("test", "value", { expires: Date.now() + 86400000 });
+ const futureCookie = new Bun.Cookie("test", "value", { expires: Math.floor(Date.now() / 1000) + 86400 });
expect(futureCookie.isExpired()).toBe(false);🤖 Prompt for AI Agents
In `@test/js/bun/cookie/cookie.test.ts` around lines 463 - 465, The test passes a
millisecond timestamp to Bun.Cookie which expects seconds; update the expires
argument used when constructing futureCookie so it represents a Unix timestamp
in seconds (e.g., current time in seconds plus 86400 for one day) rather than
Date.now() in milliseconds, ensuring the Bun.Cookie("test","value", { expires:
... }) call and the subsequent futureCookie.isExpired() assertion reflect a
1-day future expiry.
Comment on lines
+204
to
+210
| routes: { | ||
| "/dynamic": () => new Response("▲"), | ||
| "/static": new Response("▲"), | ||
| "/unicode-string": new Response("こんにちは世界"), | ||
| "/emoji": new Response("🎉🚀✨"), | ||
| }, | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unused /dynamic route defined but never tested.
The /dynamic route is declared but no assertions are made against it. Either remove it if it's not needed, or add assertions to verify dynamic routes also handle Unicode correctly (which would strengthen this regression test by comparing static vs dynamic behavior for the same content).
Option 1: Remove unused route
using server = Bun.serve({
port: 0,
routes: {
- "/dynamic": () => new Response("▲"),
"/static": new Response("▲"),
"/unicode-string": new Response("こんにちは世界"),
"/emoji": new Response("🎉🚀✨"),
},
});Option 2: Add test coverage for dynamic route
// Test basic unicode character
{
const staticResp = await fetch(`${baseUrl}/static`);
+ const dynamicResp = await fetch(`${baseUrl}/dynamic`);
const staticText = await staticResp.text();
+ const dynamicText = await dynamicResp.text();
expect(staticText).toBe("▲");
+ expect(dynamicText).toBe("▲");
expect(staticResp.headers.get("content-type")).toBe("text/plain; charset=utf-8");
+ expect(dynamicResp.headers.get("content-type")).toBe("text/plain; charset=utf-8");
}📝 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.
Suggested change
| routes: { | |
| "/dynamic": () => new Response("▲"), | |
| "/static": new Response("▲"), | |
| "/unicode-string": new Response("こんにちは世界"), | |
| "/emoji": new Response("🎉🚀✨"), | |
| }, | |
| }); | |
| routes: { | |
| "/static": new Response("▲"), | |
| "/unicode-string": new Response("こんにちは世界"), | |
| "/emoji": new Response("🎉🚀✨"), | |
| }, | |
| }); |
🤖 Prompt for AI Agents
In `@test/js/bun/http/bun-serve-static.test.ts` around lines 204 - 210, The test
defines a "/dynamic" route that is never asserted; either remove the unused
"/dynamic" entry from the routes object or add assertions that exercise it
(e.g., fetch "/dynamic" and assert the status/body/headers match expectations
and compare Unicode handling to the "/static" route). Locate the routes
definition (the routes: { "/dynamic": () => new Response("▲"), ... } block) and
either delete the "/dynamic" property or add test steps that issue an HTTP
request to "/dynamic" and verify the response body contains the expected
character(s) and matches the static route behavior.
Comment on lines
+139
to
+167
| // Regression test for #21680 | ||
| test("HTMLRewriter exception handling should not break normal operation", () => { | ||
| // Ensure that after an exception occurs, the rewriter still works normally | ||
| let normalCallCount = 0; | ||
|
|
||
| // First, trigger an exception | ||
| try { | ||
| const rewriter = new HTMLRewriter().on("div", { | ||
| element(element) { | ||
| throw new Error("test error"); | ||
| }, | ||
| }); | ||
| rewriter.transform(new Response("<div>test</div>")); | ||
| } catch (e) { | ||
| // Expected to throw | ||
| } | ||
|
|
||
| // Then ensure normal operation still works | ||
| const rewriter2 = new HTMLRewriter().on("div", { | ||
| element(element) { | ||
| normalCallCount++; | ||
| element.setInnerContent("replaced"); | ||
| }, | ||
| }); | ||
|
|
||
| const result = rewriter2.transform(new Response("<div>original</div>")); | ||
| expect(normalCallCount).toBe(1); | ||
| // The transform should complete successfully without throwing | ||
| }); |
Contributor
There was a problem hiding this comment.
Handler may not be invoked without consuming the Response body.
On line 164, rewriter2.transform(new Response(...)) returns a Response object, but the body isn't consumed. The element handler might not be invoked until the response body is read (e.g., via .text() or .arrayBuffer()).
This could result in normalCallCount being 0 at assertion time, making the test unreliable.
Proposed fix
- const result = rewriter2.transform(new Response("<div>original</div>"));
- expect(normalCallCount).toBe(1);
+ const result = await rewriter2.transform(new Response("<div>original</div>")).text();
+ expect(normalCallCount).toBe(1);
+ expect(result).toContain("replaced");Note: The test function would also need to be marked async.
🤖 Prompt for AI Agents
In `@test/js/web/html/html-rewriter-doctype.test.ts` around lines 139 - 167, The
test currently calls rewriter2.transform(new Response(...)) but never consumes
the Response body, so the element handler on HTMLRewriter (rewriter2) may not
run before the assertion; make the test async, await the transformed
Response.body consumption (e.g., await result.text() or await
result.arrayBuffer()) after calling rewriter2.transform(...) to ensure the
element handler runs and increments normalCallCount before asserting.
Comment on lines
+218
to
+238
| test("HTMLRewriter memory management - no leaks on selector parse errors", () => { | ||
| // This test ensures that selector_slice memory is properly freed | ||
| // even when selector parsing fails | ||
| for (let i = 0; i < 100; i++) { | ||
| try { | ||
| const rewriter = new HTMLRewriter(); | ||
| // Use an invalid selector to trigger error path | ||
| rewriter.on("div[incomplete", { | ||
| element(element) { | ||
| console.log("Should not reach here"); | ||
| }, | ||
| }); | ||
| } catch (e) { | ||
| // Expected to throw, but no memory should leak | ||
| } | ||
| } | ||
|
|
||
| // If there were memory leaks, running this many times would consume significant memory | ||
| // The test passes if it completes without memory issues | ||
| expect(true).toBe(true); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider improving the memory leak test assertion.
The current test relies on the assumption that 100 iterations would cause observable memory issues if there were leaks. The assertion expect(true).toBe(true) on line 237 always passes and doesn't validate anything meaningful.
While this serves as a basic smoke test, consider tracking memory usage before/after the loop using process.memoryUsage() or Bun.memoryUsage() for a more meaningful assertion, if precise memory leak detection is important.
🤖 Prompt for AI Agents
In `@test/js/web/html/html-rewriter-doctype.test.ts` around lines 218 - 238,
Update the test "HTMLRewriter memory management - no leaks on selector parse
errors" to assert memory stability: record memory with process.memoryUsage() (or
Bun.memoryUsage()) before the for-loop that repeatedly constructs new
HTMLRewriter and calls rewriter.on("div[incomplete", ...) and then record memory
after the loop; compute the relevant metric (e.g., heapUsed) delta and add an
assertion that the increase is within an acceptable threshold (small constant)
to catch real leaks instead of using the no-op expect(true).toBe(true).
Comment on lines
+261
to
+273
| test("HTMLRewriter concurrent usage should work correctly", () => { | ||
| // Same rewriter instance should handle multiple transforms | ||
| const rewriter = new HTMLRewriter().on("div", { | ||
| element(element) { | ||
| element.setInnerContent("modified"); | ||
| }, | ||
| }); | ||
|
|
||
| expect(() => { | ||
| const result1 = rewriter.transform("<div>original1</div>"); | ||
| const result2 = rewriter.transform("<div>original2</div>"); | ||
| }).not.toThrow(); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Test name may be misleading.
The test is named "concurrent usage" but actually tests sequential reuse of the same rewriter instance. True concurrency would involve parallel execution (e.g., using Promise.all). Consider renaming to "sequential reuse" or "multiple transforms" for clarity.
🤖 Prompt for AI Agents
In `@test/js/web/html/html-rewriter-doctype.test.ts` around lines 261 - 273, The
test name "HTMLRewriter concurrent usage should work correctly" is misleading
because it only calls rewriter.transform sequentially; rename the test to
something like "HTMLRewriter sequential reuse should work correctly" or
"HTMLRewriter multiple transforms should work correctly" to match behavior, or
alternatively change the test to actually run transforms in parallel (e.g., use
async transform calls with Promise.all) to assert true concurrency; update the
test title string and, if choosing the concurrency route, adjust the body to
call HTMLRewriter().transform(...) in parallel (using Promise.all) and assert no
throw. Ensure references to the rewriter instance and the transform method
(rewriter.transform) are updated accordingly.
Comment on lines
+79
to
+87
| // Regression test for #2993 | ||
| test("Request cache option is set correctly", () => { | ||
| const cacheValues = ["default", "no-store", "reload", "no-cache", "force-cache", "only-if-cached"] as const; | ||
|
|
||
| for (const cache of cacheValues) { | ||
| const request = new Request("http://localhost:8080/", { cache }); | ||
| expect(request.cache).toBe(cache); | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate the Request implementation in the codebase
fd -e ts -e js | xargs rg -l "class Request|function Request" | grep -v test | head -20Repository: oven-sh/bun
Length of output: 552
🏁 Script executed:
# Search for cache validation logic in Request
rg "only-if-cached" --type ts --type js -B 3 -A 3Repository: oven-sh/bun
Length of output: 609
🏁 Script executed:
# Look for mode validation with cache options
rg "same-origin" --type ts --type js -B 2 -A 2 | head -50Repository: oven-sh/bun
Length of output: 3391
🏁 Script executed:
# Check the test file to see the full context
head -100 test/js/web/request/request.test.tsRepository: oven-sh/bun
Length of output: 3167
🏁 Script executed:
# Look for Request implementation in src/bun.js or WebAPI implementation
fd -type f -name "*.ts" -o -name "*.js" | xargs rg -l "class Request|export.*Request" | grep -v test | grep -v node_modules | head -20Repository: oven-sh/bun
Length of output: 940
🏁 Script executed:
# Search for any validation of cache mode combinations
rg "cache.*mode|only-if-cached" --type ts --type js --type cpp -B 2 -A 2 | grep -v test | head -60Repository: oven-sh/bun
Length of output: 4424
🌐 Web query:
Fetch API specification only-if-cached cache mode same-origin mode requirement
💡 Result:
Short answer: The Fetch standard requires that cache mode "only-if-cached" is only usable when the Request’s mode is "same-origin"; with "only-if-cached" fetch uses any matching cached response (ignoring staleness) and returns a network error if none is found (browsers may surface this as a 504 in practice). [1][2]
Sources:
- Fetch Standard — only-if-cached description. [1]
- MDN Web Docs — Request.cache ("only-if-cached" + same-origin note). [2]
🏁 Script executed:
# Check if there are any other tests that use only-if-cached with specific mode
rg "only-if-cached" -B 5 -A 5Repository: oven-sh/bun
Length of output: 1795
🏁 Script executed:
# Check the Request.zig implementation for cache/mode validation
cat src/bun.js/webcore/Request.zig | head -200Repository: oven-sh/bun
Length of output: 6371
🏁 Script executed:
# Search for validation of cache and mode together
rg "only-if-cached|cache.*mode" src/bun.js/webcore/ -B 3 -A 3Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Check if there's any validation logic that would throw on invalid combinations
rg "invalid.*cache|cache.*invalid|only.if.cached" src/ --type-list | head -20Repository: oven-sh/bun
Length of output: 444
🏁 Script executed:
# Look at the full request test file to see if other tests cover this combination
cat test/js/web/request/request.test.tsRepository: oven-sh/bun
Length of output: 4819
🏁 Script executed:
# Check for any validation logic that validates cache/mode combinations
rg "only-if-cached" src/ -B 5 -A 5Repository: oven-sh/bun
Length of output: 1002
🏁 Script executed:
# Search for any error handling or validation in Request constructor/initialization
rg "FetchCacheMode|FetchRequestMode" src/bun.js/webcore/Request.zig -B 3 -A 3Repository: oven-sh/bun
Length of output: 1179
🏁 Script executed:
# Look for where cache and mode are actually used/validated
rg "\.cache|\.mode" src/bun.js/webcore/Request.zig | head -30Repository: oven-sh/bun
Length of output: 675
🏁 Script executed:
# Search for any validation or error handling when cache and mode are combined
rg "only.if.cached" src/bun.js/webcore/ -B 10 -A 10Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Look for validation or spec compliance checks related to cache modes
rg "validate|check|throw" src/bun.js/webcore/Request.zig | grep -i cacheRepository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Check FetchRequestMode and FetchCacheMode files for any validation
cat src/http/FetchRequestMode.zigRepository: oven-sh/bun
Length of output: 470
🏁 Script executed:
# Look for any fetch-related validation files
fd "Fetch" src/ -type fRepository: oven-sh/bun
Length of output: 225
🏁 Script executed:
# Search for Fetch files with proper syntax
find src -name "*Fetch*" -type f | head -20Repository: oven-sh/bun
Length of output: 551
🏁 Script executed:
# Look for any validation logic related to only-if-cached restriction
rg "same.origin" src/ --type ts --type js -B 5 -A 5 | head -100Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Check if there are any browser/spec compliance tests for this combination
rg "only-if-cached.*same-origin|same-origin.*only-if-cached" test/ -iRepository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Let's verify if there's any actual validation being done anywhere
rg "only.if.cached|only-if-cached" src/bun.js/webcore/fetch/ -B 10 -A 10Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Check if there's JavaScript/TypeScript code that handles Request construction
find src -name "*.ts" -o -name "*.js" | xargs grep -l "only-if-cached" 2>/dev/null | grep -v testRepository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Verify the current behavior by checking where cache mode is actually applied in fetch
cat src/bun.js/webcore/fetch/FetchTasklet.zig | head -300Repository: oven-sh/bun
Length of output: 10926
🏁 Script executed:
# Check if there's any validation anywhere that would reject this combination
rg "only.if.cached" . --type ts --type js --type zig -B 5 -A 5 | grep -i "throw\|error\|reject\|invalid" | head -20Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Let's examine the complete Request initialization code to see if validation happens
rg "fromJS|constructor" src/bun.js/webcore/Request.zig -A 50 | head -150Repository: oven-sh/bun
Length of output: 5765
🏁 Script executed:
# Find the complete fromJS implementation for Request
rg "pub.*fn fromJS|pub.*fn fromJSDirect" src/bun.js/webcore/Request.zig -A 100 | head -200Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Search for where cache option is parsed from the init object
rg "cache" src/bun.js/webcore/Request.zig -B 3 -A 3 | grep -A 10 "Extract cache"Repository: oven-sh/bun
Length of output: 444
Ensure only-if-cached is used with mode: "same-origin" per Fetch spec.
Per the Fetch standard, only-if-cached cache mode is only valid when the request mode is "same-origin". The test uses the default mode ("cors"), which is spec-incompliant. The current implementation does not validate this constraint, so the test passes, but this could fail or lock in non-standard behavior if validation is added.
Recommended fix
- for (const cache of cacheValues) {
- const request = new Request("http://localhost:8080/", { cache });
- expect(request.cache).toBe(cache);
- }
+ for (const cache of cacheValues) {
+ const init = cache === "only-if-cached" ? { cache, mode: "same-origin" } : { cache };
+ const request = new Request("http://localhost:8080/", init);
+ expect(request.cache).toBe(cache);
+ }🤖 Prompt for AI Agents
In `@test/js/web/request/request.test.ts` around lines 79 - 87, The test "Request
cache option is set correctly" uses Request(...) with cacheValues including
"only-if-cached", which per the Fetch spec is only valid when mode is
"same-origin"; update the test so the Request constructor supplies mode:
"same-origin" for that case (e.g., when iterating cacheValues, if cache ===
"only-if-cached" call new Request(url, { cache, mode: "same-origin" }),
otherwise call new Request(url, { cache })) so the test stays spec-compliant
while still asserting request.cache matches the input.
Comment on lines
+869
to
+898
| // Regression test for #12040 | ||
| test("ws.send callback works as expected", async () => { | ||
| const { WebSocket, WebSocketServer } = await import("ws"); | ||
| const { createServer } = await import("node:http"); | ||
| const httpServer = createServer(); | ||
| const { promise, resolve } = Promise.withResolvers(); | ||
| const { promise: promise2, resolve: resolve2 } = Promise.withResolvers(); | ||
|
|
||
| const wss = new WebSocketServer({ | ||
| server: httpServer, | ||
| WebSocket, | ||
| }); | ||
|
|
||
| wss.on("connection", ws => { | ||
| // Following are two messages about to be sent, each with a slightly different way of calling the `ws.send` method: | ||
| ws.send("foo", () => resolve()); | ||
| ws.send("bar", {}, () => resolve2()); | ||
| }); | ||
|
|
||
| const { promise: promise3, resolve: resolve3 } = Promise.withResolvers(); | ||
| httpServer.listen(0, () => resolve3()); | ||
| await promise3; | ||
|
|
||
| var ws = new WebSocket("ws://localhost:" + httpServer.address().port); | ||
|
|
||
| ws.on("message", msg => {}); | ||
| await Promise.all([promise, promise2]); | ||
| ws.close(); | ||
| wss.close(); | ||
| }); |
Contributor
There was a problem hiding this comment.
Missing httpServer.close() cleanup.
The HTTP server is never closed. While ws.close() and wss.close() are called, the underlying httpServer remains listening, which could cause resource leaks or port conflicts in concurrent test runs.
Proposed fix
await Promise.all([promise, promise2]);
ws.close();
wss.close();
+ httpServer.close();
});🤖 Prompt for AI Agents
In `@test/js/web/websocket/websocket.test.js` around lines 869 - 898, The test
never closes the underlying HTTP server: add a deterministic shutdown for
httpServer by invoking httpServer.close() and waiting for its close callback
before finishing the test (similar to how wss.close()/ws.close() are used);
update the test around the WebSocket/WebSocketServer teardown (references:
httpServer.listen, ws.close, wss.close, httpServer.close) to call
httpServer.close() and await its completion (e.g., via a Promise resolver) so
the server socket is released.
Comment on lines
+1380
to
+1477
| // Regression test for https://github.com/oven-sh/bun/issues/22157 | ||
| // Compiled binaries were including executable name in process.argv | ||
| test("issue 22157: compiled binary should not include executable name in process.argv", async () => { | ||
| const dir = tempDirWithFiles("22157-basic", { | ||
| "index.js": /* js */ ` | ||
| import { parseArgs } from "node:util" | ||
|
|
||
| console.log(JSON.stringify(process.argv)); | ||
|
|
||
| // This should work - no extra executable name should cause parseArgs to throw | ||
| parseArgs({ | ||
| args: process.argv.slice(2), | ||
| }); | ||
|
|
||
| console.log("SUCCESS"); | ||
| `, | ||
| }); | ||
|
|
||
| // Compile the binary | ||
| await using compileProc = Bun.spawn({ | ||
| cmd: [bunExe(), "build", "--compile", "--outfile=test-binary", "./index.js"], | ||
| cwd: dir, | ||
| env: bunEnv, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
|
|
||
| await compileProc.exited; | ||
|
|
||
| // Run the compiled binary - should not throw | ||
| await using runProc = Bun.spawn({ | ||
| cmd: ["./test-binary"], | ||
| cwd: dir, | ||
| env: bunEnv, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
|
|
||
| const [stdout, exitCode] = await Promise.all([runProc.stdout.text(), runProc.exited]); | ||
|
|
||
| expect(exitCode).toBe(0); | ||
| expect(stdout).toContain("SUCCESS"); | ||
|
|
||
| // Verify process.argv structure | ||
| const argvMatch = stdout.match(/\[.*?\]/); | ||
| expect(argvMatch).toBeTruthy(); | ||
|
|
||
| const processArgv = JSON.parse(argvMatch![0]); | ||
| expect(processArgv).toHaveLength(2); | ||
| expect(processArgv[0]).toBe("bun"); | ||
| // Windows uses "B:/~BUN/root/", Unix uses "/$bunfs/root/" | ||
| expect(processArgv[1]).toMatch(/(\$bunfs|~BUN).*root/); | ||
| }); | ||
|
|
||
| // Regression test for https://github.com/oven-sh/bun/issues/22157 | ||
| test("issue 22157: compiled binary with user args should pass them correctly", async () => { | ||
| const dir = tempDirWithFiles("22157-args", { | ||
| "index.js": /* js */ ` | ||
| console.log(JSON.stringify(process.argv)); | ||
|
|
||
| // Expect: ["bun", "/$bunfs/root/..." or "B:/~BUN/root/...", "arg1", "arg2"] | ||
| if (process.argv.length !== 4) { | ||
| console.error("Expected 4 argv items, got", process.argv.length); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| if (process.argv[2] !== "arg1" || process.argv[3] !== "arg2") { | ||
| console.error("User args not correct"); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| console.log("SUCCESS"); | ||
| `, | ||
| }); | ||
|
|
||
| await using compileProc = Bun.spawn({ | ||
| cmd: [bunExe(), "build", "--compile", "--outfile=test-binary", "./index.js"], | ||
| cwd: dir, | ||
| env: bunEnv, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
|
|
||
| await compileProc.exited; | ||
|
|
||
| await using runProc = Bun.spawn({ | ||
| cmd: ["./test-binary", "arg1", "arg2"], | ||
| cwd: dir, | ||
| env: bunEnv, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
|
|
||
| const [stdout, exitCode] = await Promise.all([runProc.stdout.text(), runProc.exited]); | ||
|
|
||
| expect(exitCode).toBe(0); | ||
| expect(stdout).toContain("SUCCESS"); | ||
| }); |
Contributor
There was a problem hiding this comment.
Validate compile success before executing the binary.
Right now the tests run the compiled binary without asserting the compile step succeeded. If compilation fails, the failure becomes harder to diagnose.
🐛 Proposed fix: assert compile exit status
await using compileProc = Bun.spawn({
cmd: [bunExe(), "build", "--compile", "--outfile=test-binary", "./index.js"],
cwd: dir,
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});
- await compileProc.exited;
+ const [compileErr, compileExitCode] = await Promise.all([compileProc.stderr.text(), compileProc.exited]);
+ expect(compileErr).toBe("");
+ expect(compileExitCode).toBe(0);
// Run the compiled binary - should not throw
await using runProc = Bun.spawn({
cmd: ["./test-binary"],
cwd: dir,
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
}); await using compileProc = Bun.spawn({
cmd: [bunExe(), "build", "--compile", "--outfile=test-binary", "./index.js"],
cwd: dir,
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});
- await compileProc.exited;
+ const [compileErr, compileExitCode] = await Promise.all([compileProc.stderr.text(), compileProc.exited]);
+ expect(compileErr).toBe("");
+ expect(compileExitCode).toBe(0);
await using runProc = Bun.spawn({
cmd: ["./test-binary", "arg1", "arg2"],
cwd: dir,
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});🤖 Prompt for AI Agents
In `@test/bundler/bun-build-api.test.ts` around lines 1380 - 1477, The tests
currently spawn the compiler (compileProc) but don't assert it succeeded;
capture the compile exit code from compileProc.exited after awaiting compilation
and assert it's 0 before running the produced binary (e.g., check const
compileExit = await compileProc.exited; expect(compileExit).toBe(0)); if
non-zero, include compileProc.stderr.text() in the failure message or log to aid
debugging; apply this check in both places where compileProc is created (the
"22157-basic" and "22157-args" tests) so the runProc only starts when
compileProc succeeded.
Comment on lines
+2389
to
+2405
| test("bun build --target bun should support non-ascii source", async () => { | ||
| const files = { | ||
| "index.js": ` | ||
| console.log(JSON.stringify({\u{6211}: "a"})); | ||
|
|
||
| const \u{6211} = "b"; | ||
| console.log(JSON.stringify({\u{6211}})); | ||
| `, | ||
| }; | ||
| const source = tempDirWithFiles("source", files); | ||
|
|
||
| $.throws(true); | ||
| await $`${bunExe()} build --target bun ${join(source, "index.js")} --outfile ${join(source, "bundle.js")}`; | ||
| const result = await $`${bunExe()} ${join(source, "bundle.js")}`.text(); | ||
|
|
||
| expect(result).toBe(`{"\u{6211}":"a"}\n{"\u{6211}":"b"}\n`); | ||
| }); |
Contributor
There was a problem hiding this comment.
Reset global $.throws after the test to avoid cross-test leakage.
$.throws(true) mutates global state; leaving it enabled can affect other tests in this file (especially under concurrency). Please restore the default in a finally block.
✅ Suggested fix
- $.throws(true);
- await $`${bunExe()} build --target bun ${join(source, "index.js")} --outfile ${join(source, "bundle.js")}`;
- const result = await $`${bunExe()} ${join(source, "bundle.js")}`.text();
-
- expect(result).toBe(`{"\u{6211}":"a"}\n{"\u{6211}":"b"}\n`);
+ $.throws(true);
+ try {
+ await $`${bunExe()} build --target bun ${join(source, "index.js")} --outfile ${join(
+ source,
+ "bundle.js",
+ )}`;
+ const result = await $`${bunExe()} ${join(source, "bundle.js")}`.text();
+ expect(result).toBe(`{"\u{6211}":"a"}\n{"\u{6211}":"b"}\n`);
+ } finally {
+ $.nothrow();
+ }📝 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.
Suggested change
| test("bun build --target bun should support non-ascii source", async () => { | |
| const files = { | |
| "index.js": ` | |
| console.log(JSON.stringify({\u{6211}: "a"})); | |
| const \u{6211} = "b"; | |
| console.log(JSON.stringify({\u{6211}})); | |
| `, | |
| }; | |
| const source = tempDirWithFiles("source", files); | |
| $.throws(true); | |
| await $`${bunExe()} build --target bun ${join(source, "index.js")} --outfile ${join(source, "bundle.js")}`; | |
| const result = await $`${bunExe()} ${join(source, "bundle.js")}`.text(); | |
| expect(result).toBe(`{"\u{6211}":"a"}\n{"\u{6211}":"b"}\n`); | |
| }); | |
| test("bun build --target bun should support non-ascii source", async () => { | |
| const files = { | |
| "index.js": ` | |
| console.log(JSON.stringify({\u{6211}: "a"})); | |
| const \u{6211} = "b"; | |
| console.log(JSON.stringify({\u{6211}})); | |
| `, | |
| }; | |
| const source = tempDirWithFiles("source", files); | |
| $.throws(true); | |
| try { | |
| await $`${bunExe()} build --target bun ${join(source, "index.js")} --outfile ${join( | |
| source, | |
| "bundle.js", | |
| )}`; | |
| const result = await $`${bunExe()} ${join(source, "bundle.js")}`.text(); | |
| expect(result).toBe(`{"\u{6211}":"a"}\n{"\u{6211}":"b"}\n`); | |
| } finally { | |
| $.nothrow(); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In `@test/bundler/bundler_edgecase.test.ts` around lines 2389 - 2405, The test
"bun build --target bun should support non-ascii source" sets global
$.throws(true) which can leak into other tests; wrap the body that calls
bunExe() in a try/finally and restore $.throws to its previous/default value
(e.g., false) in the finally block so $.throws is reset even on failures—modify
the test around the $.throws(true) call and the awaits for bunExe() and result
retrieval to ensure cleanup.
Comment on lines
+1218
to
+1225
| minifySyntax: true, | ||
| minifyWhitespace: true, | ||
| onAfterBundle(api) { | ||
| const code = api.readFile("/out.js"); | ||
| expect(code).toMatchInlineSnapshot(` | ||
| "console.log(Array(Math.random()>-1?1:2)); | ||
| " | ||
| `); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Normalize bundler output before the inline snapshot.
Line 1221 snapshots raw output; normalize it for cross-platform stability.
♻️ Proposed fix
- const code = api.readFile("/out.js");
+ const code = normalizeBunSnapshot(api.readFile("/out.js"));
expect(code).toMatchInlineSnapshot(`
"console.log(Array(Math.random()>-1?1:2));
"
`);As per coding guidelines, use normalizeBunSnapshot for snapshot output.
📝 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.
Suggested change
| minifySyntax: true, | |
| minifyWhitespace: true, | |
| onAfterBundle(api) { | |
| const code = api.readFile("/out.js"); | |
| expect(code).toMatchInlineSnapshot(` | |
| "console.log(Array(Math.random()>-1?1:2)); | |
| " | |
| `); | |
| minifySyntax: true, | |
| minifyWhitespace: true, | |
| onAfterBundle(api) { | |
| const code = normalizeBunSnapshot(api.readFile("/out.js")); | |
| expect(code).toMatchInlineSnapshot(` | |
| "console.log(Array(Math.random()>-1?1:2)); | |
| " | |
| `); |
🤖 Prompt for AI Agents
In `@test/bundler/bundler_minify.test.ts` around lines 1218 - 1225, The inline
snapshot is using raw output from api.readFile("/out.js") in the
onAfterBundle(api) callback; update the test to normalize output before snapshot
comparison by calling normalizeBunSnapshot on the string returned from
api.readFile("/out.js") (keep the existing minifySyntax and minifyWhitespace
options), then pass the normalized value into expect(...).toMatchInlineSnapshot
so the snapshot is stable across platforms; locate this change in the
onAfterBundle function where api.readFile("/out.js") is read and the expect is
performed.
Comment on lines
+491
to
+705
| const dir = tempDirWithFiles("formatting-regression-test", { | ||
| "package.json": JSON.stringify({ | ||
| name: "test-project", | ||
| version: "1.0.0", | ||
| dependencies: { | ||
| a: "1.0.0", // Very short package name | ||
| }, | ||
| }), | ||
| "bun.lockb": JSON.stringify({ | ||
| lockfileVersion: 3, | ||
| packages: { | ||
| a: { | ||
| integrity: "sha512-fake", | ||
| version: "1.0.0", | ||
| }, | ||
| }, | ||
| }), | ||
| }); | ||
|
|
||
| const result = await Bun.spawn({ | ||
| cmd: [bunExe(), "update", "--interactive", "--dry-run"], | ||
| cwd: dir, | ||
| env, | ||
| stdin: "inherit", | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
|
|
||
| const stderr = await new Response(result.stderr).text(); | ||
|
|
||
| // Verify no underflow errors occur | ||
| expect(stderr).not.toContain("underflow"); | ||
| expect(stderr).not.toContain("panic"); | ||
| expect(stderr).not.toContain("overflow"); | ||
| }); | ||
|
|
||
| it("should handle dev tag length calculation correctly", async () => { | ||
| // This test verifies that dev/peer/optional tags are properly accounted for | ||
| // in the column width calculations | ||
| const dir = tempDirWithFiles("dev-tag-formatting-test", { | ||
| "package.json": JSON.stringify({ | ||
| name: "test-project", | ||
| version: "1.0.0", | ||
| dependencies: { | ||
| "regular-package": "1.0.0", | ||
| }, | ||
| devDependencies: { | ||
| "dev-package": "1.0.0", | ||
| }, | ||
| peerDependencies: { | ||
| "peer-package": "1.0.0", | ||
| }, | ||
| optionalDependencies: { | ||
| "optional-package": "1.0.0", | ||
| }, | ||
| }), | ||
| "bun.lockb": JSON.stringify({ | ||
| lockfileVersion: 3, | ||
| packages: { | ||
| "regular-package": { integrity: "sha512-fake", version: "1.0.0" }, | ||
| "dev-package": { integrity: "sha512-fake", version: "1.0.0" }, | ||
| "peer-package": { integrity: "sha512-fake", version: "1.0.0" }, | ||
| "optional-package": { integrity: "sha512-fake", version: "1.0.0" }, | ||
| }, | ||
| }), | ||
| }); | ||
|
|
||
| const result = await Bun.spawn({ | ||
| cmd: [bunExe(), "update", "--interactive", "--dry-run"], | ||
| cwd: dir, | ||
| env, | ||
| stdin: "inherit", | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
|
|
||
| const stderr = await new Response(result.stderr).text(); | ||
|
|
||
| // Verify no formatting errors occur with dev tags | ||
| expect(stderr).not.toContain("underflow"); | ||
| expect(stderr).not.toContain("panic"); | ||
| expect(stderr).not.toContain("overflow"); | ||
| }); | ||
|
|
||
| it("should truncate extremely long package names without crashing", async () => { | ||
| // This test verifies that package names longer than MAX_NAME_WIDTH (60) are handled | ||
| const longPackageName = "extremely-long-package-name-that-exceeds-maximum-width-and-should-be-truncated"; | ||
| const dir = tempDirWithFiles("truncate-test", { | ||
| "package.json": JSON.stringify({ | ||
| name: "test-project", | ||
| version: "1.0.0", | ||
| dependencies: { | ||
| [longPackageName]: "1.0.0", | ||
| }, | ||
| }), | ||
| "bun.lockb": JSON.stringify({ | ||
| lockfileVersion: 3, | ||
| packages: { | ||
| [longPackageName]: { | ||
| integrity: "sha512-fake", | ||
| version: "1.0.0", | ||
| }, | ||
| }, | ||
| }), | ||
| }); | ||
|
|
||
| const result = await Bun.spawn({ | ||
| cmd: [bunExe(), "update", "--interactive", "--dry-run"], | ||
| cwd: dir, | ||
| env, | ||
| stdin: "inherit", | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
|
|
||
| const stderr = await new Response(result.stderr).text(); | ||
|
|
||
| // Verify no crashes occur with extremely long package names | ||
| expect(stderr).not.toContain("underflow"); | ||
| expect(stderr).not.toContain("panic"); | ||
| expect(stderr).not.toContain("overflow"); | ||
| expect(stderr).not.toContain("segfault"); | ||
| }); | ||
|
|
||
| it("should handle long version strings without formatting issues", async () => { | ||
| // This test verifies that version strings longer than MAX_VERSION_WIDTH (20) are handled | ||
| const longVersion = "1.0.0-alpha.1.2.3.4.5.6.7.8.9.10.11.12.13.14.15.16.17.18.19.20.21.22.23.24.25"; | ||
| const dir = tempDirWithFiles("long-version-test", { | ||
| "package.json": JSON.stringify({ | ||
| name: "test-project", | ||
| version: "1.0.0", | ||
| dependencies: { | ||
| "package-with-long-version": longVersion, | ||
| }, | ||
| }), | ||
| "bun.lockb": JSON.stringify({ | ||
| lockfileVersion: 3, | ||
| packages: { | ||
| "package-with-long-version": { | ||
| integrity: "sha512-fake", | ||
| version: longVersion, | ||
| }, | ||
| }, | ||
| }), | ||
| }); | ||
|
|
||
| const result = await Bun.spawn({ | ||
| cmd: [bunExe(), "update", "--interactive", "--dry-run"], | ||
| cwd: dir, | ||
| env, | ||
| stdin: "inherit", | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
|
|
||
| const stderr = await new Response(result.stderr).text(); | ||
|
|
||
| // Verify no crashes occur with extremely long version strings | ||
| expect(stderr).not.toContain("underflow"); | ||
| expect(stderr).not.toContain("panic"); | ||
| expect(stderr).not.toContain("overflow"); | ||
| expect(stderr).not.toContain("segfault"); | ||
| }); | ||
|
|
||
| it("should handle edge case where all values are at maximum width", async () => { | ||
| // This test verifies edge cases where padding calculations might fail | ||
| const maxWidthPackage = "a".repeat(60); // MAX_NAME_WIDTH | ||
| const maxWidthVersion = "1.0.0-" + "a".repeat(15); // MAX_VERSION_WIDTH | ||
|
|
||
| const dir = tempDirWithFiles("max-width-test", { | ||
| "package.json": JSON.stringify({ | ||
| name: "test-project", | ||
| version: "1.0.0", | ||
| dependencies: { | ||
| [maxWidthPackage]: maxWidthVersion, | ||
| }, | ||
| devDependencies: { | ||
| [maxWidthPackage + "-dev"]: maxWidthVersion, | ||
| }, | ||
| peerDependencies: { | ||
| [maxWidthPackage + "-peer"]: maxWidthVersion, | ||
| }, | ||
| optionalDependencies: { | ||
| [maxWidthPackage + "-optional"]: maxWidthVersion, | ||
| }, | ||
| }), | ||
| "bun.lockb": JSON.stringify({ | ||
| lockfileVersion: 3, | ||
| packages: { | ||
| [maxWidthPackage]: { integrity: "sha512-fake", version: maxWidthVersion }, | ||
| [maxWidthPackage + "-dev"]: { integrity: "sha512-fake", version: maxWidthVersion }, | ||
| [maxWidthPackage + "-peer"]: { integrity: "sha512-fake", version: maxWidthVersion }, | ||
| [maxWidthPackage + "-optional"]: { integrity: "sha512-fake", version: maxWidthVersion }, | ||
| }, | ||
| }), | ||
| }); | ||
|
|
||
| const result = await Bun.spawn({ | ||
| cmd: [bunExe(), "update", "--interactive", "--dry-run"], | ||
| cwd: dir, | ||
| env, | ||
| stdin: "inherit", | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
|
|
||
| const stderr = await new Response(result.stderr).text(); | ||
|
|
||
| // Verify no crashes occur at maximum width values | ||
| expect(stderr).not.toContain("underflow"); | ||
| expect(stderr).not.toContain("panic"); | ||
| expect(stderr).not.toContain("overflow"); | ||
| expect(stderr).not.toContain("segfault"); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer disposable tempDir + await using for spawned processes.
tempDirWithFiles does not auto-clean, and await using is the preferred spawn pattern in this repo. Based on learnings and as per coding guidelines, please switch to tempDir with using and await using for Bun.spawn.
♻️ Suggested refactor (apply to each case in the block)
-import { bunExe, bunEnv as env, readdirSorted, tempDirWithFiles, toBeValidBin, toHaveBins } from "harness";
+import { bunExe, bunEnv as env, readdirSorted, tempDir, tempDirWithFiles, toBeValidBin, toHaveBins } from "harness";
- const dir = tempDirWithFiles("formatting-regression-test", {
+ using dir = tempDir("formatting-regression-test", {
"package.json": JSON.stringify({
name: "test-project",
version: "1.0.0",
dependencies: {
a: "1.0.0",
},
}),
"bun.lockb": JSON.stringify({
lockfileVersion: 3,
packages: {
a: {
integrity: "sha512-fake",
version: "1.0.0",
},
},
}),
});
- const result = await Bun.spawn({
+ await using proc = Bun.spawn({
cmd: [bunExe(), "update", "--interactive", "--dry-run"],
cwd: dir,
env,
stdin: "inherit",
stdout: "pipe",
stderr: "pipe",
});
- const stderr = await new Response(result.stderr).text();
+ const stderr = await proc.stderr.text();🤖 Prompt for AI Agents
In `@test/cli/install/bun-update.test.ts` around lines 491 - 705, Replace uses of
tempDirWithFiles with the disposable tempDir helper and wrap each Bun.spawn call
in an await using block so the temporary directory and spawned process are
auto-cleaned; locate the tests that create dirs via tempDirWithFiles (e.g.,
variables named dir) and where Bun.spawn is called with cmd: [bunExe(),
"update", ...], env, stdin/stdout/stderr, then change to create the tempDir
(tempDir(...)) and use "await using (const proc = Bun.spawn(...))" (or the
repo's canonical await-using pattern) to ensure both the temp directory and the
spawned process are disposed after the test.
Comment on lines
+519
to
+704
| const stderr = await new Response(result.stderr).text(); | ||
|
|
||
| // Verify no underflow errors occur | ||
| expect(stderr).not.toContain("underflow"); | ||
| expect(stderr).not.toContain("panic"); | ||
| expect(stderr).not.toContain("overflow"); | ||
| }); | ||
|
|
||
| it("should handle dev tag length calculation correctly", async () => { | ||
| // This test verifies that dev/peer/optional tags are properly accounted for | ||
| // in the column width calculations | ||
| const dir = tempDirWithFiles("dev-tag-formatting-test", { | ||
| "package.json": JSON.stringify({ | ||
| name: "test-project", | ||
| version: "1.0.0", | ||
| dependencies: { | ||
| "regular-package": "1.0.0", | ||
| }, | ||
| devDependencies: { | ||
| "dev-package": "1.0.0", | ||
| }, | ||
| peerDependencies: { | ||
| "peer-package": "1.0.0", | ||
| }, | ||
| optionalDependencies: { | ||
| "optional-package": "1.0.0", | ||
| }, | ||
| }), | ||
| "bun.lockb": JSON.stringify({ | ||
| lockfileVersion: 3, | ||
| packages: { | ||
| "regular-package": { integrity: "sha512-fake", version: "1.0.0" }, | ||
| "dev-package": { integrity: "sha512-fake", version: "1.0.0" }, | ||
| "peer-package": { integrity: "sha512-fake", version: "1.0.0" }, | ||
| "optional-package": { integrity: "sha512-fake", version: "1.0.0" }, | ||
| }, | ||
| }), | ||
| }); | ||
|
|
||
| const result = await Bun.spawn({ | ||
| cmd: [bunExe(), "update", "--interactive", "--dry-run"], | ||
| cwd: dir, | ||
| env, | ||
| stdin: "inherit", | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
|
|
||
| const stderr = await new Response(result.stderr).text(); | ||
|
|
||
| // Verify no formatting errors occur with dev tags | ||
| expect(stderr).not.toContain("underflow"); | ||
| expect(stderr).not.toContain("panic"); | ||
| expect(stderr).not.toContain("overflow"); | ||
| }); | ||
|
|
||
| it("should truncate extremely long package names without crashing", async () => { | ||
| // This test verifies that package names longer than MAX_NAME_WIDTH (60) are handled | ||
| const longPackageName = "extremely-long-package-name-that-exceeds-maximum-width-and-should-be-truncated"; | ||
| const dir = tempDirWithFiles("truncate-test", { | ||
| "package.json": JSON.stringify({ | ||
| name: "test-project", | ||
| version: "1.0.0", | ||
| dependencies: { | ||
| [longPackageName]: "1.0.0", | ||
| }, | ||
| }), | ||
| "bun.lockb": JSON.stringify({ | ||
| lockfileVersion: 3, | ||
| packages: { | ||
| [longPackageName]: { | ||
| integrity: "sha512-fake", | ||
| version: "1.0.0", | ||
| }, | ||
| }, | ||
| }), | ||
| }); | ||
|
|
||
| const result = await Bun.spawn({ | ||
| cmd: [bunExe(), "update", "--interactive", "--dry-run"], | ||
| cwd: dir, | ||
| env, | ||
| stdin: "inherit", | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
|
|
||
| const stderr = await new Response(result.stderr).text(); | ||
|
|
||
| // Verify no crashes occur with extremely long package names | ||
| expect(stderr).not.toContain("underflow"); | ||
| expect(stderr).not.toContain("panic"); | ||
| expect(stderr).not.toContain("overflow"); | ||
| expect(stderr).not.toContain("segfault"); | ||
| }); | ||
|
|
||
| it("should handle long version strings without formatting issues", async () => { | ||
| // This test verifies that version strings longer than MAX_VERSION_WIDTH (20) are handled | ||
| const longVersion = "1.0.0-alpha.1.2.3.4.5.6.7.8.9.10.11.12.13.14.15.16.17.18.19.20.21.22.23.24.25"; | ||
| const dir = tempDirWithFiles("long-version-test", { | ||
| "package.json": JSON.stringify({ | ||
| name: "test-project", | ||
| version: "1.0.0", | ||
| dependencies: { | ||
| "package-with-long-version": longVersion, | ||
| }, | ||
| }), | ||
| "bun.lockb": JSON.stringify({ | ||
| lockfileVersion: 3, | ||
| packages: { | ||
| "package-with-long-version": { | ||
| integrity: "sha512-fake", | ||
| version: longVersion, | ||
| }, | ||
| }, | ||
| }), | ||
| }); | ||
|
|
||
| const result = await Bun.spawn({ | ||
| cmd: [bunExe(), "update", "--interactive", "--dry-run"], | ||
| cwd: dir, | ||
| env, | ||
| stdin: "inherit", | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
|
|
||
| const stderr = await new Response(result.stderr).text(); | ||
|
|
||
| // Verify no crashes occur with extremely long version strings | ||
| expect(stderr).not.toContain("underflow"); | ||
| expect(stderr).not.toContain("panic"); | ||
| expect(stderr).not.toContain("overflow"); | ||
| expect(stderr).not.toContain("segfault"); | ||
| }); | ||
|
|
||
| it("should handle edge case where all values are at maximum width", async () => { | ||
| // This test verifies edge cases where padding calculations might fail | ||
| const maxWidthPackage = "a".repeat(60); // MAX_NAME_WIDTH | ||
| const maxWidthVersion = "1.0.0-" + "a".repeat(15); // MAX_VERSION_WIDTH | ||
|
|
||
| const dir = tempDirWithFiles("max-width-test", { | ||
| "package.json": JSON.stringify({ | ||
| name: "test-project", | ||
| version: "1.0.0", | ||
| dependencies: { | ||
| [maxWidthPackage]: maxWidthVersion, | ||
| }, | ||
| devDependencies: { | ||
| [maxWidthPackage + "-dev"]: maxWidthVersion, | ||
| }, | ||
| peerDependencies: { | ||
| [maxWidthPackage + "-peer"]: maxWidthVersion, | ||
| }, | ||
| optionalDependencies: { | ||
| [maxWidthPackage + "-optional"]: maxWidthVersion, | ||
| }, | ||
| }), | ||
| "bun.lockb": JSON.stringify({ | ||
| lockfileVersion: 3, | ||
| packages: { | ||
| [maxWidthPackage]: { integrity: "sha512-fake", version: maxWidthVersion }, | ||
| [maxWidthPackage + "-dev"]: { integrity: "sha512-fake", version: maxWidthVersion }, | ||
| [maxWidthPackage + "-peer"]: { integrity: "sha512-fake", version: maxWidthVersion }, | ||
| [maxWidthPackage + "-optional"]: { integrity: "sha512-fake", version: maxWidthVersion }, | ||
| }, | ||
| }), | ||
| }); | ||
|
|
||
| const result = await Bun.spawn({ | ||
| cmd: [bunExe(), "update", "--interactive", "--dry-run"], | ||
| cwd: dir, | ||
| env, | ||
| stdin: "inherit", | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
|
|
||
| const stderr = await new Response(result.stderr).text(); | ||
|
|
||
| // Verify no crashes occur at maximum width values | ||
| expect(stderr).not.toContain("underflow"); | ||
| expect(stderr).not.toContain("panic"); | ||
| expect(stderr).not.toContain("overflow"); | ||
| expect(stderr).not.toContain("segfault"); | ||
| }); |
Contributor
There was a problem hiding this comment.
Remove panic/segfault string assertions from stderr.
These checks won’t fail reliably in CI; instead assert successful completion (and any expected output) for the regression. As per coding guidelines, avoid panic-string checks.
🛠️ Example adjustment (apply similarly across the block)
- const stderr = await new Response(result.stderr).text();
- expect(stderr).not.toContain("underflow");
- expect(stderr).not.toContain("panic");
- expect(stderr).not.toContain("overflow");
+ const stderr = await new Response(result.stderr).text();
+ const exitCode = await result.exited;
+ expect(exitCode).toBe(0);
+ expect(stderr).not.toContain("underflow");🤖 Prompt for AI Agents
In `@test/cli/install/bun-update.test.ts` around lines 519 - 704, Tests currently
assert absence of "panic"/"segfault" strings in stderr (e.g.,
expect(stderr).not.toContain("panic") / "segfault") which is brittle; remove
those negative string checks and instead assert the command completed
successfully by checking the Bun.spawn result (e.g., assert result.exitCode ===
0 or use expect(result.exitCode).toBe(0)) and/or assert specific expected output
in stdout/stderr; update each test case that uses Bun.spawn (the blocks creating
result via Bun.spawn and reading stderr) to drop the panic/segfault assertions
and replace them with a stable success assertion against result.exitCode (and
any domain-specific expected text) while keeping the other underflow/overflow
checks if still desired.
Comment on lines
+2760
to
+2782
| test("YAML import with double-quoted strings containing ... - issue #23489", async () => { | ||
| using dir = tempDir("yaml-ellipsis", { | ||
| "test.yml": 'balance: "👛 لا تمتلك محفظة... !"', | ||
| "test.ts": ` | ||
| import yaml from "./test.yml"; | ||
| console.log(JSON.stringify(yaml)); | ||
| `, | ||
| }); | ||
|
|
||
| await using proc = Bun.spawn({ | ||
| cmd: [bunExe(), "test.ts"], | ||
| env: bunEnv, | ||
| cwd: String(dir), | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
|
|
||
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
|
|
||
| expect(stderr).not.toContain("Unexpected document end"); | ||
| expect(exitCode).toBe(0); | ||
| expect(stdout.trim()).toBe('{"balance":"👛 لا تمتلك محفظة... !"}'); | ||
| }); |
Contributor
There was a problem hiding this comment.
Assert stdout before exit code.
For clearer failures, check stdout (and stderr) before expect(exitCode).toBe(0). As per coding guidelines, output assertions should come first.
♻️ Suggested change
- expect(stderr).not.toContain("Unexpected document end");
- expect(exitCode).toBe(0);
- expect(stdout.trim()).toBe('{"balance":"👛 لا تمتلك محفظة... !"}');
+ expect(stderr).not.toContain("Unexpected document end");
+ expect(stdout.trim()).toBe('{"balance":"👛 لا تمتلك محفظة... !"}');
+ expect(exitCode).toBe(0);📝 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.
Suggested change
| test("YAML import with double-quoted strings containing ... - issue #23489", async () => { | |
| using dir = tempDir("yaml-ellipsis", { | |
| "test.yml": 'balance: "👛 لا تمتلك محفظة... !"', | |
| "test.ts": ` | |
| import yaml from "./test.yml"; | |
| console.log(JSON.stringify(yaml)); | |
| `, | |
| }); | |
| await using proc = Bun.spawn({ | |
| cmd: [bunExe(), "test.ts"], | |
| env: bunEnv, | |
| cwd: String(dir), | |
| stdout: "pipe", | |
| stderr: "pipe", | |
| }); | |
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | |
| expect(stderr).not.toContain("Unexpected document end"); | |
| expect(exitCode).toBe(0); | |
| expect(stdout.trim()).toBe('{"balance":"👛 لا تمتلك محفظة... !"}'); | |
| }); | |
| test("YAML import with double-quoted strings containing ... - issue `#23489`", async () => { | |
| using dir = tempDir("yaml-ellipsis", { | |
| "test.yml": 'balance: "👛 لا تمتلك محفظة... !"', | |
| "test.ts": ` | |
| import yaml from "./test.yml"; | |
| console.log(JSON.stringify(yaml)); | |
| `, | |
| }); | |
| await using proc = Bun.spawn({ | |
| cmd: [bunExe(), "test.ts"], | |
| env: bunEnv, | |
| cwd: String(dir), | |
| stdout: "pipe", | |
| stderr: "pipe", | |
| }); | |
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | |
| expect(stderr).not.toContain("Unexpected document end"); | |
| expect(stdout.trim()).toBe('{"balance":"👛 لا تمتلك محفظة... !"}'); | |
| expect(exitCode).toBe(0); | |
| }); |
🤖 Prompt for AI Agents
In `@test/js/bun/yaml/yaml.test.ts` around lines 2760 - 2782, In the "YAML import
with double-quoted strings containing ... - issue `#23489`" test, move the
expectations that inspect process output (expect(stderr).not.toContain(...) and
expect(stdout.trim()).toBe(...)) to run before the exit code assertion;
specifically reorder the assertions that reference stderr, stdout, and exitCode
(variables proc, stderr, stdout, exitCode) so you assert stderr and stdout first
and then call expect(exitCode).toBe(0), ensuring test output is checked before
validating the process exit status.
Comment on lines
+2784
to
+2807
| // Regression test for #26088 | ||
| // YAML parser was leaking memory on each parse call because AST nodes were | ||
| // not being freed. This caused segfaults after high-volume YAML parsing. | ||
| // Fix: Use ASTMemoryAllocator to ensure AST nodes are freed at end of scope. | ||
| test("YAML.parse shouldn't leak memory", () => { | ||
| // Create YAML with 10000 single-char strings - creates many AST E.String nodes | ||
| const items = Array.from({ length: 10000 }, () => " - x").join("\n"); | ||
| const yaml = `list:\n${items}`; | ||
|
|
||
| Bun.gc(true); | ||
| const initialMemory = process.memoryUsage.rss(); | ||
|
|
||
| // Parse 100 times - each creates 10000 AST string nodes | ||
| for (let i = 0; i < 100; i++) { | ||
| YAML.parse(yaml); | ||
| } | ||
|
|
||
| Bun.gc(true); | ||
| const finalMemory = process.memoryUsage.rss(); | ||
|
|
||
| // Memory increase should be less than 50MB if AST nodes are freed properly | ||
| const memoryIncreaseMB = (finalMemory - initialMemory) / 1024 / 1024; | ||
| expect(memoryIncreaseMB).toBeLessThan(50); | ||
| }); |
Contributor
There was a problem hiding this comment.
RSS-based leak check may be flaky across platforms.
RSS can vary due to allocator behavior even without leaks. Consider a short warm‑up parse + GC before taking the baseline (or slightly widen the threshold) to reduce false positives.
♻️ Suggested change
- Bun.gc(true);
- const initialMemory = process.memoryUsage.rss();
+ // Warm up allocator to reduce RSS noise
+ for (let i = 0; i < 5; i++) YAML.parse(yaml);
+ Bun.gc(true);
+ const initialMemory = process.memoryUsage.rss();🤖 Prompt for AI Agents
In `@test/js/bun/yaml/yaml.test.ts` around lines 2784 - 2807, The RSS-based leak
assertion in the test "YAML.parse shouldn't leak memory" is flaky; perform a
warm-up parse and GC before capturing initialMemory (e.g., call YAML.parse(yaml)
once and Bun.gc(true) to stabilize allocator) or increase the memory threshold;
update the test around YAML.parse, Bun.gc(true), initialMemory, finalMemory, and
memoryIncreaseMB to use the warm-up sequence (or a larger <50 MB limit) so the
RSS comparison is less prone to false positives across platforms.
Comment on lines
+712
to
+753
| test("await fetch(req) throws if req.body is already consumed (arrayBuffer)", async () => { | ||
| const req = new Request("https://example.com/", { | ||
| body: "test", | ||
| method: "POST", | ||
| }); | ||
|
|
||
| await new Response(req.body).arrayBuffer(); | ||
| expect(() => fetch(req)).toThrow(); | ||
| expect(req.bodyUsed).toBe(true); | ||
| }); | ||
|
|
||
| test("await fetch(req) throws if req.body is already consumed (text)", async () => { | ||
| const req = new Request("https://example.com/", { | ||
| body: "test", | ||
| method: "POST", | ||
| }); | ||
|
|
||
| await new Response(req.body).text(); | ||
| expect(() => fetch(req)).toThrow(); | ||
| expect(req.bodyUsed).toBe(true); | ||
| }); | ||
|
|
||
| test("await fetch(req) throws if req.body is already consumed (stream that has been read)", async () => { | ||
| const req = new Request("https://example.com/", { | ||
| body: "test", | ||
| method: "POST", | ||
| }); | ||
|
|
||
| await req.body.getReader().read(); | ||
| expect(() => fetch(req)).toThrow(); | ||
| expect(req.bodyUsed).toBe(true); | ||
| }); | ||
|
|
||
| test("await fetch(req) throws if req.body is already consumed (stream)", async () => { | ||
| const req = new Request("https://example.com/", { | ||
| body: "test", | ||
| method: "POST", | ||
| }); | ||
|
|
||
| req.body.getReader(); | ||
| expect(() => fetch(req)).toThrow(); | ||
| expect(req.bodyUsed).toBe(true); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if there are similar fetch error patterns in other test files
rg "fetch.*expect.*toThrow" test/ --type ts --type js -B 2 -A 2 | head -50Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Look at the actual test file to understand the context better
cat -n test/js/web/fetch/body.test.ts | sed -n '700,760p'Repository: oven-sh/bun
Length of output: 1905
🏁 Script executed:
# Search for other tests that check fetch() rejection behavior
rg "expect.*fetch.*rejects|expect.*async.*fetch" test/ --type ts --type js -B 1 -A 1 | head -30Repository: oven-sh/bun
Length of output: 2057
🏁 Script executed:
# Try running the specific test to see if it passes or fails
cd test/js/web/fetch && bun test body.test.ts 2>&1 | tail -100Repository: oven-sh/bun
Length of output: 96
🏁 Script executed:
# Also check the fetch implementation for clues about sync vs async error handling
rg "consumed.*body|bodyUsed.*true" src/ --type ts --type js --type zig -B 3 -A 3 | head -60Repository: oven-sh/bun
Length of output: 949
🏁 Script executed:
# Search for fetch() implementation that checks consumed body
rg "bodyUsed|consumed.*body" src/js/ -t ts -t js -t zig -B 5 -A 5 | grep -A 10 -B 10 "fetch"Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Look at Request implementation for body consumption
rg "class Request|bodyUsed" src/js/builtins/ -t ts -t js -A 5 | head -80Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Check if there are type definitions that show when body validation happens
fd "request\|fetch" src/js/ --type ts --type js | xargs grep -l "consumed\|bodyUsed" 2>/dev/null | head -5Repository: oven-sh/bun
Length of output: 224
🏁 Script executed:
# Look for fetch implementation in Bun - check builtins or js directory
find src -name "*.ts" -o -name "*.js" -o -name "*.zig" | xargs grep -l "function fetch\|export.*fetch" 2>/dev/null | head -10Repository: oven-sh/bun
Length of output: 261
🏁 Script executed:
# Check for Request class implementation details about body validation
rg "class Request" src/ -t ts -t js -A 20 | head -100Repository: oven-sh/bun
Length of output: 3496
🏁 Script executed:
# Look at test patterns - are there other async body/request validation tests?
rg "expect.*async.*await.*fetch.*body|request.*body.*consumed" test/js/web/fetch/ -A 2 -B 2Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Examine the fetch.zig file to see when body consumption is validated
cat src/bun.js/webcore/fetch.zig | head -200Repository: oven-sh/bun
Length of output: 7243
🏁 Script executed:
# Search for when bodyUsed check happens in fetch
rg "bodyUsed.*consumed|consumed.*body" src/bun.js/ -A 5 -B 5 | head -100Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Check what happens when fetch is called with a consumed body
rg "fetch.*body.*used|body.*used.*fetch" test/js/web/fetch/ -B 3 -A 3Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Look for where body consumption/bodyUsed is validated in fetch
rg "getAndClearBody|bodyUsed" src/bun.js/webcore/ -B 3 -A 3 | head -150Repository: oven-sh/bun
Length of output: 1387
🏁 Script executed:
# Check if body validation throws synchronously or returns a rejected promise
rg "Body.*locked|Body.*used" src/bun.js/ --type zig -B 2 -A 2 | head -100Repository: oven-sh/bun
Length of output: 3135
🏁 Script executed:
# Look for where fetch validates the request body parameter
sed -n '1,400p' src/bun.js/webcore/fetch.zig | grep -A 30 "request"Repository: oven-sh/bun
Length of output: 4161
🏁 Script executed:
# Search for where fetch validates body consumption - look for handleBodyAlreadyUsed or similar
rg "handleBodyAlreadyUsed|BODY_ALREADY_USED" src/bun.js/webcore/ -B 5 -A 5Repository: oven-sh/bun
Length of output: 15517
🏁 Script executed:
# Look at the actual request body extraction in fetch to see when validation happens
sed -n '300,600p' src/bun.js/webcore/fetch.zig | tail -100Repository: oven-sh/bun
Length of output: 3316
🏁 Script executed:
# Search for getAndClearBody or body validation in fetch context
rg "getBody|clearBody" src/bun.js/webcore/fetch.zig -B 3 -A 3Repository: oven-sh/bun
Length of output: 770
🏁 Script executed:
# Check what types of errors fetch-args.test.ts is testing
grep -B 5 "expect(async () => await fetch" test/js/web/fetch/fetch-args.test.ts | head -40Repository: oven-sh/bun
Length of output: 1002
🏁 Script executed:
# Check what fetch() returns - it should return a JSPromise
grep -A 20 "pub fn Bun__fetch_" src/bun.js/webcore/fetch.zig | head -30Repository: oven-sh/bun
Length of output: 626
🏁 Script executed:
# Look for how fetch returns - does it wrap errors in promises?
grep "JSPromise\|Promise.dangerously" src/bun.js/webcore/fetch.zig | head -20Repository: oven-sh/bun
Length of output: 2305
🏁 Script executed:
# Check if bodyUsed check uses .throw() or wraps in JSPromise
rg "bodyValue.*Used.*throw" src/bun.js/webcore/fetch.zig -B 2 -A 2Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Look for the ERR().throw() pattern to understand its behavior
rg "\.ERR.*\.throw\(\)" src/bun.js/webcore/fetch.zig | head -5Repository: oven-sh/bun
Length of output: 411
🏁 Script executed:
# Check if .throw() returns a JSPromise or throws synchronously
rg "fn throw|\.throw\(" src/bun.js/ -t zig -B 1 -A 1 | head -60Repository: oven-sh/bun
Length of output: 4449
🏁 Script executed:
# Look for the throw() method definition to understand its behavior
rg "fn throw\(" src/bun.js/bindings/ -t zig -A 5 | head -40Repository: oven-sh/bun
Length of output: 531
🏁 Script executed:
# Check how JSError.throw() works
fd "JSError" src/bun.js/bindings/ -t zig | xargs grep -l "fn throw" | head -3Repository: oven-sh/bun
Length of output: 225
🏁 Script executed:
# Examine a complete fetch call path to see error handling
sed -n '200,300p' src/bun.js/webcore/fetch.zig | head -50Repository: oven-sh/bun
Length of output: 1569
🏁 Script executed:
# Check if there's a wrapper around fetch that converts errors to promises
rg "Bun__fetch|fetch.*function" src/js/builtins/ -B 3 -A 3 | head -100Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Look at how fetch is exported/wrapped
rg "export.*fetch|Bun__fetch" src/js/builtins/ -A 5 | head -50Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Check the JSC binding layer - how JSError is handled
rg "bun.JSError" src/bun.js/bindings/JSGlobalObject.zig -B 2 -A 2 | head -40Repository: oven-sh/bun
Length of output: 1740
🏁 Script executed:
# Look for how JSError is handled when returned from a JSC host function
rg "toJSHostFn|JSHostFunction" src/bun.js/bindings/ -B 2 -A 2 | head -50Repository: oven-sh/bun
Length of output: 1245
🏁 Script executed:
# Check if .throw() in Zig actually throws synchronously or returns a JSError
rg "throwValue" src/bun.js/bindings/JSGlobalObject.zig -A 3Repository: oven-sh/bun
Length of output: 1732
🏁 Script executed:
# Look at how Bun__fetch is registered/exported as a function
grep -r "Bun__fetch" src/ --include="*.ts" --include="*.js" -B 2 -A 2 | head -30Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Check how toJSHostFn handles JSError returns
rg "toJSHostFn\|JSHostFnZig" src/bun.js/bindings/ -A 10 | head -60Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Verify the actual pattern used in body.test.ts to see what currently passes
rg "expect.*fetch.*body" test/js/web/fetch/body.test.ts -B 2 -A 2Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Look for other tests in body.test.ts that check fetch errors to see if there's a pattern
rg "test.*fetch.*throws" test/js/web/fetch/body.test.ts -A 5Repository: oven-sh/bun
Length of output: 821
🏁 Script executed:
# Look at how toJSHostFn converts return values - specifically JSError handling
rg "pub fn toJSHostFn|fn toJSHostFn" src/bun.js/bindings/ -A 15 | head -80Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Confirm whether fetch returns a JSPromise or throws synchronously by checking all return paths
rg "return.*fetch\|JSPromise\|ERR.*throw" src/bun.js/webcore/fetch.zig | grep -E "(return|ERR|JSPromise)" | head -30Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Look for examples of JSError handling in tests
rg "expect.*async.*throw" test/js/web/ --type ts -B 2 -A 2 | head -80Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Check if there are passing tests for consumed body errors
rg "bodyUsed\|body.*consumed" test/js/web/fetch/ -B 5 -A 5Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Examine how the body validation error is currently being tested
sed -n '710,760p' test/js/web/fetch/body.test.tsRepository: oven-sh/bun
Length of output: 1274
Use async wrapper to properly test fetch() rejection when request body is consumed.
fetch(req) returns a rejected promise when the request body is already used. The synchronous wrapper doesn't catch async rejections—use the async wrapper pattern to properly assert the error.
🩹 Suggested changes
- expect(() => fetch(req)).toThrow();
+ expect(async () => await fetch(req)).toThrow();Apply this change to all four test cases (lines 719, 730, 741, 752).
📝 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.
Suggested change
| test("await fetch(req) throws if req.body is already consumed (arrayBuffer)", async () => { | |
| const req = new Request("https://example.com/", { | |
| body: "test", | |
| method: "POST", | |
| }); | |
| await new Response(req.body).arrayBuffer(); | |
| expect(() => fetch(req)).toThrow(); | |
| expect(req.bodyUsed).toBe(true); | |
| }); | |
| test("await fetch(req) throws if req.body is already consumed (text)", async () => { | |
| const req = new Request("https://example.com/", { | |
| body: "test", | |
| method: "POST", | |
| }); | |
| await new Response(req.body).text(); | |
| expect(() => fetch(req)).toThrow(); | |
| expect(req.bodyUsed).toBe(true); | |
| }); | |
| test("await fetch(req) throws if req.body is already consumed (stream that has been read)", async () => { | |
| const req = new Request("https://example.com/", { | |
| body: "test", | |
| method: "POST", | |
| }); | |
| await req.body.getReader().read(); | |
| expect(() => fetch(req)).toThrow(); | |
| expect(req.bodyUsed).toBe(true); | |
| }); | |
| test("await fetch(req) throws if req.body is already consumed (stream)", async () => { | |
| const req = new Request("https://example.com/", { | |
| body: "test", | |
| method: "POST", | |
| }); | |
| req.body.getReader(); | |
| expect(() => fetch(req)).toThrow(); | |
| expect(req.bodyUsed).toBe(true); | |
| test("await fetch(req) throws if req.body is already consumed (arrayBuffer)", async () => { | |
| const req = new Request("https://example.com/", { | |
| body: "test", | |
| method: "POST", | |
| }); | |
| await new Response(req.body).arrayBuffer(); | |
| expect(async () => await fetch(req)).toThrow(); | |
| expect(req.bodyUsed).toBe(true); | |
| }); | |
| test("await fetch(req) throws if req.body is already consumed (text)", async () => { | |
| const req = new Request("https://example.com/", { | |
| body: "test", | |
| method: "POST", | |
| }); | |
| await new Response(req.body).text(); | |
| expect(async () => await fetch(req)).toThrow(); | |
| expect(req.bodyUsed).toBe(true); | |
| }); | |
| test("await fetch(req) throws if req.body is already consumed (stream that has been read)", async () => { | |
| const req = new Request("https://example.com/", { | |
| body: "test", | |
| method: "POST", | |
| }); | |
| await req.body.getReader().read(); | |
| expect(async () => await fetch(req)).toThrow(); | |
| expect(req.bodyUsed).toBe(true); | |
| }); | |
| test("await fetch(req) throws if req.body is already consumed (stream)", async () => { | |
| const req = new Request("https://example.com/", { | |
| body: "test", | |
| method: "POST", | |
| }); | |
| req.body.getReader(); | |
| expect(async () => await fetch(req)).toThrow(); | |
| expect(req.bodyUsed).toBe(true); |
🤖 Prompt for AI Agents
In `@test/js/web/fetch/body.test.ts` around lines 712 - 753, The tests assert
fetch(req) throws synchronously but fetch returns a rejected Promise; update
each of the four tests (the ones creating Request(...) and using
Response(req.body).arrayBuffer(), .text(), req.body.getReader().read(), and
req.body.getReader()) to use async rejection assertions: replace expect(() =>
fetch(req)).toThrow() with await expect(fetch(req)).rejects.toThrow() (or
equivalent await/then/reject handling), keeping the existing Request, Response,
req.body.getReader() calls and the final expect(req.bodyUsed).toBe(true).
Comment on lines
+521
to
+526
| // Create raw deflate (no zlib wrapper) | ||
| const compressed = deflateRawSync(deflateTestData); | ||
|
|
||
| // Verify it doesn't have zlib header (shouldn't start with 0x78) | ||
| expect(compressed[0]).not.toBe(0x78); | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "fetch-gzip.test.ts" -type fRepository: oven-sh/bun
Length of output: 93
🏁 Script executed:
cat -n ./test/js/web/fetch/fetch-gzip.test.ts | sed -n '510,540p'Repository: oven-sh/bun
Length of output: 1242
🏁 Script executed:
head -n 30 ./test/js/web/fetch/fetch-gzip.test.tsRepository: oven-sh/bun
Length of output: 966
🏁 Script executed:
cat -n ./test/js/web/fetch/fetch-gzip.test.ts | sed -n '1,60p' | grep -A 5 "deflateTestData"Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
rg "deflateTestData" ./test/js/web/fetch/fetch-gzip.test.ts -nRepository: oven-sh/bun
Length of output: 681
🏁 Script executed:
cat -n ./test/js/web/fetch/fetch-gzip.test.ts | sed -n '485,550p'Repository: oven-sh/bun
Length of output: 2772
🌐 Web query:
raw DEFLATE RFC 1951 first byte format
💡 Result:
Short answer:
- A raw DEFLATE stream packs bits LSB-first within each byte (bit numbering 0 = least-significant bit). [1][2]
- Each block begins with a 3‑bit header: bit 0 = BFINAL, bits 1–2 = BTYPE (00/01/10/11). These three bits are the first three bits of the stream (they may not start on a byte boundary). [1][2]
- Non‑Huffman multi‑bit fields (e.g., extra length/distance bits) are read LSB‑first; Huffman codes are packed with their MSB first (i.e., bit order differs). [1][2][3]
References: RFC 1951 (DEFLATE) and its errata. [1][2][3]
🌐 Web query:
Node.js deflateRawSync first byte possible values RFC 1951
💡 Result:
Short answer: the first three bits of a raw DEFLATE stream are BFINAL (1 bit) and BTYPE (2 bits). BTYPE can be 00 (no compression), 01 (fixed Huffman), 10 (dynamic Huffman) or 11 (reserved/error). So valid streams have BTYPE ∈ {00,01,10} and BFINAL ∈ {0,1} — six valid bit-combinations (00/01/10 × 0/1); BTYPE=11 is reserved/invalid. These three bits are stored least-significant-bit first within the first byte (so e.g. BFINAL=1,BTYPE=01 produces low‑three bits 011). (datatracker.ietf.org)
If you need exact example byte values for particular BFINAL/BTYPE combinations, tell me which combination and I’ll show possible hex bytes.
Citations:
🌐 Web query:
Node.js zlib deflateRawSync deterministic output consistency across versions
💡 Result:
Short answer
- zlib.deflateRawSync is deterministic for a given zlib implementation, build, and options (same input bytes + same compression parameters + same feeding/chunking -> same output). However, there is no cross‑version guarantee: different Node.js releases, different zlib builds/versions (or alternatives like zlib-ng), different defaults (windowBits/memLevel/strategy) or different input chunking can produce different compressed bytes. [1][2][3][4]
What to do if you need reproducible output
- Pin the exact Node.js binary (or the exact zlib library Node links to) and the zlib options you use (level, windowBits, memLevel, strategy). [1][2]
- Ensure you feed the compressor identically (same buffer sizes / flush behavior). Some implementations/changes affect output when input is chunked differently. [3][4]
- If you must be resilient to library upgrades, use an explicitly deterministic compressor or an implementation that documents a reproducibility mode, or canonicalize by decompressing then recompressing with your pinned compressor. [3][4]
Sources
- Node.js zlib documentation (notes on options/behaviour and zlib changes). [1]
- zlib manual (deflate semantics, parameters that affect output). [2]
- Reports/discussions on non‑reproducible deflate output across zlib implementations/versions (reproducible builds / zlib-ng discussions). [3][4]
🏁 Script executed:
node --versionRepository: oven-sh/bun
Length of output: 62
🏁 Script executed:
cat -n ./test/js/web/fetch/fetch-gzip.test.ts | sed -n '516,545p'Repository: oven-sh/bun
Length of output: 1238
Remove brittle raw-deflate header assertion.
The first byte check at line 525 assumes the compressor output will never produce 0x78 at the start, but this assertion doesn't contribute to testing whether raw deflate decompression works. The actual validation is the end-to-end assertion at line 538 (expect(text).toBe(deflateTestData.toString())), which already confirms correctness. Remove the intermediate byte check to avoid unnecessary brittleness.
🩹 Suggested change
- // Verify it doesn't have zlib header (shouldn't start with 0x78)
- expect(compressed[0]).not.toBe(0x78);📝 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.
Suggested change
| // Create raw deflate (no zlib wrapper) | |
| const compressed = deflateRawSync(deflateTestData); | |
| // Verify it doesn't have zlib header (shouldn't start with 0x78) | |
| expect(compressed[0]).not.toBe(0x78); | |
| // Create raw deflate (no zlib wrapper) | |
| const compressed = deflateRawSync(deflateTestData); | |
🤖 Prompt for AI Agents
In `@test/js/web/fetch/fetch-gzip.test.ts` around lines 521 - 526, Remove the
brittle byte-check asserting the first byte isn't 0x78 in the raw-deflate test:
delete the line with expect(compressed[0]).not.toBe(0x78); and keep the actual
end-to-end validation that uses deflateRawSync(compressed)/deflateTestData and
the final assertion expect(text).toBe(deflateTestData.toString()); this ensures
the test verifies decompression correctness without making fragile assumptions
about the compressor output.
Comment on lines
+2517
to
+2531
| const [bunStdout, bunExitCode] = await Promise.all([new Response(bunProc.stdout).text(), bunProc.exited]); | ||
|
|
||
| expect(bunExitCode).toBe(0); | ||
| const bunResult = JSON.parse(bunStdout.trim()); | ||
|
|
||
| // The bug: Bun follows the redirect even though redirect: "manual" was specified | ||
| // Expected: status=302, url=original, redirected=false | ||
| // Actual (bug): status=200, url=target, redirected=true | ||
| expect(bunResult).toEqual({ | ||
| status: 302, | ||
| url: `${server.url}/redirect`, | ||
| redirected: false, | ||
| location: "/target", | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Check parsed stdout before exit code for better diagnostics.
Reorder the assertions so stdout parsing/expectations run before the exit-code check.
🩹 Suggested change
- expect(bunExitCode).toBe(0);
const bunResult = JSON.parse(bunStdout.trim());
@@
expect(bunResult).toEqual({
status: 302,
url: `${server.url}/redirect`,
redirected: false,
location: "/target",
});
+ expect(bunExitCode).toBe(0);As per coding guidelines, stdout assertions should precede exit-code checks in spawned process tests.
🤖 Prompt for AI Agents
In `@test/js/web/fetch/fetch.test.ts` around lines 2517 - 2531, The test parses
and asserts on bunStdout after checking bunExitCode, which hides stdout
diagnostics when the spawned process fails; reorder the checks in the test so
you await and parse bunStdout (the Promise.all result producing bunStdout from
new Response(bunProc.stdout).text()) and run the JSON.parse and
expect(bunResult).toEqual(...) assertions before asserting
expect(bunExitCode).toBe(0) (keep the same bunProc / bunStdout / bunExitCode
variables and the existing expected object, just move the exit-code assertion to
after the stdout parsing and expectations).
Contributor
|
Closing this PR because it has been inactive for more than 90 days. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
test/regression/issue/into their appropriate existing test filesChanges
This PR reorganizes regression tests by moving them into their natural home based on what they test:
test/js/bun/http/serve.test.tstest/js/node/child_process/child_process.test.tstest/js/web/fetch/body.test.tstest/js/bun/test/expect.test.jstest/js/bun/test/jest-hooks.test.tstest/js/bun/css/css.test.ts,color.test.tstest/js/bun/s3/s3.test.tstest/js/bun/shell/bunshell.test.tstest/js/node/tty.test.tstest/js/bun/websocket/websocket-server.test.tstest/bundler/*.test.tsTest Files Kept in
test/regression/issue/86 tests remain in the regression folder because they:
Traceability
All moved tests include comments referencing their original issue numbers:
// Regression test for #XXXXXThis ensures the context of why each test exists is preserved.
Test plan
USE_SYSTEM_BUN=1where applicable🤖 Generated with Claude Code