FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

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

/ node Public

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

Merged
nodejs-github-bot merged 3 commits into
nodejs:mainfrom
aduh95:test-esm-loader-hooks-flaky
Aug 12, 2023
ConversationCommits (3)ChecksFiles changed
Merged

test: reduce flakiness of test-esm-loader-hooks#49105
nodejs-github-bot merged 3 commits into
nodejs:mainfrom
aduh95:test-esm-loader-hooks-flaky

Conversation

aduh95 commented Aug 11, 2023

Copy link
Copy Markdown
Contributor

No description provided.

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

aduh95 commented Aug 11, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

MoLow reviewed Aug 11, 2023
View reviewed changes
Comment thread test/es-module/test-esm-loader-hooks.mjs
console.log('register', result);

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

MoLow Aug 11, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

isnt this just hiding a bug?

aduh95 Aug 11, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

JakobJingleheimer Aug 11, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

aduh95 Aug 11, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

JakobJingleheimer Aug 11, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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

GeoffreyBooth Aug 11, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Maybe process._rawDebug or fs.writeFileSync(1?

aduh95 commented Aug 11, 2023

Copy link
Copy Markdown
Contributor Author

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

This comment was marked as outdated.

Sign in to view

aduh95 commented Aug 11, 2023

Copy link
Copy Markdown
Contributor Author

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

aduh95 added author ready PRs that have at least one approval, no outstanding review comments, 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

nodejs-github-bot commented Aug 11, 2023

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Aug 12, 2023

Copy link
Copy Markdown
Collaborator

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

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

nodejs-github-bot commented Aug 12, 2023

Copy link
Copy Markdown
Collaborator

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

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

github-actions Bot commented Aug 12, 2023

Copy link
Copy Markdown
Contributor

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

JakobJingleheimer reviewed Aug 12, 2023
View reviewed changes
Comment thread test/es-module/test-esm-loader-hooks.mjs
Comment thread test/es-module/test-esm-loader-hooks.mjs
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

nodejs-github-bot commented Aug 12, 2023

Copy link
Copy Markdown
Collaborator

Landed in e68e359

aduh95 deleted the test-esm-loader-hooks-flaky branch August 12, 2023 10:34
This was referenced Aug 13, 2023
Open
Open
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
test: reduce flakiness of test-esm-loader-hooks
9497110 
PR-URL: nodejs#49105
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
test: reduce flakiness of test-esm-loader-hooks
d279cad 
PR-URL: nodejs#49105
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
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 <moshe@atlow.co.il>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
UlisesGascon mentioned this pull request Aug 15, 2023
Merged
RafaelGSS pushed a commit to RafaelGSS/node that referenced this pull request Aug 15, 2023
test: reduce flakiness of test-esm-loader-hooks
052707f 
PR-URL: nodejs#49105
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
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 <moshe@atlow.co.il>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
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 <moshe@atlow.co.il>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
aduh95 mentioned this pull request Aug 19, 2023
Merged
targos pushed a commit to targos/node that referenced this pull request Nov 11, 2023
test: reduce flakiness of test-esm-loader-hooks
0bfcddc 
PR-URL: nodejs#49105
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
targos mentioned this pull request Nov 11, 2023
Closed
targos pushed a commit that referenced this pull request Nov 23, 2023
test: reduce flakiness of test-esm-loader-hooks
26f0166 
PR-URL: #49105
Backport-PR-URL: #50669
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
targos mentioned this pull request Nov 28, 2023
Merged
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
test: reduce flakiness of test-esm-loader-hooks
254c695 
PR-URL: nodejs/node#49105
Backport-PR-URL: nodejs/node#50669
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
test: reduce flakiness of test-esm-loader-hooks
7b2a6da 
PR-URL: nodejs/node#49105
Backport-PR-URL: nodejs/node#50669
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

GeoffreyBooth GeoffreyBooth left review comments
JakobJingleheimer JakobJingleheimer approved these changes
benjamingr benjamingr approved these changes
MoLow MoLow approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. esm Issues and PRs related to the ECMAScript Modules implementation. fast-track PRs that do not need to wait for 48 hours to land. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL