Prohibit faking of faked timers

[cjbarth]

Purpose

This change will prohibit the faking of already faked timers.

Background (Problem in detail)

This addresses https://github.com/sinonjs/sinon/issues/2449.

The problem this PR is addressing can occur when an exception is thrown before a timer is uninstalled 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 and Date-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.

[benjamingr]

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

[cjbarth]

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.

[cjbarth]

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 beforeEach and afterEach. Thoughts?

[mantoni]

Instead of leaking a global variable, you could take a reference of setTimeout and check if it isn't the original instance anymore, no?

[mroderick]

This is a very nice pull request, as it will help people where we are failing them 👍

[cjbarth]

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.

[cjbarth]

@benjamingr , can you please run the workflow too?

[cjbarth]

Does anyone have any idea why all the tests would run fine on my machine, and apparently here, but not on chromium? I don't quite follow what the test that failed is trying to do, and since it runs on my machine, I don't have a good way to trace down what is going on. Honestly, it seems like this might actually be a problem with the test trying to install twice. Or uninstalling improperly in chromium.

[benjamingr]

It looks like most tests pass and there is a single related failure at:

 1) loop limit stack trace
       "before each" hook for "provides a stack trace for running all sync":
     TypeError: Can't install fake timers twice on the same global object.
      at Object.install (src/fake-timers-src.js:1642)
      at Context.<anonymous> (test/fake-timers-test.js:5008)

[cjbarth]

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.

[cjbarth]

Actually, I'm not sure how I might fix this. The problem is that you are skipping a test, in which case, the afterEach of the parent describe doesn't run because technically the test never ran. That leaks the fake timer. If I move the installation of the fake timer to each lower-level describe block, it breaks some of the recursive stuff you're trying to do. Thoughts?

[cjbarth]

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[bot]]

Codecov Report

Merging #426 (c0e2a0a) into main (5ea3a4d) will increase coverage by 1.33%. The diff coverage is 100.00%.

:exclamation: Current head c0e2a0a differs from pull request most recent head 3b9250a. Consider uploading reports for the commit 3b9250a to get more accurate results

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

Flag	Coverage Δ
unit	95.51% <100.00%> (+1.33%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/fake-timers-src.js	95.51% <100.00%> (+1.33%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update f8a09c9...3b9250a. Read the comment docs.

[fatso83]

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 🙌

[mroderick]

The changes in this PR are published as @sinonjs/fake-timers@9.1.2 and are included in sinon@13.0.2