test_runner: support mocking json modules #58007
Open
+57
−9
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JakobJingleheimer
added
the
test_runner
|
Review requested:
|
JakobJingleheimer
force-pushed
the
test_runner/feat/json-module-mock
branch
from
acca0d8 to
8961496
Compare
cjihrig
approved these changes
Apr 24, 2025
Comment on lines
+2199
to
+2202
| This function is used to mock the exports of ECMAScript modules, CommonJS modules, JSON modules, and | ||
| Node.js builtin modules. Any references to the original module prior to mocking are not impacted. In | ||
| order to enable module mocking, Node.js must be started with the | ||
| [`--experimental-test-module-mocks`][] command-line flag. |
There was a problem hiding this comment.
The only change here was to add JSON modules,.
There was a problem hiding this comment.
Let's minimize the diff
Suggested change
| This function is used to mock the exports of ECMAScript modules, CommonJS modules, JSON modules, and | |
| Node.js builtin modules. Any references to the original module prior to mocking are not impacted. In | |
| order to enable module mocking, Node.js must be started with the | |
| [`--experimental-test-module-mocks`][] command-line flag. | |
| This function is used to mock the exports of ECMAScript modules, CommonJS modules, JSON | |
| modules, and Node.js builtin modules. Any references to the original module | |
| prior to mocking are not impacted. In order to enable module mocking, Node.js must | |
| be started with the [`--experimental-test-module-mocks`][] command-line flag. |
Comment on lines
+128
to
+129
| case 'json': | ||
| format = 'module'; |
There was a problem hiding this comment.
This is the main change. Creating a nested ternary seemed messy, so I changed this to a switch.
| 'builtin', | ||
| 'commonjs-typescript', | ||
| 'commonjs', | ||
| 'json', |
There was a problem hiding this comment.
Adding 'json', was the only real change here.
|
Except for the linting issue: LGTM |
pmarchini
approved these changes
Apr 24, 2025
|
Got to it a split-second after you. Could you re-approve since now there has to be at least approval on the most recent commit. |
pmarchini
approved these changes
Apr 24, 2025
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #58007 +/- ##
==========================================
+ Coverage 90.27% 90.28% +0.01%
==========================================
Files 630 630
Lines 186158 186172 +14
Branches 36472 36474 +2
==========================================
+ Hits 168047 168092 +45
+ Misses 10976 10964 -12
+ Partials 7135 7116 -19
🚀 New features to boost your workflow:
|
JakobJingleheimer
added
the
request-ci
github-actions
bot
removed
the
request-ci
marco-ippolito
approved these changes
Apr 25, 2025
aduh95
reviewed
Apr 25, 2025
atlowChemi
approved these changes
Apr 25, 2025
aduh95
reviewed
Apr 25, 2025
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
aduh95
approved these changes
Apr 25, 2025
| - version: | ||
| - REPLACEME | ||
| pr-url: https://github.com/nodejs/node/pull/58007 | ||
| description: Support JSON modules. |
There was a problem hiding this comment.
nit
Suggested change
| description: Support JSON modules. | |
| description: Add support for JSON modules mocking. |
Comment on lines
+2199
to
+2202
| This function is used to mock the exports of ECMAScript modules, CommonJS modules, JSON modules, and | ||
| Node.js builtin modules. Any references to the original module prior to mocking are not impacted. In | ||
| order to enable module mocking, Node.js must be started with the | ||
| [`--experimental-test-module-mocks`][] command-line flag. |
There was a problem hiding this comment.
Let's minimize the diff
Suggested change
| This function is used to mock the exports of ECMAScript modules, CommonJS modules, JSON modules, and | |
| Node.js builtin modules. Any references to the original module prior to mocking are not impacted. In | |
| order to enable module mocking, Node.js must be started with the | |
| [`--experimental-test-module-mocks`][] command-line flag. | |
| This function is used to mock the exports of ECMAScript modules, CommonJS modules, JSON | |
| modules, and Node.js builtin modules. Any references to the original module | |
| prior to mocking are not impacted. In order to enable module mocking, Node.js must | |
| be started with the [`--experimental-test-module-mocks`][] command-line flag. |
There was a problem hiding this comment.
nit: we could use an existing fixtures JSON file (e.g. test/fixtures/experimental.json), or at least move/rename it to test/fixtures/simple.json so it can be used by other tests
JakobJingleheimer
added
the
request-ci
github-actions
bot
removed
the
request-ci
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.