test(node-suite): expand async hooks parity coverage#6448
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR expands the ChangesAsync hooks parity coverage
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test-parity/node-suite/async_hooks/storage/disable-one-pending-other.ts (1)
12-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsistently format store values.
For consistency with line 28 and the preceding
first.getStore()call, consider wrappingsecond.getStore()inString()as well.♻️ Proposed refactor
console.log( "pending isolated stores:", String(first.getStore()), - second.getStore(), + String(second.getStore()), );🤖 Prompt for 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. In `@test-parity/node-suite/async_hooks/storage/disable-one-pending-other.ts` around lines 12 - 16, Update the pending isolated stores log in the relevant async-hooks test to wrap second.getStore() with String(), matching the formatting already used for first.getStore() and the later log.test-parity/node-suite/async_hooks/hooks/multiple-hooks.ts (1)
7-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract duplicate hook creation into a factory function.
The
init,before, andaftercallbacks for both hooks are identical except for the target events array they append to. Consider extracting this configuration into a helper function to eliminate duplication.♻️ Proposed refactor
-const first = createHook({ - init(asyncId, type) { - if (type === "ParityMultipleHooks") { - targetId = asyncId; - firstEvents.push("init"); - } - }, - before(asyncId) { - if (asyncId === targetId) firstEvents.push("before"); - }, - after(asyncId) { - if (asyncId === targetId) firstEvents.push("after"); - }, -}).enable(); -const second = createHook({ - init(asyncId, type) { - if (type === "ParityMultipleHooks") { - targetId = asyncId; - secondEvents.push("init"); - } - }, - before(asyncId) { - if (asyncId === targetId) secondEvents.push("before"); - }, - after(asyncId) { - if (asyncId === targetId) secondEvents.push("after"); - }, -}).enable(); +function createTrackingHook(events: string[]) { + return createHook({ + init(asyncId, type) { + if (type === "ParityMultipleHooks") { + targetId = asyncId; + events.push("init"); + } + }, + before(asyncId) { + if (asyncId === targetId) events.push("before"); + }, + after(asyncId) { + if (asyncId === targetId) events.push("after"); + }, + }); +} + +const first = createTrackingHook(firstEvents).enable(); +const second = createTrackingHook(secondEvents).enable();🤖 Prompt for 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. In `@test-parity/node-suite/async_hooks/hooks/multiple-hooks.ts` around lines 7 - 34, Extract the duplicated async hook configuration into a factory function that accepts the target events array, preserves the shared targetId checks and callback behavior, and returns an enabled hook. Replace the separate inline configurations for first and second with factory calls using firstEvents and secondEvents.
🤖 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.
Nitpick comments:
In `@test-parity/node-suite/async_hooks/hooks/multiple-hooks.ts`:
- Around line 7-34: Extract the duplicated async hook configuration into a
factory function that accepts the target events array, preserves the shared
targetId checks and callback behavior, and returns an enabled hook. Replace the
separate inline configurations for first and second with factory calls using
firstEvents and secondEvents.
In `@test-parity/node-suite/async_hooks/storage/disable-one-pending-other.ts`:
- Around line 12-16: Update the pending isolated stores log in the relevant
async-hooks test to wrap second.getStore() with String(), matching the
formatting already used for first.getStore() and the later log.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ec7bb2d7-f866-4316-a925-8f8b54fc727d
📒 Files selected for processing (26)
test-parity/node-suite/async_hooks/README.mdtest-parity/node-suite/async_hooks/hooks/enable-disable.tstest-parity/node-suite/async_hooks/hooks/multiple-hooks.tstest-parity/node-suite/async_hooks/hooks/resource-argument.tstest-parity/node-suite/async_hooks/hooks/scope-throw-lifecycle.tstest-parity/node-suite/async_hooks/integrations/crypto-derive.tstest-parity/node-suite/async_hooks/integrations/crypto-random-bytes.tstest-parity/node-suite/async_hooks/integrations/crypto-random-fill.tstest-parity/node-suite/async_hooks/integrations/events-emitter.tstest-parity/node-suite/async_hooks/integrations/fs-callback-chain.tstest-parity/node-suite/async_hooks/integrations/fs-promises-chain.tstest-parity/node-suite/async_hooks/integrations/fs-streams.tstest-parity/node-suite/async_hooks/integrations/stream-finished.tstest-parity/node-suite/async_hooks/integrations/stream-readable.tstest-parity/node-suite/async_hooks/integrations/stream-transform.tstest-parity/node-suite/async_hooks/integrations/stream-writable.tstest-parity/node-suite/async_hooks/integrations/timers-promises.tstest-parity/node-suite/async_hooks/integrations/util-promisify.tstest-parity/node-suite/async_hooks/integrations/zlib-brotli.tstest-parity/node-suite/async_hooks/integrations/zlib-gzip-gunzip.tstest-parity/node-suite/async_hooks/resource/async-scope-storage.tstest-parity/node-suite/async_hooks/static/bind-concurrent.tstest-parity/node-suite/async_hooks/static/snapshot-async.tstest-parity/node-suite/async_hooks/storage/disable-one-pending-other.tstest-parity/node-suite/async_hooks/storage/exit-async-rejection.tstest-parity/node_suite_baseline.json
🚧 Files skipped from review as they are similar to previous changes (1)
- test-parity/node_suite_baseline.json
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test-parity/node-suite/async_hooks/propagation/dynamic-import.ts (1)
7-11: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider using a
finallyblock for global state cleanup.While a failure in this test fixture will likely exit the process anyway, it's generally best practice to clean up temporary global mutations in a
finallyblock to guarantee restoration if theimportor execution throws.🛠️ Proposed refactor
-const observed = await storage.run("dynamic-import", async () => { - const namespace = await import(source); - return namespace.store; -}); -delete (globalThis as any).parityDynamicImportStore; +let observed; +try { + observed = await storage.run("dynamic-import", async () => { + const namespace = await import(source); + return namespace.store; + }); +} finally { + delete (globalThis as any).parityDynamicImportStore; +}🤖 Prompt for 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. In `@test-parity/node-suite/async_hooks/propagation/dynamic-import.ts` around lines 7 - 11, Wrap the dynamic import and storage observation in a try/finally construct so the temporary global state is always cleaned up. Keep the existing storage.run behavior and delete parityDynamicImportStore in the finally block, including when import(source) or the callback throws.test-parity/node-suite/async_hooks/integrations/fs-access.ts (1)
6-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEnsure test file cleanup even if an error occurs.
If
accessreturns an error and the promise rejects, theunlinkSynccall will be bypassed, leaving the temporary file behind on the filesystem. Wrap the asynchronous operation in atry...finallyblock to guarantee cleanup.♻️ Proposed refactor
-await storage.run( - "fs-access", - () => - new Promise<void>((resolve, reject) => { - access(path, constants.R_OK, (error) => { - console.log("fs.access store:", storage.getStore()); - if (error) return reject(error); - resolve(); - }); - }), -); -unlinkSync(path); -console.log("fs.access outside:", String(storage.getStore())); +try { + await storage.run( + "fs-access", + () => + new Promise<void>((resolve, reject) => { + access(path, constants.R_OK, (error) => { + console.log("fs.access store:", storage.getStore()); + if (error) return reject(error); + resolve(); + }); + }), + ); +} finally { + unlinkSync(path); +} +console.log("fs.access outside:", String(storage.getStore()));🤖 Prompt for 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. In `@test-parity/node-suite/async_hooks/integrations/fs-access.ts` around lines 6 - 18, Wrap the await storage.run operation in a try...finally block so unlinkSync(path) always executes, including when access rejects with an error. Keep the existing access callback, logging, rejection, and post-cleanup store logging behavior unchanged.
🤖 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-parity/node-suite/async_hooks/integrations/fs-directory.ts`:
- Around line 1-5: Update the temporary path setup around the path constant and
realpath comparison to resolve the temporary directory base with realpathSync
before constructing the expected directory path. Import and use realpathSync
alongside the existing fs imports so resolved and expected paths match on macOS
and Linux.
In `@test-parity/node-suite/async_hooks/providers/http-client-events.ts`:
- Around line 15-42: Guarantee cleanup of servers and client/request handles
across rejected and socket-error paths. In
test-parity/node-suite/async_hooks/providers/http-client-events.ts:15-42, wrap
storage.run in try/finally and move server.close into finally; in
test-parity/node-suite/async_hooks/providers/http-request.ts:14-40, close the
server unconditionally after the request attempt; in
test-parity/node-suite/async_hooks/providers/net-connect.ts:12-29 and
test-parity/node-suite/async_hooks/providers/net-write-callbacks.ts:12-32,
destroy the client on failure and close the server in finally; in
test-parity/node-suite/async_hooks/providers/net-server-events.ts:9-27, retain
server and client handles and clean both up on rejection paths.
---
Nitpick comments:
In `@test-parity/node-suite/async_hooks/integrations/fs-access.ts`:
- Around line 6-18: Wrap the await storage.run operation in a try...finally
block so unlinkSync(path) always executes, including when access rejects with an
error. Keep the existing access callback, logging, rejection, and post-cleanup
store logging behavior unchanged.
In `@test-parity/node-suite/async_hooks/propagation/dynamic-import.ts`:
- Around line 7-11: Wrap the dynamic import and storage observation in a
try/finally construct so the temporary global state is always cleaned up. Keep
the existing storage.run behavior and delete parityDynamicImportStore in the
finally block, including when import(source) or the callback throws.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8afccbdb-ceaa-45e3-8403-a0951a852f72
📒 Files selected for processing (65)
scripts/node_suite_run.pytest-parity/node-suite/async_hooks/README.mdtest-parity/node-suite/async_hooks/hooks/cancelled-immediate-destroy.tstest-parity/node-suite/async_hooks/hooks/cancelled-timeout-destroy.tstest-parity/node-suite/async_hooks/hooks/disable-in-init.tstest-parity/node-suite/async_hooks/hooks/enable-in-init.tstest-parity/node-suite/async_hooks/hooks/multiple-hooks.tstest-parity/node-suite/async_hooks/hooks/promise-resolve.tstest-parity/node-suite/async_hooks/hooks/provider-crypto-lifecycle.tstest-parity/node-suite/async_hooks/hooks/provider-fs-lifecycle.tstest-parity/node-suite/async_hooks/hooks/provider-microtask-lifecycle.tstest-parity/node-suite/async_hooks/hooks/provider-nexttick-lifecycle.tstest-parity/node-suite/async_hooks/hooks/provider-timer-lifecycle.tstest-parity/node-suite/async_hooks/hooks/track-promises.tstest-parity/node-suite/async_hooks/integrations/crypto-generate-key-pair.tstest-parity/node-suite/async_hooks/integrations/crypto-generate-key.tstest-parity/node-suite/async_hooks/integrations/crypto-hkdf.tstest-parity/node-suite/async_hooks/integrations/crypto-prime.tstest-parity/node-suite/async_hooks/integrations/crypto-random-int.tstest-parity/node-suite/async_hooks/integrations/event-emitter-async-resource.tstest-parity/node-suite/async_hooks/integrations/fs-access.tstest-parity/node-suite/async_hooks/integrations/fs-append-file.tstest-parity/node-suite/async_hooks/integrations/fs-copy-file.tstest-parity/node-suite/async_hooks/integrations/fs-directory.tstest-parity/node-suite/async_hooks/integrations/fs-fd-operations.tstest-parity/node-suite/async_hooks/integrations/fs-metadata.tstest-parity/node-suite/async_hooks/integrations/fs-mkdtemp.tstest-parity/node-suite/async_hooks/integrations/fs-rename.tstest-parity/node-suite/async_hooks/integrations/fs-truncate.tstest-parity/node-suite/async_hooks/integrations/fs-watch-file.tstest-parity/node-suite/async_hooks/integrations/fs-watch.tstest-parity/node-suite/async_hooks/integrations/zlib-create-gzip.tstest-parity/node-suite/async_hooks/integrations/zlib-deflate-inflate.tstest-parity/node-suite/async_hooks/propagation/dynamic-import.tstest-parity/node-suite/async_hooks/propagation/nested-nexttick.tstest-parity/node-suite/async_hooks/propagation/timer-ref-unref.tstest-parity/node-suite/async_hooks/propagation/vm-context.tstest-parity/node-suite/async_hooks/providers/child-exec-file.tstest-parity/node-suite/async_hooks/providers/child-exec.tstest-parity/node-suite/async_hooks/providers/child-spawn-events.tstest-parity/node-suite/async_hooks/providers/dgram-events.tstest-parity/node-suite/async_hooks/providers/dgram-send.tstest-parity/node-suite/async_hooks/providers/dns-lookup.tstest-parity/node-suite/async_hooks/providers/dns-resolve4.tstest-parity/node-suite/async_hooks/providers/events-on-async.tstest-parity/node-suite/async_hooks/providers/fixtures/context-worker.cjstest-parity/node-suite/async_hooks/providers/fixtures/tls-credentials.jstest-parity/node-suite/async_hooks/providers/http-agent-isolation.tstest-parity/node-suite/async_hooks/providers/http-client-events.tstest-parity/node-suite/async_hooks/providers/http-request.tstest-parity/node-suite/async_hooks/providers/https-agent-isolation.tstest-parity/node-suite/async_hooks/providers/https-request.tstest-parity/node-suite/async_hooks/providers/net-connect.tstest-parity/node-suite/async_hooks/providers/net-server-events.tstest-parity/node-suite/async_hooks/providers/net-write-callbacks.tstest-parity/node-suite/async_hooks/providers/readline-interface.tstest-parity/node-suite/async_hooks/providers/stream-async-iterator.tstest-parity/node-suite/async_hooks/providers/tls-connect.tstest-parity/node-suite/async_hooks/providers/worker-message-events.tstest-parity/node-suite/async_hooks/resource/bind-receiver-values.tstest-parity/node-suite/async_hooks/resource/scope-receiver-values.tstest-parity/node-suite/async_hooks/static/snapshot-throw-cleanup.tstest-parity/node-suite/async_hooks/storage/disable-one-pending-other.tstest-parity/node-suite/async_hooks/validation/track-promises.tstest-parity/node_suite_baseline.json
🚧 Files skipped from review as they are similar to previous changes (2)
- test-parity/node-suite/async_hooks/hooks/multiple-hooks.ts
- test-parity/node-suite/async_hooks/storage/disable-one-pending-other.ts
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test-parity/node-suite/async_hooks/hooks/fs-readfile-trigger-chain.ts (1)
1-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer cross-platform temporary directory path.
Using a hardcoded
/tmp/path could fail on Windows and is less robust than using Node'sos.tmpdir(). Consider utilizingnode:osandnode:pathto construct a platform-agnostic temporary file path.♻️ Proposed refactor for cross-platform temporary path
import { createHook, executionAsyncId } from "node:async_hooks"; import { readFile, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; -const path = "/tmp/perry-async-hooks-readfile-chain.txt"; +const path = join(tmpdir(), "perry-async-hooks-readfile-chain.txt");🤖 Prompt for 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. In `@test-parity/node-suite/async_hooks/hooks/fs-readfile-trigger-chain.ts` around lines 1 - 4, Update the temporary file path declaration in the async hook test to use the platform-specific directory from node:os and construct the filename with node:path, replacing the hardcoded /tmp path while preserving the existing filename and file operations.
🤖 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.
Nitpick comments:
In `@test-parity/node-suite/async_hooks/hooks/fs-readfile-trigger-chain.ts`:
- Around line 1-4: Update the temporary file path declaration in the async hook
test to use the platform-specific directory from node:os and construct the
filename with node:path, replacing the hardcoded /tmp path while preserving the
existing filename and file operations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1421cb97-6a5e-4308-a7d3-90e1ae22ba67
📒 Files selected for processing (18)
test-parity/node-suite/async_hooks/README.mdtest-parity/node-suite/async_hooks/hooks/fs-readfile-trigger-chain.tstest-parity/node-suite/async_hooks/hooks/interval-repeat-lifecycle.tstest-parity/node-suite/async_hooks/hooks/mutate-during-lifecycle.tstest-parity/node-suite/async_hooks/hooks/nexttick-sibling-trigger.tstest-parity/node-suite/async_hooks/hooks/provider-dns-lookup-lifecycle.tstest-parity/node-suite/async_hooks/hooks/provider-dns-lookup-service-lifecycle.tstest-parity/node-suite/async_hooks/hooks/provider-dns-query-lifecycle.tstest-parity/node-suite/async_hooks/hooks/provider-random-bytes-lifecycle.tstest-parity/node-suite/async_hooks/hooks/provider-zlib-lifecycle.tstest-parity/node-suite/async_hooks/integrations/event-emitter-brand-validation.tstest-parity/node-suite/async_hooks/integrations/util-promisify-custom.tstest-parity/node-suite/async_hooks/providers/dgram-dual-context.tstest-parity/node-suite/async_hooks/providers/dns-lookup-service.tstest-parity/node-suite/async_hooks/providers/net-dual-context.tstest-parity/node-suite/async_hooks/resource/static-bind-types.tstest-parity/node-suite/async_hooks/storage/exit-instance-isolation.tstest-parity/node_suite_baseline.json
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
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-parity/node_suite_baseline.json`:
- Line 5: Update the provenance statement in the baseline note to replace the
stale clean Node 26 run totals and percentage, 2810/2863 (98.1%), with the
current overall-run values recorded on lines 7–10: 2881/3027 (95.2%). Preserve
the surrounding explanations and module-specific provenance unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0a7cdc76-ef5b-4843-8364-7199416fe6fa
📒 Files selected for processing (9)
test-parity/node-suite/async_hooks/README.mdtest-parity/node-suite/async_hooks/hooks/immediate-nested-lifecycle.tstest-parity/node-suite/async_hooks/hooks/provider-crypto-request-lifecycles.tstest-parity/node-suite/async_hooks/hooks/provider-fs-promises-lifecycles.tstest-parity/node-suite/async_hooks/hooks/provider-fs-watcher-lifecycles.tstest-parity/node-suite/async_hooks/hooks/provider-net-lifecycle-matrix.tstest-parity/node-suite/async_hooks/hooks/provider-udp-socket-lifecycle.tstest-parity/node-suite/async_hooks/resource/static-bind-types.tstest-parity/node_suite_baseline.json
🚧 Files skipped from review as they are similar to previous changes (1)
- test-parity/node-suite/async_hooks/resource/static-bind-types.ts
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
…c-hooks-parity # Conflicts: # test-parity/node_suite_baseline.json
Summary
Expands the granular
node:async_hooksparity suite from 5 to 193 fixtures (+188) using Node.js 26.5.0 as the exact oracle, with corresponding Bun and Deno compatibility checks.The suite is organized into small diagnostic cases covering:
AsyncResourceconstruction, identifiers, scope execution, binding, destruction, receiver and argument preservationAsyncLocalStoragenesting,enterWith,exit,disable, re-entry, values, staticbind/snapshot, and propagationThe latest expansion adds 12 fixtures and strengthens 3 existing fixtures:
BLOBREADERandDNSCHANNELresource lifecyclesundefined,null, falsy and object store valuesyield/waitand cancelled async interval propagationVerification
Exact Node oracle:
v26.5.0node --checkAlternate runtimes:
1.2.18: every retained new fixture completed successfully2.9.2: every retained new fixture completed successfullyPerry focused suite, repeated from clean executions:
The second focused run reproduced the same
78/193,114differences and single pre-existing TLS timeout. The granular baseline is updated from76/181to78/193; the overall baseline is2883/3051 (94.5%).Additional checks:
All completed successfully.
Stable diagnostics
The expanded suite exposes stable Perry gaps including:
AsyncResourceprototype/constructor behaviornewIt also records compatible behavior where Perry already matches Node, including the new Web Locks and store-value propagation cases.
Deliberate exclusions
The README documents the stopping judgment and upstream mappings. Remaining cases are excluded when they are redundant, unsupported by the claimed surface, or unreliable as granular parity diagnostics:
MessagePortlifecycle coverage: Bun 1.2.18 did not terminate even after listener removal,unref()and explicit closeSummary by CodeRabbit
Tests
Documentation
Chores