test: enable 54 already-passing Node compat tests#33658
test: enable 54 already-passing Node compat tests#33658nathanwhitbot wants to merge 6 commits intodenoland:mainfrom
Conversation
nathanwhitbot
left a comment
There was a problem hiding this comment.
Scope: first commit c001540 (fix(ext/node): support signal option in fs.watch/fs.promises.watch) is the open #33650 work, not part of this PR's intent. Same pattern @nathanwhit flagged on #33653 — please drop or rebase past it so the PR is purely the 54 config.jsonc enables.
nathanwhitbot
left a comment
There was a problem hiding this comment.
Update: scope got worse, not better. New commit fd173ad4 (test: catch AbortError from fs.promises.watch async iterator) is also from #33650. PR now has 3 unrelated commits: c001540, fd173ad, plus the upstream merge. Files touched include ext/node/polyfills/fs.ts (+36/-4) and tests/unit_node/_fs/_fs_watch_test.ts — both should be 0 if the PR were purely test-enables.
@nathanwhit pinging since the bot isn't dropping the unrelated commits despite the prior scope note.
|
Windows fails: 3 tests in this PR don't pass on
Per @bartlomieju's note on #33593: drop them from the PR rather than gate with |
|
2 more Windows fails on |
|
Linux fails are infra: |
|
Third failure on run 25081051632 is |
fibibot
left a comment
There was a problem hiding this comment.
Two concerns before this lands.
Title doesn't match the diff
The PR is titled test: enable 54 already-passing Node compat tests but the diff also includes production code changes in ext/node/polyfills/fs.ts:
- New
signal?: AbortSignalmember on thewatchOptionstype - AbortSignal handling in
fs.watch(close-on-abort, listener registration, deferred close on pre-aborted) - AbortSignal-aware
next()infs.promises.watchthat throwsAbortErrorwithcause: signal?.reason
If those are needed to make some of the enrolled tests pass, the PR title should be fix(ext/node): support signal option in fs.watch / fs.promises.watch and enable 54 compat tests (or split into two PRs). Pure test-enrollment PRs that touch production code under that title are easy to miss in review later.
Duplicates #33650
This is the same substance as #33650 (also fix(ext/node): support signal option in fs.watch/fs.promises.watch). That PR has had two review iterations and is the more complete version — it includes things this one doesn't:
validateAbortSignal(options?.signal, "options.signal")— defensive validation throwingERR_INVALID_ARG_TYPEfor non-AbortSignal valuesfsWatcher.once("close", () => signal.removeEventListener("abort", onAbort))cleanup so the abort listener doesn't leak when the watcher closes naturally first- A
cleanupAbort()helper called from three places innext()(top, afteriterResult.done, and from a newreturn()method) sofor awaitloops thatbreakearly also clean up the listener
The version here would land an earlier, less-careful copy of that code. Either rebase on top of #33650 once it lands, or pick up the cleanup helpers here. Whichever one merges second will need to remove the duplicate fs.ts changes.
CI
129 success, 4 failure, 2 skipped:
test integration (1/2) debug linux-x86_64andtest specs (2/2) debug linux-x86_64— both exit code 8 at sysroot decompression (Decompressing sysroot...), pure infrastructure flake unrelated to this PR.wpt release linux-x86_64— single failed test/workers/Worker-timeout-decreasing-order.html("Unhandled error in child worker"). Unrelated to fs.watch.ci status— aggregate of the above.
So the substance of the test enrollment looks fine; the actual relevant test node_compat and test unit_node shards are all green. The blockers are the title mismatch and the #33650 overlap, not the CI failures.
c160283 to
34a8ddc
Compare
|
Force-pushed Reset the branch to The |
|
After scope-down, |
Test exercises fs.promises.watch({ signal }) which depends on the signal
option support in denoland#33650 (still open). Re-add in a follow-up after denoland#33650
lands.
# Conflicts: # tests/node_compat/config.jsonc
|
Single-shard fail on |
fibibot
left a comment
There was a problem hiding this comment.
LGTM. Pure config-jsonc addition (48 lines added, no production code touched). The 54 newly-enabled tests all passed in CI; the single test node_compat (3/3) debug windows-x86_64 failure is on parallel/test-dns-resolver-max-timeout.js (timeout1: 507, timeout2: 516 — millisecond-granular > assertion that the Windows OS scheduler doesn't always satisfy) which is already enabled in main and not in this PR's diff. Same recurring flake that's hit ~6 PRs today.
The macOS / Linux / Windows-aarch64 shards all green confirm the new tests work. Worth a separate cleanup later to either mark test-dns-resolver-max-timeout.js as flaky: true in config or bump its timing tolerance, since it's been blocking ~half the daily PRs on a single millisecond-comparison assertion.
Summary
Enables 54 Node.js compat tests that already pass on main without any code changes. No production code touched — just
tests/node_compat/config.jsoncentries.parallel/test-net-connect-nodelay.jsparallel/test-net-connect-options-path.jsparallel/test-net-listen-exclusive-random-ports.jsparallel/test-net-listen-handle-in-cluster-2.jsparallel/test-net-pingpong.jsparallel/test-net-server-close-before-ipc-response.jsparallel/test-net-write-fully-async-hex-string.jsparallel/test-child-process-send-utf8.jsparallel/test-child-process-server-close.jsparallel/test-module-builtin.jsparallel/test-worker-http2-stream-terminate.jsparallel/test-dgram-cluster-bind-error.jsparallel/test-dgram-cluster-close-during-bind.jsparallel/test-dgram-cluster-close-in-listening.jsparallel/test-dgram-unref-in-cluster.jsparallel/test-fs-append-file.jsparallel/test-fs-watch-recursive-sync-write.jsparallel/test-fs-watch-recursive-update-file.jsparallel/test-fs-write-file-sync.jsparallel/test-https-agent-session-injection.jsparallel/test-https-client-reject.jsparallel/test-https-drain.jsparallel/test-https-localaddress.jsparallel/test-https-server-close-destroy-timeout.jsparallel/test-http2-exceeds-server-trailer-size.jsparallel/test-http2-generic-streams.jsparallel/test-http2-options-max-reserved-streams.jsparallel/test-http2-reset-flood.jsparallel/test-http2-server-push-stream-errors-args.jsparallel/test-http2-server-socket-destroy.jsparallel/test-http2-util-nghttp2error.jsparallel/test-diagnostics-channel-http2-client-stream-close.jsparallel/test-diagnostics-channel-http2-client-stream-finish.jsparallel/test-diagnostics-channel-http2-server-stream-close.jsparallel/test-diagnostics-channel-http2-server-stream-created-start-timing.jsparallel/test-diagnostics-channel-http2-server-stream-created.jsparallel/test-diagnostics-channel-http2-server-stream-finish.jsparallel/test-diagnostics-channel-http2-server-stream-start.jsparallel/test-fastutf8stream-end.jsparallel/test-fastutf8stream-flush-sync.jsparallel/test-fastutf8stream-flush.jsparallel/test-fastutf8stream-mode.jsparallel/test-fastutf8stream-partial-write-utf8.jsparallel/test-fastutf8stream-periodicflush.jsparallel/test-fastutf8stream-reopen.jsparallel/test-file-write-stream.jsparallel/test-file-write-stream4.jsparallel/test-perf-hooks-timerify-error.jsparallel/test-perf-hooks-timerify-invalid-args.jsparallel/test-perf-hooks-timerify-multiple-wrapping.jsparallel/test-perf-hooks-timerify-return-value.jsparallel/test-promise-unhandled-throw.jsparallel/test-source-map-invalid-url.jsparallel/test-v8-collect-gc-profile-in-worker.jsTest plan
cargo test --test node_compatrun on Linux confirms no regressions from the additions