test_runner: mock dual-package with conditional exports - #62943
test_runner: mock dual-package with conditional exports#62943maruthang wants to merge 2 commits into
Conversation
Sorry, something went wrong.
When `mock.module()` targets a package whose `exports` field maps `import` and `require` to different files, the ESM resolver and the CJS resolver disagree on the resolved path. Only the ESM path was registered in `mockMap`, so `require()` of the mocked specifier bypassed the mock and loaded the real CJS module. Resolve the specifier through `Module._resolveFilename` from the caller's directory in addition to the existing ESM resolution. When the two paths differ, register the CJS path as a second key in `mockMap` and invalidate `Module._cache[cjsPath]`, restoring it on `restore()`. Single-resolution packages keep their existing behavior. Fixes: nodejs#58231 Signed-off-by: Maruthan G <maruthang4@gmail.com>
|
Review requested:
|
Sorry, something went wrong.
Codecov Report❌ Patch coverage is 82.95455% with 15 lines in your changes missing coverage. Please review.
@@ Coverage Diff @@
## main #62943 +/- ##
==========================================
- Coverage 89.66% 89.64% -0.03%
==========================================
Files 706 706
Lines 219370 219489 +119
Branches 42065 42095 +30
==========================================
+ Hits 196694 196751 +57
- Misses 14586 14638 +52
- Partials 8090 8100 +10
... and 49 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks for this! The test is correct, and I see it's passing.
I'll do a pass on the implementation soon.
I can't remember whether http/s specifiers are still supported (I think that was maybe removed?).
Sorry, something went wrong.
|
It used to be experimental, but i believe it was removed a few majors ago. |
Sorry, something went wrong.
|
Confirming what @ljharb said — --experimental-network-imports (and with it http: / https: ESM specifiers) was dropped in PR #53822 (commit 15a94e67b1c, landed in v22.6.0, July 2024). Network specifiers are no longer resolvable. For this PR specifically, that means the http: / https: branches in resolveAsCJS (lib/internal/test_runner/mock/mock.js:1011-1012) are dead code in practice: the upstream sharedState.moduleLoader.resolveSync(...) call at line 681 errors out for any network specifier well before resolveAsCJS runs at line 708. They were defensive against a case that can't reach the function. Happy to drop those two checks as a follow-up cleanup commit, or keep them as belt-and-braces defense — your call. The other branches (isBuiltin, node:, data:) all still guard live cases, so I'd leave those. |
Sorry, something went wrong.
My call is #62943 (comment)
🙂 |
Sorry, something went wrong.
JakobJingleheimer
left a comment
There was a problem hiding this comment.
🙌 Thanks!
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
When `mock.module()` targets a package whose `exports` field maps `import` and `require` to different files, the ESM resolver and the CJS resolver disagree on the resolved path. Only the ESM path was registered in `mockMap`, so `require()` of the mocked specifier bypassed the mock and loaded the real CJS module. Resolve the specifier through `Module._resolveFilename` from the caller's directory in addition to the existing ESM resolution. When the two paths differ, register the CJS path as a second key in `mockMap` and invalidate `Module._cache[cjsPath]`, restoring it on `restore()`. Single-resolution packages keep their existing behavior. Fixes: #58231 Signed-off-by: Maruthan G <maruthang4@gmail.com> PR-URL: #62943 Reviewed-By: Aviv Keller <me@aviv.sh> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Jacob Smith <jacob@frende.me>
When mock.module() targets a package whose exports field maps
import and require to different files, the ESM resolver and the
CJS resolver disagree on the resolved path. Only the ESM path was
registered in mockMap, so require() of the mocked specifier
bypassed the mock and loaded the real CJS module.
Resolve the specifier through Module._resolveFilename from the
caller's directory in addition to the existing ESM resolution. When
the two paths differ, register the CJS path as a second key in
mockMap and invalidate Module._cache[cjsPath], restoring it on
restore(). Single-resolution packages keep their existing behavior.
Note: I was unable to run the test suite locally (no built out/Release/node available on this Windows host). All four files pass node --check; the resolver-divergence premise was verified empirically against system Node. Looking forward to CI verification.
Fixes: #58231