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

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

Merged
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
Copy link
Member

@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
stderr = stderr.toString();
ok(stderr.includes(
'Error [ERR_MODULE_NOT_FOUND]: Cannot find package \'i-dont-exist\' ' +
'imported from'
Copy link
Member

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
Member

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.

Copy link
Member

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

@nodejs-github-bot

This comment has been minimized.

@GeoffreyBooth

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@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?

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.

@nodejs-github-bot

This comment has been minimized.

@GeoffreyBooth GeoffreyBooth removed the needs-ci PRs that need a full CI run. label Jan 1, 2022
Copy link
Member

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

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@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
targos pushed a commit that referenced this pull request Jan 14, 2022
PR-URL: #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 30, 2022
PR-URL: nodejs#41352
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
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
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
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

doc: How to run just the .out tests?
6 participants