test_runner: add exports option to mock.module#61727
test_runner: add exports option to mock.module#61727Han5991 wants to merge 4 commits intonodejs:mainfrom
Conversation
|
Review requested:
|
nodejs-github-bot
added
needs-ci
Han5991
force-pushed
the
test-runner-mock-exports
branch
from
3d95c0c to
fa99ada
Compare
|
Related: #58443 (I haven't read through this PR yet though) |
I was working on it with that in mind. |
Han5991
force-pushed
the
test-runner-mock-exports
branch
from
73c4cf1 to
715c12c
Compare
|
Yes, sounds good go me! It would be ideal (but not required or anything) to land this with a userland migration ready to go. I think it could be done fairly easily. Cc @Ceres6 this should make your Jest → node:test migration easier 🙂 |
|
Thanks for the feedback. I will scope this PR to Step 2 of the plan: introduce |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #61727 +/- ##
==========================================
+ Coverage 89.73% 89.74% +0.01%
==========================================
Files 675 676 +1
Lines 204525 206112 +1587
Branches 39310 39525 +215
==========================================
+ Hits 183529 184974 +1445
- Misses 13287 13305 +18
- Partials 7709 7833 +124
🚀 New features to boost your workflow:
|
Han5991
force-pushed
the
test-runner-mock-exports
branch
from
715c12c to
424675e
Compare
Add options.exports support in mock.module() and normalize option shapes through a shared exports path. Keep defaultExport and namedExports as aliases, emit runtime deprecation warnings for legacy options, and update docs and tests, including output fixtures and coverage snapshots. Refs: nodejs#58443
Han5991
force-pushed
the
test-runner-mock-exports
branch
from
424675e to
0498de3
Compare
| if ('exports' in options) { | ||
| validateObject(options.exports, 'options.exports'); | ||
| copyOwnProperties(options.exports, moduleExports); | ||
| } |
There was a problem hiding this comment.
Since last-wins, I think the "new" should win (and be moved below namedExports and defaultExport copying).
Or maybe when exports exists, the others should be ignored? I feel like there's some kind of back-version compatibility thing here. @ljharb thoughts?
Rename the named export in the mock.module() example from `fn` to `foo` for improved readability.
- Unify internal export representation into a single `moduleExports` object, removing the split into `defaultExport`, `hasDefaultExport`, and `namedExports`. - DRY up `emitLegacyMockOptionWarning` with a lookup map instead of a switch statement with separate flag variables. - Use `ObjectDefineProperty` for `defaultExport` option to be consistent with descriptor copying in `namedExports`.
JakobJingleheimer
left a comment
JakobJingleheimer
left a comment
There was a problem hiding this comment.
I think this is looking pretty good :)
This commit addresses several feedback items for mock.module():
- Fixes a potential prototype pollution vulnerability by using
ObjectAssign({ __proto__: null }, ...) for property descriptors.
- Migrates legacy option warnings to use the internal
deprecateProperty() utility for better consistency.
- Updates documentation to use "a later version" instead of "a future
major release" for deprecation timelines, as the feature is
experimental.
- Adds comprehensive test cases to ensure getter properties in
mock options are correctly preserved across both module systems.
5 participants
Summary
mock.module(..., { exports })and normalize option shapes through a shared exports pathdefaultExportandnamedExportsas aliases, and emit runtimeDeprecationWarningwhen legacy options are usedLegacy Option Mixing Behavior
options.exportswith legacy options is currently allowed.Scope
defaultExport+namedExportsintoexports#58443 plan.Testing
make lint-jspython3 tools/test.py test/parallel/test-runner-module-mocking.jspython3 tools/test.py test/test-runner/test-output-coverage-with-mock.mjs test/test-runner/test-output-typescript-coverage.mjsRefs: #58443