chore(node-integration-tests): Improve node test runner naming#21685
chore(node-integration-tests): Improve node test runner naming#21685mydea wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0de77ce. Configure here.
| cleanupChildProcesses(); | ||
|
|
||
| try { | ||
| await rm(tmpDirPath, { recursive: true, force: true }); | ||
| } catch { | ||
| if (process.env.DEBUG) { | ||
| // eslint-disable-next-line no-console | ||
| console.error(`Failed to remove tmp dir: ${tmpDirPath}`); | ||
| } | ||
| }, 30_000); | ||
| }); | ||
| } | ||
| }, 30_000); |
There was a problem hiding this comment.
Bug: Multiple afterAll hooks registered by createEsmAndCjsTests create a race condition, causing one hook to prematurely clear a global CLEANUP_STEPS set, leading to resource leaks.
Severity: MEDIUM
Suggested Fix
Refactor the cleanup mechanism to be robust against concurrent execution. Instead of a single global set that gets cleared, each afterAll hook should be responsible for cleaning up only the resources it created. This could be achieved by having the start() function return the specific cleanup functions required, which are then managed locally within the scope of createEsmAndCjsTests and its afterAll hook, avoiding the shared global state.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: dev-packages/node-integration-tests/utils/runner.ts#L362-L374
Potential issue: When `createEsmAndCjsTests` is invoked multiple times within the same
`describe` block, each call registers an `afterAll` hook. These hooks all reference a
single global `CLEANUP_STEPS` set. Because Vitest may run `afterAll` hooks in parallel,
a race condition is introduced. The first hook to complete will call
`cleanupChildProcesses`, which clears the entire global set. This can happen before
other tests' cleanup functions have been executed, or even added to the set, leading to
leaked child processes, docker containers, and mock servers during test runs.
Noticed that this is really verbose right now: When using
createEsmAndCjsTestsright now, it produes a rather deeply nested structure, which is technically correct but pretty verbose:This PR adjusts this to instead produce this:
Note that it also suffixes "- failed" in cases where a given env (e.g. esm or cjs) fails, making this more apparent in the test results.