test_runner: sync ESM exports when mocking timers#62141
test_runner: sync ESM exports when mocking timers#62141Siddhartha-singh01 wants to merge 1 commit intonodejs:mainfrom
Conversation
|
Review requested:
|
384259e to
b5ee33c
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #62141 +/- ##
========================================
Coverage 89.65% 89.65%
========================================
Files 676 676
Lines 206342 206546 +204
Branches 39531 39553 +22
========================================
+ Hits 184994 185183 +189
- Misses 13475 13482 +7
- Partials 7873 7881 +8
🚀 New features to boost your workflow:
|
Synchronize ESM facades whenever mocked timers are enabled, disabled, or reset. This ensures that 'node:timers/promises' and other built-ins correctly reflect the mocked state when imported in ESM modules. Fixes: nodejs#62081
b5ee33c to
f629518
Compare
There was a problem hiding this comment.
I'm not so sure that we should be calling syncBuiltinESMExports() on behalf of the user – there isn't a way to constrain its effects, and it only impacts a specific import pattern (ie. an ESM namespace loaded prior to the mock call), so doesn't universally address the caveats for built-in mocking either. Possibly not the end of the world, but I do wonder if better documentation and signposting would be a preferable road to go down.
@nodejs/loaders do you have an opinion on this?
|
Thanks for the feedback @Renegade334! sir That's a fair point regarding the global impact of syncBuiltinESMExports() My goal was to make the mock.timers experience as seamless as possible for ESM users so they don't have to worry about the underlying synchronization logic. I'm definitely interested to hear what @nodejs/loaders thinks about whether this should be automated internally or handled via documentation/signposting. I’m happy to pivot the PR toward documentation if that’s the preferred direction. |
This change ensures that ESM imports of node:timers/promises are correctly mocked when using mock.timers.enable(). This is achieved by calling syncBuiltinESMExports() whenever the mocked timers are toggled or reset.
Fixes: #62081
Before submitting a pull request, please read:
https://github.com/nodejs/node/blob/HEAD/doc/contributing/pull-requests.md#commit-message-guidelines
For code changes:
make -j4 test(UNIX), orvcbuild test(Windows) passes.If you believe this PR should be highlighted in the Node.js CHANGELOG
please add the
notable-changelabel.Developer's Certificate of Origin 1.1
By making a contribution to this project, I certify that:
(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or
(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or
(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.
(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.