-
-
Notifications
You must be signed in to change notification settings - Fork 33.8k
test_runner: add mock-timers support for AbortSignal.timeout #60751
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
test_runner: add mock-timers support for AbortSignal.timeout #60751
Conversation
|
Review requested:
|
|
Hi @Renegade334 @ErickWendel 👋 I’ve opened a fresh PR with a clean branch so it’s easier to review. Please let me know if anything needs adjusting, happy to update it! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #60751 +/- ##
==========================================
- Coverage 88.54% 88.52% -0.02%
==========================================
Files 703 703
Lines 208252 208306 +54
Branches 40153 40160 +7
==========================================
+ Hits 184391 184413 +22
- Misses 15869 15893 +24
- Partials 7992 8000 +8
🚀 New features to boost your workflow:
|
8cbc185 to
1b2d32f
Compare
|
I've updated both the implementation and the regression test: Fixed the missing function name & trailing comma issues in mock_timers.js Cleaned up unused variables Ensured the mock AbortSignal.timeout() matches Node.js behavior Updated the test file to avoid restricted syntax and comply with Node’s linting rules The PR is ready for the next round of review |
Renegade334
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good! Just a couple of recommended tweaks.
| if (NumberIsNaN(delay)) { | ||
| throw new ERR_INVALID_ARG_VALUE('delay', delay, 'delay must be a number'); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if (NumberIsNaN(delay)) { | |
| throw new ERR_INVALID_ARG_VALUE('delay', delay, 'delay must be a number'); | |
| } | |
| validateUint32(delay, 'delay', false); |
This should use validateUint32() for consistency with AbortSignal.timeout(). (You'll need to add it to the list of imports from internal/validators.)
| /** | ||
| * @enum {('setTimeout'|'setInterval'|'setImmediate'|'Date'|'scheduler.wait')[]} Supported timers | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This docblock should be restored, with the addition of 'AbortSignal.timeout' to the list.
| * EnableOptions type: | ||
| * | ||
| * @typedef {object} EnableOptions | ||
| * @property {Array<string>} apis List of timers to enable, defaults to all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * EnableOptions type: | |
| * | |
| * @typedef {object} EnableOptions | |
| * @property {Array<string>} apis List of timers to enable, defaults to all | |
| * @typedef {object} EnableOptions | |
| * @property {SUPPORTED_APIS} apis List of timers to enable, defaults to all |
| if (this.#realAbortSignalTimeout) { | ||
| ObjectDefineProperty(AbortSignal, 'timeout', this.#realAbortSignalTimeout); | ||
| } else { | ||
| delete AbortSignal.timeout; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if (this.#realAbortSignalTimeout) { | |
| ObjectDefineProperty(AbortSignal, 'timeout', this.#realAbortSignalTimeout); | |
| } else { | |
| delete AbortSignal.timeout; | |
| } | |
| ObjectDefineProperty(AbortSignal, 'timeout', this.#realAbortSignalTimeout); |
AbortSignal.timeout() exists in all supported versions of Node.js, so this conditional is unnecessary; the internal property will always be defined if this code is reached.
StefanStojanovic
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Make sure you fix issues found by the linter.
1b2d32f to
ef06137
Compare
|
All requested updates have now been applied. Summary of changes:Updated mock_timers.js according to the reviewer’s feedback Preserved the required comment and applied all suggested code adjustments Updated the test file to follow the Node.js test style (including loading common first) Verified everything locally, all lint issues are resolved @StefanStojanovic @Renegade334 |
|
@Renegade334 |
Fixes: #60509
This PR completes the fix for making
AbortSignal.timeout()compatible withthe test runner's mock timers, and adds a regression test to ensure correct
behavior going forward.
What’s included
1. Mock timers fix
"AbortSignal.timeout"as a discrete mock-timers API target.AbortSignal.timeoutproperty (no silent failures).toFake/toRealpaths to matchthe design of other mockable timer APIs.
unintentionally removed earlier.
2. New regression test
test/parallel/test-mock-timers-abortsignal-timeout.js{ apis: ['AbortSignal.timeout'] }worksmock.tick()correctly triggers abortion after the expected delayNotes
I created a fresh branch to ensure a clean diff without unrelated changes and
to fully incorporate the feedback from @Renegade334 and @ErickWendel.
Please let me know if you'd like any adjustments to the test or implementation.
Thanks for the clear guidance and review support