test: fix dangling promise in test_runner no isolation test setup

[JakobJingleheimer]

The --imported / --required file contained an un-awaited dynamic import, so it's subsequent code was not guaranteed to finish before the entry-point starts. Strangely that should have caused this test to fail some times, but it seems to be consistently fast enough so that the race condition doesn't occur.

I checked with the original author of the test, and this was not intended (it was only done that way in an attempt to simplify setup and recycle fixtures).

However, after speed improvements in #57419, the race condition was exposed and does occur consistently (causing the global hooks to get registered out of their intended/expected sequence).

[nodejs-github-bot]

Review requested:

• @nodejs/test_runner

[JakobJingleheimer]

The test is passing on the --import case but failing for the --require case (red lines are missing in actual vs expected):

before(): global before one: <root> suite one before two: <root> suite two - beforeEach(): global beforeEach one: suite one - test beforeEach two: suite one - test suite one - test - afterEach(): global afterEach one: suite one - test afterEach two: suite one - test before suite two: suite two - beforeEach(): global beforeEach one: suite two - test beforeEach two: suite two - test suite two - test - afterEach(): global afterEach one: suite two - test afterEach two: suite two - test - after(): global after one: <root> after two: <root>

@avivkeller and I did some digging, and they've summarised our findings over here (but let's keep the discussion of it to this PR) #57419 (comment).

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.25%. Comparing base (a4f556f) to head (aee5b62).
Report is 30 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #57595 +/- ## ======================================= Coverage 90.24% 90.25% ======================================= Files 630 630 Lines 185129 185074 -55 Branches 36238 36220 -18 ======================================= - Hits 167064 167030 -34 + Misses 11045 11024 -21  Partials 7020 7020 

see 34 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[MoLow]

can we add tests for both require and import in cjs?

[JakobJingleheimer]

There is already in this PR, no?

[MoLow]

I might be missing it but where is there a cjs test with import?

[JakobJingleheimer]

OH, sorry I think I misunderstood. You want

node --import global.cjs test.js node --require global.cjs test.js // this exists node --import global.mjs test.js // this exists 

[JakobJingleheimer]

Done (and the import CJS case passes): 6a38d32

[JakobJingleheimer]

@MoLow 👀

[pmarchini]

@JakobJingleheimer if nobody precedes me, I'll take a look ASAP.

[JakobJingleheimer]

@pmarchini looks like nobody is, so if you could, that would be great 😀

[MoLow]

tests seem to need an adjustment, but implementation LGTM

[JakobJingleheimer]

tests seem to need an adjustment, but implementation LGTM

@MoLow hmm? could you advise what about the test needs adjusting? After fixing the dangling promise issue, it looks good to me 🤔 Since it affects only the --require case, I think it's a bug in that branch of the test runner code.

[MoLow]

@JakobJingleheimer I was referring to the failing tests, not necessarily related to this change

[pmarchini]

cc @JakobJingleheimer as per our discussion.

I checked and, unless I completely missed something, the cause is the following:

Because of the --require flag (which is executed before the root test is created by the test runner here:

node/lib/internal/process/pre_execution.js

Line 670 in 3db5491

functionloadPreloadModules(){

), and considering that the module contains test runner hooks, a root test is created via lazy bootstrap.
This executes the before hook but no other hooks.
At this point, the test runner (due to the --test flag) creates a new root test, which, however, does not handle the --require flag.
For this reason, the behaviour is what we’re seeing.

Sure! Here's just the part you asked for:

Here's an example output with some added logs:

preloadModules: [...]/test/fixtures/test-runner/no-isolation/global-hooks.cjs <-- lazyBootstrapRoot <-- before(): global Creating test tree <-- before one: <root> suite one before two: <root> suite two beforeEach one: suite one - test beforeEach two: suite one - test ... 

[JakobJingleheimer]

Awesome! Thank you @pmarchini 🙏

In that case, this is an outstanding bug in the implementation, which I think is outside the scope of this PR and the others that are affected (blocked) by it. Since it's a bug that already exists and was discovered by fixing a test, I think it's acceptable to mark the failing test-case as an expected failure, and fix that subsequently. Thoughts @MoLow@pmarchini?

[pmarchini]

@JakobJingleheimer +1 on my side, I'd suggest adding a todo comment about the root cause

[pmarchini]

LGTM

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/65975/

[nodejs-github-bot]

Landed in 5a2614f