Prohibit faking of faked timers #426

cjbarth · 2022-04-09T18:31:42Z

Purpose

This change will prohibit the faking of already faked timers.

Background (Problem in detail)

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 · 2022-04-10T08:28:32Z

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 · 2022-04-10T13:25:07Z

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.

fatso83

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 🤓

fatso83 · 2022-04-10T14:46:31Z

test/fake-timers-test.js

@@ -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", () => {


Suggested change

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 🤔

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.

fatso83 · 2022-04-10T14:57:21Z

src/fake-timers-src.js

+        if (
+            typeof stockDate === "function" &&
+            Math.abs(new Date().getTime() - new stockDate().getTime()) > 100
+        ) {


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!

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.

cjbarth · 2022-04-11T13:23:46Z

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 · 2022-04-11T17:14:19Z

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 · 2022-04-11T17:16:30Z

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

cjbarth · 2022-04-11T18:41:24Z

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.

benjamingr

Changes lgtm, good tests :)

cjbarth · 2022-04-11T20:28:15Z

@benjamingr , can you please run the workflow too?

cjbarth · 2022-04-11T22:11:01Z

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 · 2022-04-12T06:43:44Z

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 · 2022-04-12T13:52:58Z

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 · 2022-04-12T14:06:01Z

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 · 2022-04-12T14:13:23Z

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 · 2022-04-13T07:00:54Z

Codecov Report

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

❗ 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%)`	⬆️

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%)`	⬆️

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 · 2022-04-13T07:01:19Z

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 🙌

test/fake-timers-test.js

mroderick · 2022-04-14T15:43:29Z

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

Disallow faking of faked timers

fb58a72

cjbarth mentioned this pull request Apr 9, 2022

Impossible to restore fake timers in certain situations. sinonjs/sinon#2449

Closed

fatso83 reviewed Apr 10, 2022

View reviewed changes

cjbarth added 2 commits April 10, 2022 17:42

Use a property on the global context when fake is installed

c83aa3b

Lint

797512c

Correct isFake property placement

41d52bd

benjamingr approved these changes Apr 11, 2022

View reviewed changes

Fix broken test

c0e2a0a

fatso83 approved these changes Apr 13, 2022

View reviewed changes

test/fake-timers-test.js Outdated Show resolved Hide resolved

typo

3b9250a

fatso83 merged commit 7046e72 into sinonjs:main Apr 13, 2022

cjbarth deleted the incepted-timers branch April 13, 2022 11:53

WikiRik mentioned this pull request Apr 14, 2022

chore(deps): update dependency sinon to v13.0.2 sequelize/sequelize#14390

Merged

1 task

cjbarth mentioned this pull request Apr 14, 2022

Correctly reset Sinon fake timers node-saml/node-saml#60

Merged

WikiRik mentioned this pull request Apr 15, 2022

test: remove additional fake timer sequelize/sequelize#14401

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prohibit faking of faked timers #426

Prohibit faking of faked timers #426

cjbarth commented Apr 9, 2022

benjamingr commented Apr 10, 2022

cjbarth commented Apr 10, 2022

fatso83 left a comment

fatso83 Apr 10, 2022

cjbarth Apr 10, 2022

fatso83 Apr 10, 2022

cjbarth Apr 10, 2022

cjbarth commented Apr 11, 2022

mantoni commented Apr 11, 2022

mroderick commented Apr 11, 2022

cjbarth commented Apr 11, 2022

benjamingr left a comment

cjbarth commented Apr 11, 2022

cjbarth commented Apr 11, 2022

benjamingr commented Apr 12, 2022

cjbarth commented Apr 12, 2022

cjbarth commented Apr 12, 2022

cjbarth commented Apr 12, 2022

codecov bot commented Apr 13, 2022 •

edited

fatso83 commented Apr 13, 2022

mroderick commented Apr 14, 2022

	describe("issue #2449: perminent loss of native functions", () => {
	describe("issue #2449: permanent loss of native functions", function () {

Prohibit faking of faked timers #426

Prohibit faking of faked timers #426

Conversation

cjbarth commented Apr 9, 2022

Purpose

Background (Problem in detail)

Solution

benjamingr commented Apr 10, 2022

cjbarth commented Apr 10, 2022

fatso83 left a comment

Choose a reason for hiding this comment

fatso83 Apr 10, 2022

Choose a reason for hiding this comment

cjbarth Apr 10, 2022

Choose a reason for hiding this comment

fatso83 Apr 10, 2022

Choose a reason for hiding this comment

cjbarth Apr 10, 2022

Choose a reason for hiding this comment

cjbarth commented Apr 11, 2022

mantoni commented Apr 11, 2022

mroderick commented Apr 11, 2022

cjbarth commented Apr 11, 2022

benjamingr left a comment

Choose a reason for hiding this comment

cjbarth commented Apr 11, 2022

cjbarth commented Apr 11, 2022

benjamingr commented Apr 12, 2022

cjbarth commented Apr 12, 2022

cjbarth commented Apr 12, 2022

cjbarth commented Apr 12, 2022

codecov bot commented Apr 13, 2022 • edited

Codecov Report

fatso83 commented Apr 13, 2022

mroderick commented Apr 14, 2022

codecov bot commented Apr 13, 2022 •

edited