Skip to content
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

Merged
merged 6 commits into from
May 18, 2023

Conversation

swenzel-arc
Copy link
Contributor

Purpose (TL;DR) - mandatory

Fix issue #466

Solution

It works like this:

Try to import timers module as timersModule. If it doesn't work, that's okay.

Upon FakeTimers.install(), iff we're installing on the globalObject and timersModule 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 holding timersModule's originals is truthy:
For each entry set the corresponding property back to the original.

Copy link
Contributor

@fatso83 fatso83 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 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.

@@ -0,0 +1,144 @@
"use strict";
Copy link
Contributor

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.

Comment on lines 7 to 13
if (typeof require === "function" && typeof module === "object") {
try {
timersModule = require("timers");
} catch (e) {
// ignored
}
}
Copy link
Contributor

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 :)

@@ -886,6 +894,14 @@ function withGlobal(_global) {
}
}
}
if (clock.timersModuleMocks) {
Copy link
Contributor

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

@swenzel-arc
Copy link
Contributor Author

All done :)
Btw. do you think we should add a config flag that allows to skip altering the timers module?

@benjamingr
Copy link
Member

Btw. do you think we should add a config flag that allows to skip altering the timers module?

No but that's a good point, it should respect the regular "should I mock setTimeout/setInterval or whatever" option (toFake)

Copy link
Member

@benjamingr benjamingr left a 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?

@swenzel-arc
Copy link
Contributor Author

Does it correctly not mock methods if they're not part of toFake?

Yes it does :)

@fatso83 fatso83 merged commit a111a71 into sinonjs:main May 18, 2023
9 checks passed
@fatso83
Copy link
Contributor

fatso83 commented May 18, 2023

Out as 10.2.0

@fatso83
Copy link
Contributor

fatso83 commented Jun 12, 2023

Seems like #470 made a good candidate for why this could have been behind a flag. Some weird regression hitting us.

Gudahtt added a commit to MetaMask/metamask-extension that referenced this pull request May 27, 2024
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
Gudahtt added a commit to MetaMask/metamask-extension that referenced this pull request May 28, 2024
<!--
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants