esm: refactor esm tests out of test/message by GeoffreyBooth · Pull Request #41352 · nodejs/node · GitHub

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:

Running tools/test.py -J es-module now does, in fact, run all the ESM tests.
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

🎉😭🙇 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.

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

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)

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

🙌 LGTM

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?

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.

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

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

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

Landed in 8dd0658

PR-URL: #41352 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

PR-URL: nodejs#41352 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

PR-URL: #41352 Backport-PR-URL: #41776 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

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 added the needs-ci PRs that need a full CI run. label Dec 30, 2021

JakobJingleheimer reviewed Dec 30, 2021

View reviewed changes

GeoffreyBooth force-pushed the esm-out-tests branch from baa3f88 to 30ee141 Compare December 30, 2021 20:15