chore(node-integration-tests): Improve node test runner naming#21685
Conversation
mydea
requested review from
JPeer264 and
andreiborza
and removed request for
a team
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
JPeer264
left a comment
JPeer264
left a comment
There was a problem hiding this comment.
The output looks way better. The tests are failing, but it seems like a docker related issue, it should work after a rerun
mydea
force-pushed
the
fn/node-runner-changes
branch
from
4fe9328 to
e8f0ae8
Compare
| } | ||
|
|
||
| if (prop === 'each' || prop === 'for') { | ||
| return wrapTestEachApi(target[prop], suffix); | ||
| } | ||
|
|
||
| return Reflect.get(target, prop); |
There was a problem hiding this comment.
Bug: Test cleanup logic fragilely depends on Vitest's default afterAll hook execution order. A configuration change could break test cleanup.
Severity: LOW
Suggested Fix
Make the dependency on cleanup order explicit. Instead of relying on implicit framework behavior, the createEsmAndCjsTests function could return its cleanup function, which the calling test suite can then explicitly invoke in the correct order within its own afterAll block. This removes the invisible dependency on Vitest's configuration.
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/createEsmAndCjsTests.ts#L241-L247
Potential issue: The test cleanup logic relies on the implicit default execution order
of `afterAll` hooks in Vitest, which is 'stack' (reverse registration order). This means
the inner cleanup hooks from `createEsmAndCjsTests` run before the outer
`cleanupChildProcesses` hook. While this works correctly under the current
configuration, it is fragile. If the Vitest configuration were changed to
`sequence.hooks: 'list'` or the test runner were switched to a framework with a
different hook execution order, the outer cleanup would run first, prematurely clearing
steps and causing subsequent cleanup logic to fail silently.
There was a problem hiding this comment.
I don't think so, we do not care when this runs because it just cleans up stuff.
2 participants
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.
It also adjust runner cleanup for
createEsmAndCjsTeststo only cleanup processes started by this specific suite, not all of them.While at it, I also split the runner code into multiple files as this has become unwieldy, nothing functionally should change.