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
Prohibit faking of faked timers #426
Conversation
I'm not sure we should do this - I can see someone writing a test using fake timers inside a suite using fake timers and expecting double faking + restoring to work |
Well, it doesn't now, it fails silently. So, this change will let them know that what they are doing doesn't work. It is actually my guess that if people are trying to get this to work, and their tests are passing (like those in the project I'm working on), then they would probably like to know that their tests are faulty (like I would have wanted to know). It was easy enough to change my tests to be compliant once I knew the problem. Additionally, no documentation says you can fake faked timers, so if they are doing that, it is undocumented and broken. |
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 like the idea you propose here, as it seems to support the most common scenario one would want to avoid. Theoretically, someone could be doing the suggested double-faking, but as you pointed out, that does not actually work, so if that is the case, I have no objections towards doing this. Just the implementation 🤓
test/fake-timers-test.js
Outdated
@@ -53,6 +53,69 @@ const utilPromisifyAvailable = promisePresent && utilPromisify; | |||
const timeoutResult = global.setTimeout(NOOP, 0); | |||
const addTimerReturnsObject = typeof timeoutResult === "object"; | |||
|
|||
describe("issue #2449: perminent loss of native functions", () => { |
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.
describe("issue #2449: perminent loss of native functions", () => { | |
describe("issue #2449: permanent loss of native functions", function () { |
typo-fix. Also Mocha relies heavily on this
, so it's a malpractice to use arrow functions. Not sure why we do not have ESLint rules to handle this 🤔
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.
Done. Also, I forgot to run npm run lint
, so I didn't see the problem with the arrow functions. However, since I'm not using this
in my tests, it wouldn't have mattered.
src/fake-timers-src.js
Outdated
if ( | ||
typeof stockDate === "function" && | ||
Math.abs(new Date().getTime() - new stockDate().getTime()) > 100 | ||
) { |
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 do not fancy the implicitness of this. It also won't cover a bunch of scenarios where we do not fake Date
. fake-timers support faking a single timer or many the APIs. See the toFake
config option: https://github.com/sinonjs/fake-timers#var-clock--faketimersinstallconfig
I would much rather be specific in testing for a property on the various timers, along the lines of what Luxon DateTime.isDatetime(obj)
is doing. This would only mean adding a boolean isFakeTimer
to Date and the timers. And the test would be explicit!
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 noted that the faked timers can't be used unless install
is first called, so I tried to put the property on the global only when install
is called. I tried to leave the global object clean after uninstall
.
This global leak detection thing is really causing a problem. It seems that adding a property, even if I remove it when done, prohibits Mocha leak detection from working correctly with |
Instead of leaking a global variable, you could take a reference of |
This is a very nice pull request, as it will help people where we are failing them 👍 |
Ok, I've made another adjustment that is actually even more correct than my first try at this. This tracks very well if the object is a fake or not. |
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.
Changes lgtm, good tests :)
@benjamingr , can you please run the workflow too? |
Does anyone have any idea why all the tests would run fine on my machine, and apparently here, but not on |
It looks like most tests pass and there is a single related failure at:
|
So, after further research, I found that this failing test is actually leaking a faked date instance. So, even the tests for the project were in need of this exception to catch bad use of these timers! I'll push up a fix in a bit. |
Actually, I'm not sure how I might fix this. The problem is that you are skipping a test, in which case, the |
Ok, I fixed the test. If you have any questions, please let me know. Otherwise, I look forward to this landing. I don't think it would require a semver-major release, in fact, it is probably just a semver-patch release because it "fixes" a bug. |
Codecov Report
@@ Coverage Diff @@
## main #426 +/- ##
==========================================
+ Coverage 94.17% 95.51% +1.33%
==========================================
Files 1 1
Lines 618 624 +6
==========================================
+ Hits 582 596 +14
+ Misses 36 28 -8
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Thank you for sticking with us all the way through. Great work and great tests, and that you discovered a bug in the test suite just shows all the more so how this prevents common cases 🙌 |
The changes in this PR are published as |
Purpose
This change will prohibit the faking of already faked timers.
Background (Problem in detail)
This addresses sinonjs/sinon#2449.
The problem this PR is addressing can occur when an exception is thrown before a timer is
uninstall
ed or if a nested test tries to fake timers again instead of adjusting the currently faked timer or clearing the previous fake first.If a faked timer is faked again, it will be impossible to recover the original
Date
andDate
-related functions again in call cases. So, instead of trying to detect incepted faked timers, this is prohibited in all cases.Solution
If an attempt is make to fake a timer that is already faked, an exception will be thrown.