GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — esm: refactor esm tests out of test/message by GeoffreyBooth · Pull Request #41352 · nodejs/node

Conversation

@GeoffreyBooth

@GeoffreyBooth GeoffreyBooth commented Dec 30, 2021

Copy link
Copy Markdown
Member

Resolves #40920 (comment).

This PR refactors the ESM tests that are currently in test/message to instead be with the rest of the ESM tests in test/es-module. This meant rewriting them from the .out style to instead spawn child processes. This has two benefits:

  1. Running tools/test.py -J es-module now does, in fact, run all the ESM tests.
  2. Refactors to the ESM loader and associated files that change the call stack no longer require updating a bunch of .out files’ stack traces.

In addition, there were some duplicated import assertion tests that I removed; and I slimmed down a few fixtures that we could do without.

cc @nodejs/modules @nodejs/loaders

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji JakobJingleheimer reacted with heart emoji
All reactions
  • 👍 1 reaction
  • ❤️ 1 reaction
@GeoffreyBooth GeoffreyBooth added module Issues and PRs related to the module subsystem. test Issues and PRs related to Node.js core tests and test infrastructure. esm Issues and PRs related to the ECMAScript Modules implementation. loaders Issues and PRs related to ES module loaders. labels Dec 30, 2021
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Dec 30, 2021
JakobJingleheimer
JakobJingleheimer reviewed Dec 30, 2021
View reviewed changes

@JakobJingleheimer JakobJingleheimer left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

🎉😭🙇 Thank you!

Something to consider for all the replacement tests: they're currently checking the full error message (which is already far better than also checking the stacktrace), but I'm thinking that might be overly rigid and brittle. An unimportant change to the error message, say, adding a comma, will cause the test to fail for something we don't care about. I think we only care about specific substrings, like the type of error, file paths if applicable, relevant arguments, etc. The rest is just fluff.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/es-module/test-esm-export-not-found.mjs Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread test/es-module/test-esm-export-not-found.mjs Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread test/es-module/test-esm-loader-not-found.mjs Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread test/es-module/test-esm-loader-not-found.mjs Outdated

@JakobJingleheimer JakobJingleheimer Dec 30, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I think it might be important to include the file path that follows (we want to ensure the error does actually contain it and that it's correct).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@GeoffreyBooth GeoffreyBooth Dec 30, 2021

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

There’s no path:

Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'i-dont-exist' imported from /Users/Geoffrey/Sites/node/
    at new NodeError (node:internal/errors:371:5)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
JakobJingleheimer reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@JakobJingleheimer JakobJingleheimer Dec 30, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Oh weird. (Out of scope) i think there should be

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
GeoffreyBooth reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@GeoffreyBooth
GeoffreyBooth force-pushed the esm-out-tests branch from baa3f88 to 30ee141 Compare December 30, 2021 20:15
@nodejs-github-bot

This comment has been minimized.

Sign in to view
JakobJingleheimer
JakobJingleheimer approved these changes Dec 30, 2021
View reviewed changes

@JakobJingleheimer JakobJingleheimer left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

🙌 LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@github-actions github-actions Bot mentioned this pull request Dec 31, 2021
CI Reliability 2021-12-31 nodejs/reliability#161
Open
26 tasks
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@GeoffreyBooth

This comment has been minimized.

Sign in to view
@GeoffreyBooth
GeoffreyBooth force-pushed the esm-out-tests branch from 30ee141 to 5ce1fd6 Compare December 31, 2021 02:43
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@GeoffreyBooth
GeoffreyBooth force-pushed the esm-out-tests branch from 5ce1fd6 to 8548e52 Compare December 31, 2021 04:07
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@GeoffreyBooth

GeoffreyBooth commented Dec 31, 2021

Copy link
Copy Markdown
Member Author

So I was using path.join to get the absolute file path to the entry point, which in Windows would then be run via something like

node --experimental-loader C:/workspace/node-test-binary-windows-js-suites/node/test/es-module/test-esm-loader-with-syntax-error.mjs

And this was erroring, because apparently even in CLI input the path to the entry point (or loader) needs to be a URL, not a file path. Is this what we want? I know within import statements we want to ensure URLs, but the node command? I guess forcing these to be URLs too allows future other protocols here like https?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
GeoffreyBooth
GeoffreyBooth commented Dec 31, 2021
View reviewed changes
Comment thread test/es-module/test-esm-module-not-found-commonjs-hint.mjs Outdated
Comment on lines 9 to 11

@GeoffreyBooth GeoffreyBooth Dec 31, 2021

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Is this a bug? Shouldn’t the error message say Did you mean to import ./print-error-message.js? (as in, ./ rather than ../)?

The same was present in the previous test, so I think that fixing the bug (if this is one) is outside the scope of this PR, but I just wanted to ask if this is a bug that we should file an issue for.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@github-actions github-actions Bot mentioned this pull request Jan 1, 2022
CI Reliability 2022-01-01 nodejs/reliability#162
Open
24 tasks
@GeoffreyBooth GeoffreyBooth removed the needs-ci PRs that need a full CI run. label Jan 1, 2022
JakobJingleheimer
JakobJingleheimer approved these changes Jan 1, 2022
View reviewed changes

@JakobJingleheimer JakobJingleheimer left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Oow, yes, I like the updated substrings test for the deprecated hooks.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@github-actions github-actions Bot mentioned this pull request Jan 2, 2022
CI Reliability 2022-01-02 nodejs/reliability#163
Open
23 tasks
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

nodejs-github-bot commented Jan 6, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/41772/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Jan 6, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/41773/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@GeoffreyBooth GeoffreyBooth added author ready PRs with CI started, the required approvals, and no outstanding review comments. commit-queue-squash PRs the Commit Queue should land as one squashed commit. labels Jan 6, 2022
targos
targos approved these changes Jan 6, 2022
View reviewed changes
@aduh95 aduh95 added the commit-queue PRs queued for automated landing through the Commit Queue. label Jan 6, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Jan 6, 2022
@nodejs-github-bot
nodejs-github-bot merged commit 8dd0658 into nodejs:master Jan 6, 2022
@nodejs-github-bot

nodejs-github-bot commented Jan 6, 2022

Copy link
Copy Markdown
Collaborator

Landed in 8dd0658
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@GeoffreyBooth
GeoffreyBooth deleted the esm-out-tests branch January 6, 2022 14:14
This was referenced Jan 7, 2022
CI Reliability 2022-01-07 nodejs/reliability#168
Open
CI Reliability 2022-01-08 nodejs/reliability#169
Open
CI Reliability 2022-01-09 nodejs/reliability#170
Open
CI Reliability 2022-01-12 nodejs/reliability#171
Open
CI Reliability 2022-01-13 nodejs/reliability#172
Open
@github-actions github-actions Bot mentioned this pull request Jan 14, 2022
CI Reliability 2022-01-14 nodejs/reliability#173
Open
22 tasks
targos pushed a commit that referenced this pull request Jan 14, 2022
@GeoffreyBooth @targos
esm: refactor esm tests out of test/message
8a94ca7 
PR-URL: #41352
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@targos targos mentioned this pull request Jan 16, 2022
v17.4.0 release proposal #41557
Merged
@aduh95 aduh95 added the backport-open-v16.x label Jan 30, 2022
@aduh95 aduh95 mentioned this pull request Jan 30, 2022
[v16.x Backport] Import assertions related PRs #41776
Closed
aduh95 pushed a commit to aduh95/node that referenced this pull request Jan 31, 2022
@GeoffreyBooth @aduh95
esm: refactor esm tests out of test/message
e79707e 
PR-URL: nodejs#41352
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
@GeoffreyBooth @danielleadams
esm: refactor esm tests out of test/message
4e05026 
PR-URL: #41352
Backport-PR-URL: #41776
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@danielleadams danielleadams added backported-to-v16.x and removed backport-open-v16.x labels Feb 1, 2022
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
v16.14.0 proposal #41804
Merged
@JakobJingleheimer JakobJingleheimer mentioned this pull request Feb 15, 2022
esm: improve fetch_module test coverage and remove hack #41947
Merged
@GeoffreyBooth GeoffreyBooth mentioned this pull request Apr 2, 2023
test_runner: hide failing tests title when all tests pass #47370
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JakobJingleheimer JakobJingleheimer JakobJingleheimer approved these changes
@aduh95 aduh95 aduh95 approved these changes
+1 more reviewer
@targos targos targos approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. commit-queue-squash PRs the Commit Queue should land as one squashed commit. esm Issues and PRs related to the ECMAScript Modules implementation. loaders Issues and PRs related to ES module loaders. module Issues and PRs related to the module subsystem. test Issues and PRs related to Node.js core tests and test infrastructure.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

doc: How to run just the .out tests?

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@GeoffreyBooth @nodejs-github-bot @targos @JakobJingleheimer @aduh95 @danielleadams
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.