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

Prohibit faking of faked timers #426

Merged
merged 6 commits into from Apr 13, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/fake-timers-src.js
@@ -1,6 +1,7 @@
"use strict";

const globalObject = require("@sinonjs/commons").global;
const globalDate = globalObject.Date;

/**
* @typedef {object} IdleDeadline
Expand Down Expand Up @@ -133,6 +134,7 @@ const globalObject = require("@sinonjs/commons").global;
* @returns {FakeTimers}
*/
function withGlobal(_global) {
const stockDate = _global.Date;
const userAgent = _global.navigator && _global.navigator.userAgent;
const isRunningInIE = userAgent && userAgent.indexOf("MSIE ") > -1;
const maxTimeout = Math.pow(2, 31) - 1; //see https://heycam.github.io/webidl/#abstract-opdef-converttoint
Expand Down Expand Up @@ -1635,6 +1637,16 @@ function withGlobal(_global) {
);
}

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

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!

Copy link
Contributor Author

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.

// Allow 100 ms variance for determining already faked timers.
// Timers are already faked; this is a problem.
// Make the user reset timers before continuing.
throw new TypeError("Can't fake timers twice.");
}

// eslint-disable-next-line no-param-reassign
config = typeof config !== "undefined" ? config : {};
config.shouldAdvanceTime = config.shouldAdvanceTime || false;
Expand Down
63 changes: 63 additions & 0 deletions test/fake-timers-test.js
Expand Up @@ -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", () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤔

Copy link
Contributor Author

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.

it("should not fake faked timers", () => {
const currentTime = new Date().getTime();
const date1 = new Date("2015-09-25");
const date2 = new Date("2015-09-26");
let clock = FakeTimers.install({ now: date1 });
assert.same(clock.now, date1.getTime());
assert.same(new Date().getTime(), 1443139200000);
assert.exception(function () {
FakeTimers.install({ now: date2 });
});
clock.uninstall();
clock = FakeTimers.install({ now: date2 });
assert.same(clock.now, date2.getTime());
clock.uninstall();
assert.greater(new Date().getTime(), currentTime, true);
});

it("should not fake faked timers on a custom target", () => {
const setTimeoutFake = sinon.fake();
const context = {
Date: Date,
setTimeout: setTimeoutFake,
clearTimeout: sinon.fake(),
};
let clock = FakeTimers.withGlobal(context).install();
assert.equals(setTimeoutFake.callCount, 1);
clock.setTimeout(NOOP, 0);
assert.equals(setTimeoutFake.callCount, 1);
assert.exception(function () {
clock = FakeTimers.withGlobal(context).install();
});
clock.uninstall();
});

it("should allow a fake on a custom target if the global is faked", () => {
const globalClock = FakeTimers.install();
const setTimeoutFake = sinon.fake();
const context = {
Date: Date,
setTimeout: setTimeoutFake,
clearTimeout: sinon.fake(),
};
const clock = FakeTimers.withGlobal(context).install();

clock.uninstall();
globalClock.uninstall();
});

it("should allow a fake on the global if a fake on a customer target is already defined", () => {
const setTimeoutFake = sinon.fake();
const context = {
Date: Date,
setTimeout: setTimeoutFake,
clearTimeout: sinon.fake(),
};
const clock = FakeTimers.withGlobal(context).install();
const globalClock = FakeTimers.install();

globalClock.uninstall();
clock.uninstall();
});
});
describe("issue #59", function () {
it("should install and uninstall the clock on a custom target", function () {
const setTimeoutFake = sinon.fake();
Expand Down