test: fix test-repl-import-referrer flakiness by dario-piotrowicz · Pull Request #58014 · nodejs/node

dario-piotrowicz · 2025-04-24T22:59:52Z

codecov · 2025-04-24T23:48:25Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.26%. Comparing base (b665127) to head (01e3f9d).
Report is 4 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #58014 +/- ##
==========================================
- Coverage 90.27% 90.26% -0.01% 
==========================================
Files 630 630 Lines 186158 186158 Branches 36475 36474 -1 ==========================================
- Hits 168047 168031 -16 - Misses 10980 10988 +8 - Partials 7131 7139 +8

see 22 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.

aduh95 · 2025-04-24T23:51:28Z

Why 250? Did you try other values? Could we use a lower one? We should probably be using common.platformTimeout() btw

Why 250? Did you try other values? Could we use a lower one?
250 is totally arbitrary 😓 , I did try different values, 100 seems to be working fine on my
machine but I increased the value as I think a bigger delay would probably be needed in CI
would you prefer me to put 100 instead and see how this performs in CI?
We should probably be using common.platformTimeout() btw
ah yeah for sure, sorry I forgot about this 😓

lpinca · 2025-04-25T06:30:58Z

please let me know if you think there are better alternatives 🙏

The following patch seems to fix the issue on my machine.

diff --git a/test/parallel/test-repl-import-referrer.js b/test/parallel/test-repl-import-referrer.js
index 9c3e961e5e1..57aae7858ab 100644
--- a/test/parallel/test-repl-import-referrer.js+++ b/test/parallel/test-repl-import-referrer.js@@ -8,23 +8,24 @@ const args = ['--interactive'];
const opts = { cwd: fixtures.path('es-modules') };
const child = cp.spawn(process.execPath, args, opts);
-const outputs = [];+let output = '';
child.stdout.setEncoding('utf8');
child.stdout.on('data', (data) => {
- outputs.push(data);- if (outputs.length === 3) {+ output += data;++ if (+ !child.stdin.writableEnded &&+ output.includes('[Module: null prototype] { message: \'A message\' }')+ ) {
// All the expected outputs have been received
// so we can close the child process's stdin
child.stdin.end();
}
});
-child.on('exit', common.mustCall(() => {- const results = outputs[2].split('\n')[0];- assert.strictEqual(- results,- '[Module: null prototype] { message: \'A message\' }'- );+child.on('exit', common.mustCall((code, signal) => {+ assert.strictEqual(code, 0);+ assert.strictEqual(signal, null);
}));
child.stdin.write('await import(\'./message.mjs\');\n');

dario-piotrowicz · 2025-04-25T08:04:45Z

please let me know if you think there are better alternatives 🙏
The following patch seems to fix the issue on my machine.
...

Thanks a lot @lpinca 🙂

Checking for the output string in the on data listener is something that occurred to me too, but I don't attempt to go that route as I think it doesn't leads to nice patterns here 😕 😓

In your suggested solution specifically I have concerns about the readability of the test, using '[Module: null prototype] { message: \'A message\' }' as signal for when to exit seems to me much much less clear than asserting that the value is in the output (basically we're asserting that value in an indirect way, we could re-assert it in the on exit listener but that feels pretty unnecessary/ugly 😓)

Anyways if you strongly believe that your suggested solution is better (I'd also be curious to see other opinions on it hopefully) I am totally fine to go with it, as using setTimeout is indeed pretty nasty in its own way 😓

lpinca · 2025-04-25T08:10:51Z

Checking for the output string in the on data listener is something that occurred to me too, but I don't attempt to go that route as I think it doesn't leads to nice patterns here 😕 😓

In your suggested solution specifically I have concerns about the readability of the test, using '[Module: null prototype] { message: \'A message\' }' as signal for when to exit seems to me much much less clear than asserting that the value is in the output

What is the difference? Currently it is based on counting data chunks which is not reliable. I definitely think it is way better than adding a random delay. Timers are known to cause flakiness.

dario-piotrowicz · 2025-04-25T08:17:44Z

What is the difference?

The main difference I see is that we're not directly asserting that the string is in the output which is the whole point of the test 😖

Currently it is based on counting data chunks which is not reliable.

Is it not? how come? isn't that deterministic? 😮

I definitely think it is way better than adding a random delay.

yeah I think that this is a very fair point of view 👍

lpinca · 2025-04-25T08:18:58Z

The main difference I see is that we're not directly asserting that the string is in the output which is the whole point of the test 😖

We do, if there isn't the test times out.

dario-piotrowicz · 2025-04-25T08:25:36Z

We do, if there isn't the test times out.

yes, of course we do, I was saying that it's not being done in a direct way (there is not assert.strictEqual() being used for that)

this in my opinion reduces readability and it might make the issue more difficult to diagnose since the failure would be a timeout instead of an assert failure (although someone could argue that given the current flakes that's almost already the case 😓)

lpinca · 2025-04-25T08:31:24Z

yes, of course we do, I was saying that it's not being done in a direct way (there is not assert.strictEqual() being used for that)

There are hundreds of tests like this.

Is it not? how come? isn't that deterministic? 😮

Here is an example when test currently fails found by applying this patch

diff --git a/test/parallel/test-repl-import-referrer.js b/test/parallel/test-repl-import-referrer.js
index 9c3e961e5e1..41204efda17 100644
--- a/test/parallel/test-repl-import-referrer.js+++ b/test/parallel/test-repl-import-referrer.js@@ -12,6 +12,7 @@ const outputs = [];
child.stdout.setEncoding('utf8');
child.stdout.on('data', (data) => {
outputs.push(data);
+ console.log(outputs)
if (outputs.length === 3) {
// All the expected outputs have been received
// so we can close the child process's stdin

=== release test-repl-import-referrer === Path: parallel/test-repl-import-referrer
[
'Welcome to Node.js v24.0.0-pre.\nType ".help" for more information.\n> '
]
[
'Welcome to Node.js v24.0.0-pre.\nType ".help" for more information.\n> ',
"[Module: null prototype] { message: 'A message' }\n> "
]
Command: out/Release/node --test-reporter=./test/common/test-error-reporter.js --test-reporter-destination=stdout /Users/luigi/code/node/test/parallel/test-repl-import-referrer.js
--- TIMEOUT ---

As you can see there are only 2 chunks, the test waits for 3 before calling child.stdin.end();. This is issue.

Co-authored-by: Luigi Pinca <luigipinca@gmail.com>

dario-piotrowicz · 2025-04-25T08:34:12Z

There are hundreds of tests like this.

👍

As you can see there are only 2 chunks, the test waits for 3 before calling child.stdin.end();. This is issue.

ah ok, sorry I thought that the chunking would be pretty stable/deterministic (I guess it is on the same machine?), thanks for pointing this 🙏

dario-piotrowicz · 2025-04-25T08:35:04Z

Thanks a lot for the suggested change @lpinca 🙏, I've applied it 🙂

nodejs-github-bot · 2025-04-25T08:47:25Z

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

aduh95 · 2025-04-25T08:51:18Z

Stress test CI: https://ci.nodejs.org/job/node-stress-single-test/564/

Co-authored-by: Luigi Pinca <luigipinca@gmail.com>

nodejs-github-bot · 2025-04-25T10:42:18Z

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

lpinca · 2025-04-25T14:48:40Z

I was thinking of a simpler fix with fewer changes than the original test

diff --git a/test/parallel/test-repl-import-referrer.js b/test/parallel/test-repl-import-referrer.js
index 9c3e961e5e1..c1b82198a25 100644
--- a/test/parallel/test-repl-import-referrer.js+++ b/test/parallel/test-repl-import-referrer.js@@ -8,24 +8,19 @@ const args = ['--interactive'];
const opts = { cwd: fixtures.path('es-modules') };
const child = cp.spawn(process.execPath, args, opts);
-const outputs = [];+let output = '';
child.stdout.setEncoding('utf8');
child.stdout.on('data', (data) => {
- outputs.push(data);- if (outputs.length === 3) {- // All the expected outputs have been received- // so we can close the child process's stdin- child.stdin.end();- }+ output += data;
});
child.on('exit', common.mustCall(() => {
- const results = outputs[2].split('\n')[0];- assert.strictEqual(- results,- '[Module: null prototype] { message: \'A message\' }'- );+ const results = output.replace(/^> /mg, '').split('\n').slice(2);+ assert.deepStrictEqual(results, [+ '[Module: null prototype] { message: \'A message\' }',+ ''+ ]);
}));
child.stdin.write('await import(\'./message.mjs\');\n');
-child.stdin.write('.exit');+child.stdin.write('.exit\n');

but it fails as an ERR_USE_AFTER_CLOSE error is thrown. Is this expected or it is an unwanted effect of 8e7f32f? I think no error should be thrown.

dario-piotrowicz · 2025-04-25T15:26:02Z

I was thinking of a simpler fix with fewer changes than the original test
...
but it fails as an ERR_USE_AFTER_CLOSE error is thrown. Is this expected or it is an unwanted effect of 8e7f32f? I think no error should be thrown.

No, for sure this is not expected... and I would imagine it to indeed most likely be an unwanted effect of 8e7f32f, the fact that you see the error without manually closing the child's stdin is pretty concerning to me 😕

I'll have a look as soon as I can (later today/tomorrow), I wonder if this is so severe that 8e7f32f should be reverted 😖

Co-authored-by: Luigi Pinca <luigipinca@gmail.com>

lpinca · 2025-04-25T15:49:44Z

I wonder if this is so severe that 8e7f32f should be reverted 😖

I was thinking the same thing.

Co-authored-by: Luigi Pinca <luigipinca@gmail.com>

dario-piotrowicz · 2025-04-25T15:58:01Z

I wonder if this is so severe that 8e7f32f should be reverted 😖
I was thinking the same thing.

I'm happy to investigate this, but if we're concerned about user's impact it could be safer to revert sooner rather than later...

since #57680 was marked as semver-major can I assume that only users using node 25 (when it will come out) would potentially be affected?

lpinca · 2025-04-25T16:05:18Z

Yes, there are also nightly builds, but I don't expect people to use them in production 😄.

FWIW, I can also reproduce it manually

$ ./node
Welcome to Node.js v24.0.0-pre.
Type ".help" for more information.
> await import('./test/fixtures/es-modules/message.mjs');
[Module: null prototype] { message: 'A message' }
> .exit
node:internal/readline/interface:603
throw new ERR_USE_AFTER_CLOSE('readline');
^
Error [ERR_USE_AFTER_CLOSE]: readline was closed
at REPLServer.pause (node:internal/readline/interface:603:13)
at REPLServer.<anonymous> (node:internal/main/repl:48:16)
at REPLServer.emit (node:events:519:35)
at REPLServer.emit (node:domain:489:12)
at REPLServer.emitExit (node:repl:841:10)
at REPLServer.emit (node:events:519:35)
at REPLServer.emit (node:domain:489:12)
at REPLServer.close (node:internal/readline/interface:594:10)
at node:repl:1115:5
at process.processTicksAndRejections (node:internal/process/task_queues:85:11) {
code: 'ERR_USE_AFTER_CLOSE'
}
Node.js v24.0.0-pre

dario-piotrowicz · 2025-04-25T16:20:53Z

@lpinca yes I can as well! 😓

ok this is pretty bad... (I'm surprised that no tests consistently failed because of this 😓)

maybe I should close this PR and revert 8e7f32f right away to be on the safe side, then try to reintroduce the change but also making sure that there's no repl error being thrown... what do you think? 😓

(PS: I'm really sorry for the inconvenience caused here, I'm really surprised that something like this managed to sneak through 😓)

lpinca · 2025-04-25T16:28:20Z

I think it makes sense.

(PS: I'm really sorry for the inconvenience caused here, I'm really surprised that something like this managed to sneak through 😓)

No problem, it happens.

dario-piotrowicz · 2025-04-25T16:42:22Z

Closing in favour of #58024

nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Apr 24, 2025

aduh95 reviewed Apr 24, 2025
View reviewed changes

dario-piotrowicz changed the title ~~test: add setTimeout to remove test-repl-import-referrer flakiness~~test: fix test-repl-import-referrer flakinessApr 25, 2025

dario-piotrowiczand others added 2 commits April 25, 2025 10:31

test: fix test-repl-import-referrer flakiness
a35b1a6

apply proposed solution that doesn't rely on setTiemout
7f7cea4
Co-authored-by: Luigi Pinca <luigipinca@gmail.com>

dario-piotrowicz force-pushed the dario/58009/test-repl-import-referrer-flaky branch from c8c1e6c to 7f7cea4Compare April 25, 2025 08:31

lpinca approved these changes Apr 25, 2025
View reviewed changes

Comment threadtest/parallel/test-repl-import-referrer.js Outdated

lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 25, 2025

aduh95 approved these changes Apr 25, 2025
View reviewed changes

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 25, 2025

Update test/parallel/test-repl-import-referrer.js
a1e4a2b
Co-authored-by: Luigi Pinca <luigipinca@gmail.com>

lpinca approved these changes Apr 25, 2025
View reviewed changes

lpinca reviewed Apr 25, 2025
View reviewed changes

Comment threadtest/parallel/test-repl-import-referrer.js Outdated

lpinca approved these changes Apr 25, 2025
View reviewed changes

lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 25, 2025

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 25, 2025

aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 25, 2025

lpinca added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Apr 25, 2025

aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Apr 25, 2025

Update test/parallel/test-repl-import-referrer.js
88f9b43
Co-authored-by: Luigi Pinca <luigipinca@gmail.com>

lpinca removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 25, 2025

lpinca reviewed Apr 25, 2025
View reviewed changes

Comment threadtest/parallel/test-repl-import-referrer.js

Update test/parallel/test-repl-import-referrer.js
01e3f9d
Co-authored-by: Luigi Pinca <luigipinca@gmail.com>

dario-piotrowicz mentioned this pull request Apr 25, 2025
Revert "readline: add stricter validation for functions called after close #58024
Merged

dario-piotrowicz closed this Apr 25, 2025

dario-piotrowicz deleted the dario/58009/test-repl-import-referrer-flaky branch April 25, 2025 21:59

dario-piotrowicz mentioned this pull request May 11, 2025
readline: add stricter validation for functions called after closed (second attempt) #58283
Merged

Uh oh!

Uh oh!

Conversation

dario-piotrowicz commented Apr 24, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecovBot commented Apr 24, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

aduh95Apr 24, 2025

Choose a reason for hiding this comment

Uh oh!

dario-piotrowiczApr 25, 2025

Choose a reason for hiding this comment

Uh oh!

lpinca commented Apr 25, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dario-piotrowicz commented Apr 25, 2025

Uh oh!

lpinca commented Apr 25, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dario-piotrowicz commented Apr 25, 2025

Uh oh!

lpinca commented Apr 25, 2025

Uh oh!

dario-piotrowicz commented Apr 25, 2025

Uh oh!

lpinca commented Apr 25, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dario-piotrowicz commented Apr 25, 2025

Uh oh!

dario-piotrowicz commented Apr 25, 2025

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 25, 2025

Uh oh!

aduh95 commented Apr 25, 2025

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 25, 2025

Uh oh!

lpinca commented Apr 25, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dario-piotrowicz commented Apr 25, 2025

Uh oh!

lpinca commented Apr 25, 2025

Uh oh!

Uh oh!

dario-piotrowicz commented Apr 25, 2025

Uh oh!

lpinca commented Apr 25, 2025

Uh oh!

dario-piotrowicz commented Apr 25, 2025

Uh oh!

lpinca commented Apr 25, 2025

Uh oh!

dario-piotrowicz commented Apr 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dario-piotrowicz commented Apr 24, 2025•
edited
Loading

codecovBot commented Apr 24, 2025•
edited
Loading

lpinca commented Apr 25, 2025•
edited
Loading

lpinca commented Apr 25, 2025•
edited
Loading

lpinca commented Apr 25, 2025•
edited
Loading

lpinca commented Apr 25, 2025•
edited
Loading