Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

esm: refactor esm tests out of test/message #41352

Merged
merged 17 commits into from Jan 6, 2022
Merged

esm: refactor esm tests out of test/message #41352

merged 17 commits into from Jan 6, 2022

Conversation

GeoffreyBooth
Copy link
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

@GeoffreyBooth GeoffreyBooth added module Issues and PRs related to the module subsystem. test Issues and PRs related to the tests. 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
Copy link
Contributor

@JakobJingleheimer JakobJingleheimer left a 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.

test/es-module/test-esm-export-not-found.mjs Outdated Show resolved Hide resolved
test/es-module/test-esm-export-not-found.mjs Outdated Show resolved Hide resolved
test/es-module/test-esm-loader-not-found.mjs Show resolved Hide resolved
test/es-module/test-esm-loader-not-found.mjs Outdated
stderr = stderr.toString();
ok(stderr.includes(
'Error [ERR_MODULE_NOT_FOUND]: Cannot find package \'i-dont-exist\' ' +
'imported from'
Copy link
Contributor

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

Copy link
Member Author

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)

Copy link
Contributor

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

@nodejs-github-bot

This comment has been minimized.

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

@JakobJingleheimer JakobJingleheimer left a comment

Choose a reason for hiding this comment

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

🙌 LGTM

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@github-actions github-actions bot mentioned this pull request Dec 31, 2021
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
@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
@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
Copy link
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?

GeoffreyBooth
GeoffreyBooth commented Dec 31, 2021
View reviewed changes
test/es-module/test-esm-module-not-found-commonjs-hint.mjs Outdated
Comment on lines 9 to 11
input: 'import "./print-error-message"',
// Did you mean to import ../print-error-message.js?
expected: ' ../print-error-message.js?'
Copy link
Member Author

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.

@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
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
Copy link
Contributor

@JakobJingleheimer JakobJingleheimer left a 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.

@github-actions github-actions bot mentioned this pull request Jan 2, 2022
Open
23 tasks
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@GeoffreyBooth GeoffreyBooth added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Jan 6, 2022
@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 6, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 6, 2022
@nodejs-github-bot nodejs-github-bot merged commit 8dd0658 into nodejs:master Jan 6, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 8dd0658

@GeoffreyBooth GeoffreyBooth deleted the esm-out-tests branch January 6, 2022 14:14
This was referenced Jan 7, 2022
Open
Open
Open
Open
Open
@github-actions github-actions bot mentioned this pull request Jan 14, 2022
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 <duhamelantoine1995@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@targos targos mentioned this pull request Jan 16, 2022
Merged
aduh95 pushed a commit to aduh95/node that referenced this pull request Jan 30, 2022
@GeoffreyBooth @aduh95
esm: refactor esm tests out of test/message
b5efa9b 
PR-URL: nodejs#41352
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@aduh95 aduh95 mentioned this pull request Jan 30, 2022
Closed
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
@GeoffreyBooth @Linkgoron
esm: refactor esm tests out of test/message
8e89a4b 
PR-URL: nodejs#41352
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
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 <duhamelantoine1995@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
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 <duhamelantoine1995@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
Merged
@JakobJingleheimer JakobJingleheimer mentioned this pull request Feb 15, 2022
Merged
@GeoffreyBooth GeoffreyBooth mentioned this pull request Apr 2, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@targos targos targos approved these changes

@JakobJingleheimer JakobJingleheimer JakobJingleheimer approved these changes

@aduh95 aduh95 aduh95 approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. 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 the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

doc: How to run just the .out tests?
6 participants
@GeoffreyBooth @nodejs-github-bot @targos @JakobJingleheimer @aduh95 @danielleadams