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

Faking performance when not present should throw an error (and minor changes) - fix for #374 #400

Merged
merged 3 commits into from
Nov 15, 2021
Merged

Faking performance when not present should throw an error (and minor changes) - fix for #374 #400

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
81c5e9b
faking performance when it is not present throws
itayperry Sep 16, 2021
fa323c8
test no longer conditional and now uses built-in exception assertion
itayperry Nov 13, 2021
584a20c
README passes prettier check
itayperry Nov 15, 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
14 changes: 7 additions & 7 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,14 +150,14 @@ The `loopLimit` argument sets the maximum number of timers that will be run when

Installs FakeTimers using the specified config (otherwise with epoch `0` on the global scope). The following configuration options are available

| Parameter | Type | Default | Description |
| -------------------------------- | ----------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| `config.now` | Number/Date | 0 | installs FakeTimers with the specified unix epoch |
| Parameter | Type | Default | Description |
| -------------------------------- | ----------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| `config.now` | Number/Date | 0 | installs FakeTimers with the specified unix epoch |
| `config.toFake` | String[] | ["setTimeout", "clearTimeout", "setImmediate", "clearImmediate","setInterval", "clearInterval", "Date", "requestAnimationFrame", "cancelAnimationFrame", "requestIdleCallback", "cancelIdleCallback", "hrtime", "performance"] | an array with explicit function names (or objects, in the case of "performance") to hijack. _When not set, FakeTimers will automatically fake all methods **except** `nextTick`_ e.g., `FakeTimers.install({ toFake: ["setTimeout","nextTick"]})` will fake only `setTimeout` and `nextTick` |
| `config.loopLimit` | Number | 1000 | the maximum number of timers that will be run when calling runAll() |
| `config.shouldAdvanceTime` | Boolean | false | tells FakeTimers to increment mocked time automatically based on the real system time shift (e.g. the mocked time will be incremented by 20ms for every 20ms change in the real system time) |
| `config.advanceTimeDelta` | Number | 20 | relevant only when using with `shouldAdvanceTime: true`. increment mocked time by `advanceTimeDelta` ms every `advanceTimeDelta` ms change in the real system time. |
| `config.shouldClearNativeTimers` | Boolean | false | tells FakeTimers to clear 'native' (i.e. not fake) timers by delegating to their respective handlers. These are not cleared by default, leading to potentially unexpected behavior if timers existed prior to installing FakeTimers. |
| `config.loopLimit` | Number | 1000 | the maximum number of timers that will be run when calling runAll() |
| `config.shouldAdvanceTime` | Boolean | false | tells FakeTimers to increment mocked time automatically based on the real system time shift (e.g. the mocked time will be incremented by 20ms for every 20ms change in the real system time) |
| `config.advanceTimeDelta` | Number | 20 | relevant only when using with `shouldAdvanceTime: true`. increment mocked time by `advanceTimeDelta` ms every `advanceTimeDelta` ms change in the real system time. |
| `config.shouldClearNativeTimers` | Boolean | false | tells FakeTimers to clear 'native' (i.e. not fake) timers by delegating to their respective handlers. These are not cleared by default, leading to potentially unexpected behavior if timers existed prior to installing FakeTimers. |

### `var id = clock.setTimeout(callback, timeout)`

Expand Down
33 changes: 19 additions & 14 deletions src/fake-timers-src.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1584,20 +1584,6 @@ function withGlobal(_global) {

if (performancePresent) {
clock.performance = Object.create(null);

if (hasPerformancePrototype) {
const proto = _global.Performance.prototype;

Object.getOwnPropertyNames(proto).forEach(function (name) {
if (name.indexOf("getEntries") === 0) {
// match expected return type for getEntries functions
clock.performance[name] = NOOP_ARRAY;
} else {
clock.performance[name] = NOOP;
}
});
}

clock.performance.now = function FakeTimersNow() {
const hrt = hrtime();
const millis = hrt[0] * 1000 + hrt[1] / 1e6;
Expand Down Expand Up @@ -1675,6 +1661,25 @@ function withGlobal(_global) {
clock.attachedInterval = intervalId;
}

if (clock.methods.includes("performance")) {
Copy link
Contributor

@fatso83 fatso83 Sep 17, 2021 •
edited

Choose a reason for hiding this comment

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

The check here in line 1602 and line 1613 is basically the same, right? If so, I'd make that explicit by replace this one with the config check and at the same time flatten the conditionals by throwing early:

if (config.toFake.includes("performance") {
            if (!hasPerformancePrototype) { 
                    throw new ReferenceError("non-existent performance object cannot be faked");
            }
            // code to stub out performance follows 
            var proto = _global.Performance.prototype;        
            ...
}

Optionally, put the suite of tests inside a describe() block and stuff the this.skip() in a check inside a beforeEach. Only if there are more than one test, ofc.

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 tested it now and it's not exactly the same - many tests are failing, I think it's due to this piece of code that runs earlier:

        clock.methods = config.toFake || [];

        if (clock.methods.length === 0) {
            // do not fake nextTick by default - GitHub#126
            clock.methods = Object.keys(timers).filter(function (key) {
                return key !== "nextTick" && key !== "queueMicrotask";
            });
        }

if config.toFake is empty I still need to stub performance in many tests cause the clock.methods array will later have it

...and at the same time flatten the conditionals by throwing early

I still wanna do that for the code to be cleaner - maybe I'll separate the conditions entirely? But the code still ain't pretty I'm afraid.

As in, doing the first one that throws in here:

clock.methods = config.toFake || [];

if (
    config.toFake &&
    config.toFake.includes("performance") &&
    !hasPerformancePrototype
) {
    // user explicitly tried to fake performance when not present
    throw new ReferenceError(
        "non-existent performance object cannot be faked"
    );
}

if (clock.methods.length === 0) {
    // do not fake nextTick by default - GitHub#126
    clock.methods = Object.keys(timers).filter(function (key) {
        return key !== "nextTick" && key !== "queueMicrotask";
    });
}

and later do this:

        if (clock.methods.includes("performance") && hasPerformancePrototype) {
            var proto = _global.Performance.prototype;
            Object.getOwnPropertyNames(proto).forEach(function (name) {
                if (name !== "now") {
                    clock.performance[name] =
                        name.indexOf("getEntries") === 0 ? NOOP_ARRAY : NOOP;
                }
            });
        }

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK, the problem you have is that the default methods to fake includes stuff that is not necessarily supported, right? If so, why not just create a general isSupported map that you can check in that loop? Something like :

const isSupported = new Map();
isSupported.set("performance", hasPerformancePrototype);
isSupported.set("nextTick", hasNextTick);
...
const supported = key => !isSupported.has(key) || isSupported.get(key) // true for all not explicitly added
if (clock.methods.length === 0) {
  clock.methods = Object.keys(timers).filter(supported).filter(function (key) {
      return key !== "nextTick" && key !== "queueMicrotask";
  });
}

That should filter out performance and other unsupported stuff from the default list of methods to fake if toFake was empty.

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 have another question - regarding the suggested code above:

if (config.toFake.includes("performance") {
            if (!hasPerformancePrototype) { 
                    throw new ReferenceError("non-existent performance object cannot be faked");
            }
            // code to stub out performance follows 
            var proto = _global.Performance.prototype;        
            ...
}

It ignores the situation in which toFake is empty and then the entire var proto = _global.Performance.prototype; does not run, and it should in many cases - the isSupported Map does not seem to deal directly with this problem, although it's a really cool and neat idea, and maybe I should do it anyway.

if (hasPerformancePrototype) {
var proto = _global.Performance.prototype;
Object.getOwnPropertyNames(proto).forEach(function (name) {
if (name !== "now") {
clock.performance[name] =
name.indexOf("getEntries") === 0
? NOOP_ARRAY
: NOOP;
}
});
} else if (config.toFake.includes("performance")) {
// user explicitly tried to fake performance when not present
throw new ReferenceError(
"non-existent performance object cannot be faked"
);
}
}

for (i = 0, l = clock.methods.length; i < l; i++) {
const nameOfMethodToReplace = clock.methods[i];
if (nameOfMethodToReplace === "hrtime") {
Expand Down
21 changes: 21 additions & 0 deletions test/fake-timers-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3706,6 +3706,27 @@ describe("FakeTimers", function () {
});
}

it("throws when adding performance to tofake array when performance not present", function () {
assert.exception(
function () {
const setTimeoutFake = sinon.fake();
const context = {
Date: Date,
setTimeout: setTimeoutFake,
clearTimeout: sinon.fake(),
performance: undefined,
};
FakeTimers.withGlobal(context).install({
toFake: ["performance"],
});
},
{
name: "ReferenceError",
message: "non-existent performance object cannot be faked",
}
);
});

if (performanceNowPresent) {
it("replaces global performance.now", function () {
this.clock = FakeTimers.install();
Expand Down