test: enable stale todo tests that now pass - #34857
Conversation
Unskip tests across 20 files whose underlying behavior has since been fixed. Each was verified passing against a debug build at HEAD. Two tests required small input corrections to be meaningful: - node-tls-cert.test.ts: update expected error code to ERR_SSL_PEER_DID_NOT_RETURN_A_CERTIFICATE (matches Node.js 26). - expect.test.js 'to return undefined' block: give the matchers valid arguments (a mock instead of a bare arrow, required nth/count args) so the assertion actually exercises the return value. Left unchanged because they still fail: - url-format-invalid-input.test.js, url-parse-invalid-input.test.js (error code / message mismatches) - diagnostics_channel.test.ts subscriber-error / bind-store cases (depend on uncaughtException semantics inside bun:test) - mock-module.test.ts 'adding a default on a module with no default' - serve.test.ts 'text from JS throws on start with no error handler' (uncaught error fails the test) - expect.test.js 'toContainEqual to return undefined' (toContainEqual returns the Expect instance, not undefined) Not touched: the two placeholder skips with no test body (websocket-server terminate-inside-open, websocket-permessage-deflate compressed-control-frames) and the v8-date-parser todoOnWindows guard.
WalkthroughChangesThe pull request enables previously pending tests across Bun runtime APIs, Jest-compatible matchers, Node and Deno compatibility APIs, Fetch behavior, and Worker termination. It also adds Windows gating for one hostname test and updates a TLS error expectation. Runtime and compatibility test coverage
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 8:05 PM PT - Jul 20th, 2026
@Jarred-Sumner, your commit 8d06195 is building: |
|
Found 7 issues this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nit: verified the expect.test.js rewrites reference the shared mocked fixture (defined and called once at ~line 4734), so the new toHaveNthReturnedWith(1, ...) / toHaveReturnedTimes(1) assertions are satisfiable. Also checked the node-tls-cert.test.ts error-code change — the connect() helper rejects with client.err || server.err, and the server-side tlsClientError is ERR_SSL_PEER_DID_NOT_RETURN_A_CERTIFICATE under BoringSSL, so the new expectation is consistent. CI (#76445) will be the real arbiter for the 29 unskips across platforms.
Extended reasoning...
This PR is a test-only housekeeping pass that unskips 29 .todo tests across 20 files, with two non-mechanical adjustments (TLS error code, expect.test.js matcher arguments). One nit was found regarding the fetch.stream.test.ts unskip removing a documented-flaky marker; that inline comment covers the primary concern. I additionally verified the two substantive test-body edits are internally consistent with their surrounding fixtures. Not approving because CI hasn't reported yet and the PR itself defers correctness to CI — a maintainer should confirm the multi-platform run is green before merge.
Windows can't bind a listening socket to hostname "0" (POSIX resolves it to 0.0.0.0), so the coercion assertion never gets to run there.
There was a problem hiding this comment.
LGTM — thanks for looping the fetch.stream variants 200× and checking the fetch-tls-cert sibling. Test-only housekeeping; CI is the real gate for any residual platform sensitivity.
What was reviewed:
- All 29 un-todo'd tests are pure
.todo→test/itswaps except the three documented adjustments (TLS error code, expect.test.js matcher args, WindowsskipIfon hostname0). - Prior flakiness concern on
fetch.stream.test.tsaddressed with a 200-iteration debug loop + green CI lane. fetch-tls-cert.test.tssibling confirmed still blocked onfetch({tls:{maxVersion}})and noted in the PR description.
Extended reasoning...
Overview
Test-only PR: removes .todo from 29 tests across 20 files whose underlying behavior now passes at HEAD, verified via bun test --todo. Three non-mechanical adjustments: (1) node-tls-cert.test.ts expected error code updated to the server-side ERR_SSL_PEER_DID_NOT_RETURN_A_CERTIFICATE (verified against Node 26); (2) seven expect.test.js "to return undefined" cases given valid matcher args so they exercise the return value instead of throwing on missing args; (3) bun-serve-args.test.ts hostname-0 test gated with skipIf(isWindows) since Windows can't bind to hostname "0".
Security risks
None. No runtime code touched; only test files. The TLS error-code change is asserting an observed value, not altering TLS behavior.
Level of scrutiny
Low. This is mechanical test housekeeping. The blast radius of a mistake is a red CI lane, not a shipped bug — a newly-enabled test that flakes gets re-.todo'd. No CODEOWNER paths, no production code.
Other factors
Both nits from my prior review pass are resolved: the author looped the previously-flaky-commented fetch.stream abort variants 200× on debug HEAD with zero failures (and cited a green CI build), and confirmed the fetch-tls-cert.test.ts sibling is still legitimately blocked on fetch({tls:{maxVersion}}) plumbing (now noted in the PR description). The e0a283d2 commit adding the Windows guard is a standard platform skipIf with an explanatory comment. The PR description exhaustively documents what was enabled, what was adjusted and why, and what was intentionally left .todo.
|
CI summary across the three runs:
The diff is green on every lane that actually ran it. Ready for review. |
There was a problem hiding this comment.
LGTM — test-only housekeeping, CI-verified across three builds, and both prior nits (fetch.stream flake evidence, fetch-tls-cert sibling) were addressed with concrete evidence.
What was reviewed:
- All 29
.todo→ active flips are mechanical; the two content edits (TLS error code →ERR_SSL_PEER_DID_NOT_RETURN_A_CERTIFICATE, expect.test.js matcher args) are documented and match neighboring fixtures. - fetch.stream.test.ts flake concern: author looped 200× on debug HEAD with zero failures; build #76445 cleared all lanes.
- Windows
hostname: 0bind failure caught in CI and fixed withskipIf(isWindows)+ reason comment. fetch-tls-cert.test.tssibling checked and confirmed still blocked (different code path); noted in PR description.
Extended reasoning...
Overview
Test-only PR flipping 29 test.todo/it.todo entries to active across 20 files. No production code touched. Two test bodies were adjusted while enabling: the TLS1.2-no-cert expected error code (updated to match what both Node 26 and Bun actually surface via tlsClientError), and seven expect.test.js "to return undefined" cases that were previously calling matchers with missing required arguments (now given the same mocked fixture / args as neighbors). One Windows-specific bind failure surfaced in CI and was fixed with a documented skipIf(isWindows).
Security risks
None. No production code, no auth/crypto/permissions logic. The TLS test change updates an assertion to match observed Node.js behavior; it does not touch TLS implementation.
Level of scrutiny
Low-to-medium. Enabling tests only increases coverage — the failure mode is CI flakiness, not shipped bugs. The two content edits warranted a closer look: the TLS error-code change is verified against Node 26 and the PR explains the server-side vs client-side distinction; the expect.test.js edits give matchers valid arguments so the "returns undefined" invariant is actually exercised (previously they threw before reaching the assertion). Both are correct and well-documented.
Other factors
- Three CI builds: #76445 found the Windows hostname issue (fixed in e0a283d), #76468 passed 261 lanes (Windows canceled by Azure outage), #76558 passed 269 lanes including Windows x64/x64-baseline. Remaining red is windows-aarch64 build-rust infra timeout plus known unrelated flakes.
- My two prior inline nits are resolved: the fetch.stream.test.ts flake concern was answered with a 200-iteration local loop + all-lane CI pass, and the
fetch-tls-cert.test.tssibling was checked and confirmed still legitimately blocked (fetch({tls:{maxVersion}})not plumbed) with a note added to the PR description. - The PR description explicitly enumerates what was left as
.todoand why, satisfying REVIEW.md's "if a site is intentionally excluded, say so."
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/test/spyMatchers.test.ts`:
- Around line 596-599: Strengthen the assertions in the spy-received matcher
tests around createSpy and jestExpect, including the corresponding case near the
second referenced test, so they verify the matcher-specific error type or a
precise stable message fragment rather than accepting any thrown error. Preserve
the existing invalid-received validation scenarios while proving they fail
through the intended matcher path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5e23b924-fa2b-4193-ae57-960569110e79
📒 Files selected for processing (20)
test/js/bun/http/bun-serve-args.test.tstest/js/bun/http/serve.test.tstest/js/bun/jsc/domjit.test.tstest/js/bun/plugin/plugins.test.tstest/js/bun/resolve/resolve.test.tstest/js/bun/test/expect.test.jstest/js/bun/test/jest-extended.test.jstest/js/bun/test/spyMatchers.test.tstest/js/deno/url/url.test.tstest/js/node/http/node-http-connect.test.tstest/js/node/process/process.test.jstest/js/node/tls/node-tls-cert.test.tstest/js/node/url/url-canParse-whatwg.test.jstest/js/node/url/url-fileurltopath.test.jstest/js/node/url/url-parse-format.test.jstest/js/node/url/url-parse-query.test.jstest/js/web/fetch/exiting.test.tstest/js/web/fetch/fetch-leak.test.tstest/js/web/fetch/fetch.stream.test.tstest/js/web/workers/worker.test.ts
2 participants
Housekeeping pass: unskip
test.todo/it.todoentries whose underlying behavior has since been fixed. Each was verified passing against a debug build at HEAD usingbun test --todo(which fails when a todo-marked test body passes).Enabled (29 tests across 20 files)
test/js/web/fetch/exiting.test.tstest/js/web/fetch/fetch.stream.test.tstest/js/web/fetch/fetch-leak.test.tstest/js/bun/http/bun-serve-args.test.tstest/js/bun/http/serve.test.tstest/js/node/http/node-http-connect.test.tstest/js/node/tls/node-tls-cert.test.tstest/js/node/url/url-parse-query.test.jstest/js/node/url/url-canParse-whatwg.test.jstest/js/node/url/url-fileurltopath.test.jstest/js/node/url/url-parse-format.test.jstest/js/deno/url/url.test.tstest/js/node/process/process.test.jstest/js/web/workers/worker.test.tstest/js/bun/resolve/resolve.test.tstest/js/bun/plugin/plugins.test.tstest/js/bun/jsc/domjit.test.tstest/js/bun/test/spyMatchers.test.tstest/js/bun/test/jest-extended.test.jstest/js/bun/test/expect.test.jsAdjusted while enabling
node-tls-cert.test.ts: the expected error code was stale. Both Node.js 26 and Bun surface the server-sidetlsClientErrorhere, which isERR_SSL_PEER_DID_NOT_RETURN_A_CERTIFICATE(the oldERR_SSL_SSLV3_ALERT_HANDSHAKE_FAILUREpredates the OpenSSL rename and was the client-side code anyway).expect.test.js"to return undefined" block: seven of the nine todo entries were calling matchers with no arguments (e.g.expect({}).toHaveProperty()), so they threw before the return value could be checked. Gave them the samemockedfixture / required args used by the neighboring tests.Left as
test.todo(still fail at HEAD)url-format-invalid-input.test.jsandurl-parse-invalid-input.test.jsinvalid input: error message wording still differs.diagnostics_channel.test.tscan handle subscriber errors/can use bind store: depend onuncaughtExceptionhandling insidebun:test, which still short-circuitsmustCall.mock-module.test.tsadding a default on a module with no default.serve.test.tstext from JS throws on start with no error handler: the uncaught error still marks the test as failed.expect.test.jstoContainEqual to return undefined:toContainEqualreturns theExpectinstance, notundefined.Not touched
websocket-server.test.tsterminate() inside open() calls close()andwebsocket-permessage-deflate.test.tsWebSocket client rejects compressed control frameshave no real test body; enabling them would assert nothing.v8-date-parser.test.jstodoOnWindows: already runs on Linux/macOS; leaving the Windows guard in place for a Windows-verified follow-up.fetch-tls-cert.test.tssibling of the TLS1.2-no-cert test: still blocked becausefetch({ tls: { maxVersion } })is not plumbed through yet (throwsERR_INVALID_ARG_TYPE), so its TODO comment remains accurate.no test proof · iteration 3 · Platform-specific test-only change; deferring to CI.