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

function loadPreloadModules() {

), 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