test(node-integration-tests): Run ESM and CJS scenarios concurrently#22086
test(node-integration-tests): Run ESM and CJS scenarios concurrently#22086mydea wants to merge 4 commits into
Conversation
`createEsmAndCjsTests` (and the ESM/CJS-only variants) registered their runs
with plain `test`, so the ESM run finished before the CJS run started even
though each spawns its own independent child process and mostly sits idle
waiting on it. Default them to `test.concurrent` so ESM+CJS (and multiple test
cases in a suite) overlap — a big win for local/targeted runs where cores are
otherwise idle, bounded in CI by vitest's `maxConcurrency`.
Suites that share mutable external state must stay sequential:
- Suites with a co-located `docker-compose.yml` (shared DB/broker container) are
auto-detected and defaulted to sequential — self-maintaining for future ones.
- Mongo/mysql-style suites that share a `MongoMemoryServer`/mock server started
in `beforeAll` have no compose file, so they pass `{ sequential: true }`.
A new `sequential` option (on `CommonTestOptions`) drives this and always wins
over the auto-detected default.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mydea
requested review from
JPeer264 and
andreiborza
and removed request for
a team
…s sequential Suites that call `expect.assertions()` / `expect.hasAssertions()` use the module-global `expect`, whose assertion counter is shared across concurrently running tests — so under the new `test.concurrent` default, sibling runs inflated the count and tripped "expected N assertions, but got 2N". The per-test context `expect` isn't a reliable workaround here because these suites assert inside I/O callbacks (e.g. mock server handlers) that lose vitest's async context, so those assertions wouldn't be counted at all. Detect this the same way as docker-compose suites: read the suite's `test.ts` and default it to sequential when it uses assertion counting. Self-maintaining — future suites are covered without per-suite opt-outs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…suites as sequential" This reverts commit 1c4d20c.
![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 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f292a5b. Configure here.
| createEsmAndCjsTests(__dirname, 'scenario with space.mjs', 'instrument.mjs', (createRunner, test, mode) => { | ||
| test('reads encoded context lines from filenames with spaces', async () => { | ||
| expect.assertions(1); | ||
|
|
There was a problem hiding this comment.
Global expect with concurrent tests
Medium Severity
This suite registers tests through createEsmAndCjsTests, which now uses test.concurrent by default, but the callback still uses the module-level expect import inside matchers. Vitest requires the test-scoped expect from the test context for concurrent tests; other suites in the same PR were updated to async ({ expect }) => for that reason.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit f292a5b. Configure here.
| createCjsTests(__dirname, 'scenario.mjs', 'instrument.mjs', (createRunner, test) => { | ||
| test('ignores user from request', async () => { | ||
| expect.assertions(2); | ||
|
|
There was a problem hiding this comment.
Global expect with concurrent tests
Medium Severity
createCjsTests now passes test.concurrent into this callback, so the two cases can overlap, yet both still assert with the imported expect in the event callback. That bypasses Vitest’s concurrent test context and matches the same gap fixed elsewhere in this PR with async ({ expect }) =>.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit f292a5b. Configure here.
1 participant
Each
createRunnerscenario runs in its own spawnednodechild process, so within a suite the runner mostly sits idle waiting on that child. ButcreateEsmAndCjsTests(and the ESM/CJS-only variants) registered their runs with plaintest, so the ESM run fully finished before the CJS run even started.This defaults those runs to
test.concurrent, so ESM + CJS (and multiple test cases in a suite) overlap. Big win for local / targeted runs where cores are otherwise idle (a two-mode suite drops from ~sum to ~max of the two runs); in CI it stays bounded by vitest’smaxConcurrency.Suites that share mutable external state must stay sequential, handled via a new
sequentialoption onCommonTestOptions:docker-compose.yml(shared database/broker container) are auto-detected and defaulted to sequential — self-maintaining for future docker suites.MongoMemoryServer/ mock server started inbeforeAllhave no compose file, so they pass{ sequential: true }explicitly (mysql’s mock server is stateless and stays concurrent).An explicit
sequentialalways wins over the auto-detected default.Pairs well with (but is independent of) the event-driven completion and compile-cache PRs.
🤖 Generated with Claude Code