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.

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