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

[itayperry]

Original purpose: enabling adding performance to toFake array - apparently this was already possible so a few minor changes were made.

Issue #374 refers to the fact that fake-timers overrides performance object (as in other properties/methods in addition to overriding performance.now()). The first suggested solution/notion was to enable users to add performance to the toFake array - after realizing this was already possible a test was added to make sure an error is thrown when a user puts performance in the toFake array but the performance object isn't present.

I don't know if this PR makes a good addition/improvement - I'm waiting for your notes and comments and I'll fix whatever's needed.

[fatso83]

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.

[itayperry]

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;
                }
            });
        }

[fatso83]

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.

[itayperry]

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.

[fatso83]

This is an anti-pattern. I see it is being done above at line 3682, but we should use Mocha's built-ins to skip tests where they use unsupported featues. Generally do this instead:

            it("throws when adding performance to tofake array when performance not present", function () {
              if(!performanceNowPresent) { this.skip(); }
           ...
}

The difference is that this shows the test as running and whether or not it is actually run in a bigger suite.

I would perhaps actually rewrite this test so that it does not depend on the current environment at all. We have the possibility of choosing what the target is, so you could just create a dumb const myGlobal = { setTimer: noop, setInterval: noop, performance: undefined } and directly pass that. That can run regardless of environment and will give you immediate feedback on whether it works or not, even in Node or Chrome.

[itayperry]

use Mocha's built-ins to skip tests

You're so right! I've used this before and I should have known better than to forget about it :)

We have the possibility of choosing what the target is

I didn't think about it, it's really cool! I can't seem to get it to work exactly like that but I cheated a little lol and stole properties from another test that uses a global/target and it finally worked!

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

Is this ok?

[fatso83]

Hi, @itayperry . Sorry, for leaving you hanging for more than a month! I was not aware I was blocking it. This seems good to me!

With regards to the assertion, I would check out the exception assertion in referee: https://sinonjs.github.io/referee/#exception

[itayperry]

I was too insecure to send a message lol I'm on it. I'll do it as fast as I can :)

I would check out the exception assertion in referee

Looks like best practice! Thank you :)

[codecov]

Codecov Report

Merging #400 (584a20c) into master (a13a054) will increase coverage by 0.18%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #400      +/-   ##
==========================================
+ Coverage   94.10%   94.29%   +0.18%     
==========================================
  Files           1        1              
  Lines         611      613       +2     
==========================================
+ Hits          575      578       +3     
+ Misses         36       35       -1     

Flag	Coverage Δ
unit	94.29% <100.00%> (+0.18%)	⬆️

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

Impacted Files	Coverage Δ
src/fake-timers-src.js	94.29% <100.00%> (+0.18%)	⬆️

---

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 a13a054...584a20c. Read the comment docs.

[itayperry]

Hi @fatso83 :) I didn't edit the README file, but I still get a prettier error because of it - should I fix that and just make another commit?

[warn] README.md

[fatso83]

Yup, that was me editing the README in the web UI.

[fatso83]

Finally mergeable! And in time for X-Mas! 😸 Great stuff, Itay.

[itayperry]

For glory, for fake-timers! lol :)