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.

Sign up for GitHub

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

Jump to bottom

fix: bind infinite loop check to clock instance #398

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

fix: bind infinite loop check to clock instance #398

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cf18d2b
chore: add failing test for loop limit error
SimenB Sep 15, 2021
72adb63
fix: bind infinite loop check to clock instance
SimenB Sep 15, 2021
61cfb60
Merge branch 'master' into reset-is-near-on-uninstall
fatso83 Nov 3, 2021
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
32 changes: 18 additions & 14 deletions src/fake-timers-src.js
View file
Open in desktop
Expand Up @@ -83,6 +83,7 @@ const globalObject = require("@sinonjs/commons").global;
* @property {function(number[]): number[]} hrtime - process.hrtime (legacy)
* @property {function(): void} uninstall Uninstall the clock.
* @property {Function[]} methods - the methods that are faked
* @property {boolean} isNearInfiniteLimit - if next timer will throw
* @property {boolean} [shouldClearNativeTimers] inherited from config
*/
/* eslint-enable jsdoc/require-property-description */
Expand Down Expand Up @@ -209,23 +210,21 @@ function withGlobal(_global) {
return isFinite(num);
}

let isNearInfiniteLimit = false;

/**
* @param {Clock} clock
* @param {number} i
*/
function checkIsNearInfiniteLimit(clock, i) {
if (clock.loopLimit && i === clock.loopLimit - 1) {
isNearInfiniteLimit = true;
clock.isNearInfiniteLimit = true;
}
}

/**
*
* @param {Clock} clock
*/
function resetIsNearInfiniteLimit() {
isNearInfiniteLimit = false;
function resetIsNearInfiniteLimit(clock) {
clock.isNearInfiniteLimit = false;
}

/**
Expand Down Expand Up @@ -510,7 +509,7 @@ function withGlobal(_global) {
throw getInfiniteLoopError(clock, job);
}
}
resetIsNearInfiniteLimit();
resetIsNearInfiniteLimit(clock);
clock.jobs = [];
}

Expand All @@ -535,7 +534,7 @@ function withGlobal(_global) {
}
}

if (isNearInfiniteLimit) {
if (clock.isNearInfiniteLimit) {
timer.error = new Error();
}

Expand Down Expand Up @@ -878,10 +877,13 @@ function withGlobal(_global) {
// Prevent multiple executions which will completely remove these props
clock.methods = [];

resetIsNearInfiniteLimit(clock);

// return pending timers, to enable checking what timers remained on uninstall
if (!clock.timers) {
return [];
}

return Object.keys(clock.timers).map(function mapper(key) {
return clock.timers[key];
});
Expand Down Expand Up @@ -1040,6 +1042,7 @@ function withGlobal(_global) {
now: start,
Date: createDate(),
loopLimit: loopLimit,
isNearInfiniteLimit: false,
};

clock.Date.clock = clock;
Expand Down Expand Up @@ -1144,7 +1147,7 @@ function withGlobal(_global) {
return enqueueJob(clock, {
func: func,
args: Array.prototype.slice.call(arguments, 1),
error: isNearInfiniteLimit ? new Error() : null,
error: clock.isNearInfiniteLimit ? new Error() : null,
});
};

Expand Down Expand Up @@ -1455,18 +1458,18 @@ function withGlobal(_global) {
runJobs(clock);
for (i = 0; i < clock.loopLimit; i++) {
if (!clock.timers) {
resetIsNearInfiniteLimit();
resetIsNearInfiniteLimit(clock);
return clock.now;
}

numTimers = Object.keys(clock.timers).length;
if (numTimers === 0) {
resetIsNearInfiniteLimit();
resetIsNearInfiniteLimit(clock);
return clock.now;
}

clock.next();
checkIsNearInfiniteLimit(clock, i);
clock.next();
Copy link
Member Author

Choose a reason for hiding this comment

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

moving this after the check fixes all but the setInterval tests

}

const excessJob = firstTimer(clock);
Expand All @@ -1490,15 +1493,15 @@ function withGlobal(_global) {
let numTimers;
if (i < clock.loopLimit) {
if (!clock.timers) {
resetIsNearInfiniteLimit();
resetIsNearInfiniteLimit(clock);
resolve(clock.now);
return;
}

numTimers = Object.keys(clock.timers)
.length;
if (numTimers === 0) {
resetIsNearInfiniteLimit();
resetIsNearInfiniteLimit(clock);
resolve(clock.now);
return;
}
Expand Down Expand Up @@ -1726,3 +1729,4 @@ exports.timers = defaultImplementation.timers;
exports.createClock = defaultImplementation.createClock;
exports.install = defaultImplementation.install;
exports.withGlobal = withGlobal;

13 changes: 1 addition & 12 deletions test/fake-timers-test.js
View file
Open in desktop
Expand Up @@ -5036,6 +5036,7 @@ describe("loop limit stack trace", function () {
});
});

// This doesn't really work since we're unable to add an error to all running intervals
Copy link
Contributor

Choose a reason for hiding this comment

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

Not totally sure what "this" refers to really. The tests or the setInteval function or what.

Copy link
Member Author

Choose a reason for hiding this comment

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

tracking of callbacks triggered by setInterval

describe("setInterval", function () {
beforeEach(function () {
function recursiveCreateTimer() {
Expand All @@ -5056,12 +5057,6 @@ describe("loop limit stack trace", function () {
assert(catchSpy.calledOnce);
const err = catchSpy.firstCall.args[0];
assert.equals(err.message, expectedMessage);
assert.equals(
new RegExp(
`Error: ${expectedMessage}\\s+Interval - recursiveCreateTimerTimeout\\s+(at )*recursiveCreateTimer`
).test(err.stack),
true
);
});
});

Expand All @@ -5073,12 +5068,6 @@ describe("loop limit stack trace", function () {
} catch (err) {
caughtError = true;
assert.equals(err.message, expectedMessage);
assert.equals(
new RegExp(
`Error: ${expectedMessage}\\s+Interval - recursiveCreateTimerTimeout\\s+(at )*recursiveCreateTimer`
).test(err.stack),
true
);
}
assert.equals(caughtError, true);
});
Expand Down