Skip to content

test_runner: add exports option to mock.module#61727

Open
Han5991 wants to merge 4 commits intonodejs:mainfrom
Han5991:test-runner-mock-exports
Open

test_runner: add exports option to mock.module#61727
Han5991 wants to merge 4 commits intonodejs:mainfrom
Han5991:test-runner-mock-exports

Conversation

@Han5991
Copy link
Contributor

@Han5991 Han5991 commented Feb 8, 2026 •
edited
Loading

Summary

  • add mock.module(..., { exports }) and normalize option shapes through a shared exports path
  • keep defaultExport and namedExports as aliases, and emit runtime DeprecationWarning when legacy options are used
  • update docs, tests, and output fixtures/snapshots to reflect deprecation signaling and the new preferred option shape

Legacy Option Mixing Behavior

  • Mixed usage of options.exports with legacy options is currently allowed.
  • Values are normalized/merged into the same internal exports structure.
  • Legacy options are deprecated, but still supported for compatibility during migration.

Scope

Testing

  • make lint-js
  • python3 tools/test.py test/parallel/test-runner-module-mocking.js
  • python3 tools/test.py test/test-runner/test-output-coverage-with-mock.mjs test/test-runner/test-output-typescript-coverage.mjs

Refs: #58443

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 8, 2026

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Feb 8, 2026
@Han5991 Han5991 marked this pull request as draft February 8, 2026 00:27
@Han5991 Han5991 force-pushed the test-runner-mock-exports branch from 3d95c0c to fa99ada Compare February 8, 2026 00:35
@JakobJingleheimer
Copy link
Member

JakobJingleheimer commented Feb 8, 2026 •
edited
Loading

Related: #58443

(I haven't read through this PR yet though)

@Han5991
Copy link
Contributor Author

Han5991 commented Feb 8, 2026

Related: #58443

(I haven't read through this PR yet though)

@JakobJingleheimer

I was working on it with that in mind.
I'm planning to proceed only up to Step 2 of the plan. Is that okay?

@Han5991 Han5991 force-pushed the test-runner-mock-exports branch from 73c4cf1 to 715c12c Compare February 8, 2026 01:11
@JakobJingleheimer
Copy link
Member

JakobJingleheimer commented Feb 8, 2026 •
edited
Loading

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 🙂

@Han5991
Copy link
Contributor Author

Han5991 commented Feb 8, 2026

Thanks for the feedback. I will scope this PR to Step 2 of the plan: introduce options.exports, keep defaultExport/namedExports as aliases (with deprecation signaling), and defer userland migration + removal to follow-up work.

@Han5991 Han5991 marked this pull request as ready for review February 8, 2026 01:33
@codecov
Copy link

codecov bot commented Feb 8, 2026 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.74%. Comparing base (0777dcc) to head (60f2d68).
⚠️ Report is 241 commits behind head on main.

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
Files with missing lines Coverage Δ
lib/internal/test_runner/mock/loader.js 99.35% <100.00%> (ø)
lib/internal/test_runner/mock/mock.js 98.78% <100.00%> (+0.05%) ⬆️

... and 181 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Han5991 Han5991 force-pushed the test-runner-mock-exports branch from 715c12c to 424675e Compare February 8, 2026 02:47
@Han5991
test_runner: add exports option for module mocks
0498de3 
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 Han5991 force-pushed the test-runner-mock-exports branch from 424675e to 0498de3 Compare February 8, 2026 07:24
JakobJingleheimer
JakobJingleheimer reviewed Feb 25, 2026
View reviewed changes
Copy link
Member

@JakobJingleheimer JakobJingleheimer left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for picking this up!

lib/internal/test_runner/mock/mock.js
Comment on lines +826 to +829
if ('exports' in options) {
validateObject(options.exports, 'options.exports');
copyOwnProperties(options.exports, moduleExports);
}
Copy link
Member

@JakobJingleheimer JakobJingleheimer Feb 25, 2026

Choose a reason for hiding this comment

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

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?

@Han5991
doc: improve mock.module example clarity
f956d87 
Rename the named export in the mock.module() example from `fn`
to `foo` for improved readability.
@Han5991
test_runner: simplify mock.module normalization
13b13b8 
- 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
JakobJingleheimer reviewed Feb 26, 2026
View reviewed changes
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.

I think this is looking pretty good :)

avivkeller
avivkeller reviewed Feb 26, 2026
View reviewed changes
avivkeller
avivkeller reviewed Feb 26, 2026
View reviewed changes
@Han5991
test_runner: improve mock.module options handling
60f2d68 
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.
@Han5991
Copy link
Contributor Author

Han5991 commented Mar 2, 2026

@JakobJingleheimer,

I'd like to help with the userland-migration tool for mock.module.

If you approve, I can implement the migration script in short order to help users move to the new exports shape. Should I contribute it to https://github.com/nodejs/userland-migrations?

@JakobJingleheimer
Copy link
Member

JakobJingleheimer commented Mar 3, 2026

I'd like to help with the userland-migration tool for mock.module.

If you approve, I can implement the migration script in short order to help users move to the new exports shape. Should I contribute it to https://github.com/nodejs/userland-migrations?

Heck yes! And yes, that is where to go. @brunocroh may have started. Could you sync with him? I think this would be a great one to get started with 🙂

@brunocroh
Copy link
Member

brunocroh commented Mar 3, 2026 •
edited
Loading

I'd like to help with the userland-migration tool for mock.module.

If you approve, I can implement the migration script in short order to help users move to the new exports shape. Should I contribute it to https://github.com/nodejs/userland-migrations?

Heck yes! And yes, that is where to go. @brunocroh may have started. Could you sync with him? I think this would be a great one to get started with 🙂

Its almost done, https://github.com/nodejs/userland-migrations/pull/390/changes, I just need add more edge cases and docs for it

JakobJingleheimer
JakobJingleheimer reviewed Mar 11, 2026
View reviewed changes
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 aside from the item I'd like some others' opinions on. TC39 and some other conferences are this week, so I'll poke people next week after things are calmer.

@JakobJingleheimer JakobJingleheimer moved this to In Progress in Test Runner Board Mar 11, 2026
@brunocroh brunocroh mentioned this pull request Mar 14, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JakobJingleheimer JakobJingleheimer JakobJingleheimer left review comments

@jasnell jasnell jasnell approved these changes

@avivkeller avivkeller Awaiting requested review from avivkeller

@pmarchini pmarchini Awaiting requested review from pmarchini

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem.

Projects

Test Runner Board
Status: In Progress

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Han5991 @nodejs-github-bot @JakobJingleheimer @brunocroh @jasnell @avivkeller