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

test_runner: support module detection in module mocks #53642

Merged

Conversation

GeoffreyBooth
Copy link
Member

The tests in test-runner-module-mocking.js fail when module detection is enabled; you can see by running the following on current main:

./node --experimental-test-module-mocks --experimental-require-module --experimental-detect-module \
./test/parallel/test-runner-module-mocking.js

(The first two flags are present already at the top of test-runner-module-mocking.js.)

I think that this means that the test runner module mocking feature is already broken for anyone trying to use it along with --experimental-detect-module; and of course this becomes all the more urgent to fix as we hopefully unflag --experimental-detect-module (#53619).

The issue stems from the module mocking using resolution to determine the format to use for what kind of source to generate for the mock, CommonJS or ESM. The format property returned from the resolve hook is optional, so this approach is an unsafe one; it breaks not only when detection is enabled, but also if the user registers a custom resolve hook that doesn’t return a format property.

I’ve updated the logic to instead use the format property returned by the next load hook in the chain, which is usually Node’s internal defaultLoad. The format property is required to be returned from load hooks, so this should be a safe approach. The only potential complication is that this requires the module to be mocked to actually exist on disk, so that nextLoad succeeds; it’s unclear to me whether this was an expected requirement previously. (There were no tests for mocking nonexistent modules.) I think it should be acceptable, because generally users would be mocking existing things? If mocking nonexistent modules is a requirement, we will need to take another approach or add a fallback approach for that condition.

cc @cjihrig @nodejs/test_runner

@GeoffreyBooth GeoffreyBooth added module Issues and PRs related to the module subsystem. loaders Issues and PRs related to ES module loaders test_runner Issues and PRs related to the test runner subsystem. labels Jun 29, 2024
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Jun 29, 2024
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

sure thanks

@GeoffreyBooth GeoffreyBooth added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Jul 4, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 4, 2024
@nodejs-github-bot nodejs-github-bot merged commit 18a1ac9 into nodejs:main Jul 4, 2024
73 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 18a1ac9

@GeoffreyBooth GeoffreyBooth deleted the fix-mocking-with-detection branch July 8, 2024 06:29
aduh95 pushed a commit that referenced this pull request Jul 12, 2024
PR-URL: #53642
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@aduh95 aduh95 mentioned this pull request Jul 12, 2024
aduh95 pushed a commit that referenced this pull request Jul 16, 2024
PR-URL: #53642
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
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. loaders Issues and PRs related to ES module loaders module Issues and PRs related to the module subsystem. needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. test Issues and PRs related to the tests.
6 participants