-
Notifications
You must be signed in to change notification settings - Fork 107
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
Add mocks to NodeJS "timers" module #467
Conversation
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.
I think you could probably afford adding a line to the README that we touch the "timers"
module, but otherwise this generally looks fine! Just a few minor things.
test/issue-466-test.js
Outdated
@@ -0,0 +1,144 @@ | |||
"use strict"; |
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.
Stuff the tests inside of this into a suitably named new describe block in fake-timers-test.js
. We usually reserve the issues*
files for regression test cases.
test/issue-466-test.js
Outdated
if (typeof require === "function" && typeof module === "object") { | ||
try { | ||
timersModule = require("timers"); | ||
} catch (e) { | ||
// ignored | ||
} | ||
} |
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.
Just use Mocha's built-ins to selectively run it in supported environments
before(function () {
if (!timersModule) {
this.skip();
}
});
Enables a bit better reporting :)
src/fake-timers-src.js
Outdated
@@ -886,6 +894,14 @@ function withGlobal(_global) { | |||
} | |||
} | |||
} | |||
if (clock.timersModuleMocks) { |
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.
They are not actual mocks, though, so could trim that bit off.
To be consistent with the methods
prop, just name it timersModuleMethods
All done :) |
No but that's a good point, it should respect the regular "should I mock setTimeout/setInterval or whatever" option (toFake) |
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.
Good job!
Does it correctly not mock methods if they're not part of toFake?
Yes it does :) |
Out as 10.2.0 |
Seems like #470 made a good candidate for why this could have been behind a flag. Some weird regression hitting us. |
The `nock` library is not compatible with the fake timers we use in unit tests because it uses the Node.js `timers` API. This API is not mocked correctly by the version of Jest we are using. Jest uses `@sinon/fake-timers` internally, which didn't support mocking the Node.js `timers` API until v11.0.0 (see sinonjs/fake-timers#467) This package is updated in Jest as part of the v30 release, which is currently under development. To workaround this problem in the meantime, the `nock` package has been updated and patched to use global timers rather than the `timers` API. Global timers are mocked correctly. This was required for some unit tests that I will be submitting in a different PR, intended for #24503
<!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **Description** The `nock` library is not compatible with the fake timers we use in unit tests because it uses the Node.js `timers` API. This API is not mocked correctly by the version of Jest we are using. Jest uses `@sinon/fake-timers` internally, which didn't support mocking the Node.js `timers` API until v11.0.0 (see sinonjs/fake-timers#467) This package is updated in Jest as part of the v30 release, which is currently under development. To workaround this problem in the meantime, the `nock` package has been updated and patched to use global timers rather than the `timers` API. Global timers are mocked correctly. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/24805?quickstart=1) ## **Related issues** This was required for some unit tests that I will be submitting in a different PR, intended for #24503 ## **Manual testing steps** N/A ## **Screenshots/Recordings** N/A ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.
Purpose (TL;DR) - mandatory
Fix issue #466
Solution
It works like this:
Try to import
timers
module astimersModule
. If it doesn't work, that's okay.Upon
FakeTimers.install()
, iff we're installing on theglobalObject
andtimersModule
was successfully imported:For each property that was mocked on the globalObject iff it also exists on
timersModule
:Safe its name and original value from
timersModule
to a new property on the Clock object.Then set it to the same property as the faked property on
globalObject
.Upon
FakeTimers.uninstall()
, iff the object holdingtimersModule
's originals is truthy:For each entry set the corresponding property back to the original.