test: reduce flakiness of `test-esm-loader-hooks` by aduh95 · Pull Request #49105 · nodejs/node

aduh95 · 2023-08-11T08:16:28Z

No description provided.

aduh95 · 2023-08-11T08:19:19Z

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

MoLow · 2023-08-11T08:21:58Z

test/es-module/test-esm-loader-hooks.mjs

        console.log('register', result);

        await import('node:os');
+        await setTimeout(99); // delay to limit flakiness


isnt this just hiding a bug?

Not really, race conditions are inherent to cross thread communication I think

Wouldn't a user have to do this too? If so, this feels like a legit bug.

We've known this since we started working on off-threading the loader hooks, it's documented here:

node/doc/api/esm.md

Lines 740 to 743 in 6ad8318

Hooks are run in a separate thread, isolated from the main. That means it is a

different [realm](https://tc39.es/ecma262/#realm). The hooks thread may be

terminated by the main thread at any time, so do not depend on asynchronous

operations (like `console.log`) to complete.

OH! Sorry, this is for a console log. Sorry, yes.

Wouldn't it be better to use something not console log then?

Maybe process._rawDebug or fs.writeFileSync(1?

aduh95 · 2023-08-11T10:47:57Z

The stress test reveals that should have `register` work with cjs is also flaky, working on that

aduh95 · 2023-08-11T11:04:56Z

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

nodejs-github-bot · 2023-08-11T12:05:57Z

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

nodejs-github-bot · 2023-08-12T05:13:23Z

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

nodejs-github-bot · 2023-08-12T06:57:04Z

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

github-actions · 2023-08-12T07:35:24Z

Fast-track has been requested by @aduh95. Please 👍 to approve.

test/es-module/test-esm-loader-hooks.mjs

nodejs-github-bot · 2023-08-12T10:20:13Z

Landed in e68e359

PR-URL: nodejs#49105 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

PR-URL: #49105 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

PR-URL: nodejs#49105 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

PR-URL: #49105 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

PR-URL: nodejs#49105 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

PR-URL: #49105 Backport-PR-URL: #50669 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

PR-URL: nodejs/node#49105 Backport-PR-URL: nodejs/node#50669 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

test: reduce flakiness of test-esm-loader-hooks

0d9baf1

nodejs-github-bot added esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Aug 11, 2023

MoLow reviewed Aug 11, 2023

View reviewed changes

MoLow approved these changes Aug 11, 2023

View reviewed changes

fixup! test: reduce flakiness of test-esm-loader-hooks

5604d92

This comment was marked as outdated.

Sign in to view

JakobJingleheimer approved these changes Aug 11, 2023

View reviewed changes

fixup! fixup! test: reduce flakiness of test-esm-loader-hooks

37bdb8f

aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Aug 11, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 11, 2023

GeoffreyBooth removed the needs-ci PRs that need a full CI run. label Aug 12, 2023

aduh95 added needs-ci PRs that need a full CI run. fast-track PRs that do not need to wait for 48 hours to land. labels Aug 12, 2023

JakobJingleheimer reviewed Aug 12, 2023

View reviewed changes

test/es-module/test-esm-loader-hooks.mjs Show resolved Hide resolved

test/es-module/test-esm-loader-hooks.mjs Show resolved Hide resolved

benjamingr approved these changes Aug 12, 2023

View reviewed changes

aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 12, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 12, 2023

nodejs-github-bot merged commit e68e359 into nodejs:main Aug 12, 2023

aduh95 deleted the test-esm-loader-hooks-flaky branch August 12, 2023 10:34

This was referenced Aug 13, 2023

CI Reliability 2023-08-13 nodejs/reliability#635

Open

CI Reliability 2023-08-14 nodejs/reliability#636

Open

RafaelGSS pushed a commit that referenced this pull request Aug 15, 2023

test: reduce flakiness of test-esm-loader-hooks

b7c602b

PR-URL: #49105 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

UlisesGascon mentioned this pull request Aug 15, 2023

v20.6.0 proposal #49185

Merged

RafaelGSS pushed a commit that referenced this pull request Aug 16, 2023

test: reduce flakiness of test-esm-loader-hooks

8365389

PR-URL: #49105 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

RafaelGSS pushed a commit that referenced this pull request Aug 17, 2023

test: reduce flakiness of test-esm-loader-hooks

2fcb855

PR-URL: #49105 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

aduh95 mentioned this pull request Aug 19, 2023

test: reduce flakiness of test-esm-loader-hooks #49248

Merged

targos mentioned this pull request Nov 11, 2023

[v18.x] Backport most ESM and customization hook changes #50669

Closed

targos added the backported-to-v18.x label Nov 23, 2023

targos mentioned this pull request Nov 28, 2023

v18.19.0 release proposal #50953

Merged

	Hooks are run in a separate thread, isolated from the main. That means it is a
	different [realm](https://tc39.es/ecma262/#realm). The hooks thread may be
	terminated by the main thread at any time, so do not depend on asynchronous
	operations (like `console.log`) to complete.

Uh oh!

Conversation

aduh95 commented Aug 11, 2023

Uh oh!

aduh95 commented Aug 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MoLow Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

aduh95 Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

JakobJingleheimer Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

aduh95 Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

JakobJingleheimer Aug 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GeoffreyBooth Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

aduh95 commented Aug 11, 2023

Uh oh!

This comment was marked as outdated.

aduh95 commented Aug 11, 2023

Uh oh!

nodejs-github-bot commented Aug 11, 2023

Uh oh!

nodejs-github-bot commented Aug 12, 2023

Uh oh!

nodejs-github-bot commented Aug 12, 2023

Uh oh!

github-actions bot commented Aug 12, 2023

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Aug 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

aduh95 commented Aug 11, 2023 •

edited

Loading

JakobJingleheimer Aug 11, 2023 •

edited

Loading