Fake timers override mocks on performance object

[Smrtnyk]

• FakeTimers version : I am using jest: 26.6.3 which uses FakeTimers
• Environment : Windows 10 10.0.19042
• Other libraries you are using: jest 26.6.3

I am actually redirected here from https://github.com/facebook/jest/issues/11330 I am using jest with jsdom

What did you expect to happen? I would expect that properties that I added to be preserved after fake timers install

What actually happens performance object is overriden and properties I added before install are gone

How to reproduce I will explain how I did it in jest which they say they just only install FakeTimers and don't do any logic Please check this https://github.com/facebook/jest/issues/11330

I have global mocks file where before testing I do this

Object.defineProperties(performance, {
    timing: {
        value: {
            connectStart: now + 1,
            connectEnd: now + 1,
            domComplete: now + 100,
            domContentLoadedEventEnd: now + 50,
            domContentLoadedEventStart: now + 40,
            domInteractive: now + 39,
            domLoading: now + 10,
            domainLookupStart: now + 1,
            domainLookupEnd: now + 1,
            fetchStart: now + 1,
            loadEventEnd: now + 1000,
            loadEventStart: now + 1000,
            navigationStart: now,
            redirectEnd: 0,
            redirectStart: 0,
            requestStart: now + 1,
            responseStart: now + 2,
            responseEnd: now + 30,
            secureConnectionStart: 0,
            unloadEventEnd: 0,
            unloadEventStart: 0
        },
        writable: true
    },
    navigation: {
        value: {
            type: 0
        },
        writable: true
    },
    getEntries: {
        value: (): PerformanceEntry[] => {
            return [];
        },
        writable: true
    },
    getEntriesByType: {
        value: (type: string): (PerformanceNavigationTiming | PerformanceResourceTiming)[] => {
            return performanceObj.filter(perf => {
                return perf.entryType === type;
            });
        },
        writable: true
    },
    getEntriesByName: {
        value: (): PerformanceEntry[] => {
            return [];
        },
        writable: true
    },
    setResourceTimingBufferSize: {
        value: jest.fn(),
        writable: true
    },
    clearResourceTimings: {
        value: jest.fn(),
        writable: true
    }
});

My app relies on these values. After I install FakeTimers the performance.timing is gone With jest legacy timers (which are not sinon/FakeTimers) this is not happening

From what I get is that FakeTimers mock performance.now() etc... but is it possible to preserve values that were added to performance object before clock install?

[benjamingr]

From what I get is that FakeTimers mock performance.now() etc... but is it possible to preserve values that were added to performance object before clock install?

Yes, that sounds like a reasonable ask. PR welcome or if another contributor wants to take it :)

[Smrtnyk]

alright, I am pretty much overloaded currently with work but will try to brew something if noone does this in the near future

[benjamingr]

@itayperry want to take a stab at this :)?

[itayperry]

I'll take a look into it - I might ask for some advice :) Hope I'll be able to help! 🙂

[fatso83]

Seems like easy pickings for anyone interested party to pick up.

[itayperry]

https://github.com/sinonjs/fake-timers/blob/604f0909b7023133a66f56b1aba701501dc61992/src/fake-timers-src.js#L1526-L1547

Hi everyone, it took me a while to get to this :) I looked at the code and it seems like performance.now() and a few kinds of performance.getEntries() are the significant methods and properties that need to be rewritten. Is it okay to just change them and leave the rest as it is?

Something like that:

clock.performance['getEntries'] = NOOP_ARRAY; 
clock.performance['getEntriesByName'] = NOOP_ARRAY; 
clock.performance['getEntriesByType'] = NOOP_ARRAY; 

clock.performance['anythingElse'] = _global.performance['anythingElse']; (the original value); 

https://github.com/sinonjs/fake-timers/blob/604f0909b7023133a66f56b1aba701501dc61992/test/fake-timers-test.js#L3751-L3775

[Smrtnyk]

I do understand that you need to overwrite performance.now() due to timers controll in sync manner, but can you explain why clock.performance['getEntries'] = NOOP_ARRAY; clock.performance['getEntriesByName'] = NOOP_ARRAY; clock.performance['getEntriesByType'] = NOOP_ARRAY; need to be overriden? In my case I mock these in my global setup that happens before tests run (they do not give anything in jsdom) so I can have some mock resources to work in my tests. In your proposal after I install the clock it will override the spies and I will need to remock them again

[itayperry]

I'm not sure I know why it was done that way, I'm taking a look at the source code and at old PRs and issues to see how it originated :)

[Smrtnyk]

Lets see what owners think. I do not see the relation between timers and methods that return array of resources from the performance object. Maybe I am wrong and missing something, so clarification would be nice.

[itayperry]

Found it! "Return empty arrays for performance.getEntries, other relevant methods" PR #240

[Smrtnyk]

I can live with installing clock destroying my performance getEntries() mocks as long as I can remock them again after clock install. I still am not sure why getEntries should be affected with the timers, it is just returning an array of performance entries and in case of jsdom it is not defined (hence the reason of my need to mock it).

[fatso83]

I still am not sure why getEntries should be affected with the timer

Usually we are not afforded the luxury of knowing why the authors did as they did originally for every line, but it is not very hard to make an educated guess that makes sense when looking at the code and considering the context

• you are an author that wants to make sure Performance is stubbed and restored in the tests.
• you want to spend as little time as possible to make it work
• you are more interested in making most code just pass than considering every little detail
• you know that if someone is interested in better stubs they will provide them

As you see, this was originally just a simple loop over every property on the Performance object that replaced everything with no-ops. That has to be the smallest possible implementation. It was just slightly modified when someone found that this changed the API for getEntries.

The reason why it should still probably be stubbed is probably just that these entries come from some actual implementation and that it would not be right to return something that is based on timers that are not present. You could, of course, supply a PR that tells fake-timers not to install a stub for this specific method, but it would probably be a lot more cost efficient to just store the original values (your mocks) and then overwrite them again after installing lolex. That would be two lines of code. Given that very, very few people are likely to need any such option, I would just do this, if I were you.

[itayperry]

Hi @fatso83, thank you for the elaborated and enlightening reply! So, basically.. if we just take and change Performance.now() and take all the other values as they are (instead of replacing them with no-ops) that would be inefficient/unnecessary?

[Smrtnyk]

@fatso83 makes sense, I will remock my performance implementation in the jsdom after clock install as it is not something big of a deal. You can close this ticket here without any changes if you find it suitable.

[fatso83]

@Smrtnyk 👍

if we just take and change Performance.now() and take all the other values as they are (instead of replacing them with no-ops) that would be inefficient/unnecessary?

@itayperry I think you are suggesting to just leave them be, right, if they exist? Not sure why that would be inefficient. Seems like less work to me. TBH, if the functions are there and we do not replace them with something meaningful, I am not sure why we do it at all. Your guess is as good as mine and some quick git blame did not make me smarter as to why 🤷

[itayperry]

Then let's do it maybe? Leave them be and replace all the noops with the original values? I would have to delete the should replace the getEntries, getEntriesByX methods with noops that return [] tests and basically, PR #240 would become irrelevant.

I could do that:

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

        Object.getOwnPropertyNames(proto).forEach(function (name) {
                clock.performance[name] = _global.performance[name];
        });
 }

It would only break one test (npm run test-headless) :

https://github.com/sinonjs/fake-timers/blob/ce85feaadc34a354488392b21757d7f09a973e89/test/fake-timers-test.js#L3721-L3729

Do you think that maybe specifically the mark() should stay a noop? I did a console.log and it seems like the mark() exists but I don't understand why it says "Illegal invocation".

[fatso83]

We no longer support Safari 9, so any code targetting that could be removed. I am not sure about the illegal invocation thing, but it might be a thing native functions have some expectations of where they are being called. So when no longer present on the native Performance.prototype, they crap out when "thing x" is not present or something like that.

If calling native implementations on another object is illegal is the case, then we can't do the naive copy-thing.

Anyway, I am thinking we go about this the wrong way. Right now, all the suggestions is basically just about not stubbing the performance timers, right? If that's the case, why not just add a possibility to not stub them? As in making it possible to add "performance" to the config.toFake options array? No changes needed anywhere else.

[itayperry]

it might be a thing native functions have some expectations of where they are being called

this blows my mind lol

As in making it possible to add "performance" to the config.toFake options array? No changes needed anywhere

Ohhh I see, that sounds very neat!

So basically, I'm gonna have to move all the performance object code to the install function and call it only if it's in the toFake array?

As in moving this

https://github.com/sinonjs/fake-timers/blob/ce85feaadc34a354488392b21757d7f09a973e89/src/fake-timers-src.js#L1526-L1547

in here? :

https://github.com/sinonjs/fake-timers/blob/ce85feaadc34a354488392b21757d7f09a973e89/src/fake-timers-src.js#L1616-L1635

[fatso83]

Yes, that sounds about right.

it might be a thing native functions have some expectations of where they are being called

this blows my mind lol

Not so strange, though, if comparing to normal JS. You can't just store a method reference and expect it to work when called outside of its original context, unless you explicitly bind it, a la const myMethod = method.bind(originalObject), as all internal references to this would be lost. So, quite possible that something similar can happen for other objects and their methods.

[itayperry]

So, quite possible that something similar can happen for other objects and their methods.

Thank you!

About adding performance to the toFake array: I've been trying to move this code around.. https://github.com/sinonjs/fake-timers/blob/ce85feaadc34a354488392b21757d7f09a973e89/src/fake-timers-src.js#L1526-L1547 but I don't know how to deal with the fact that hrTime() function would be undefined. Do you have any idea how I could solve it? Generally speaking, when a user chooses not to fake performance then clock.performance.now shouldn't be created?

If I add a test that adds performance to the toFake array test-headless passes and test-node passes as long as I do it inside this condition: https://github.com/sinonjs/fake-timers/blob/ce85feaadc34a354488392b21757d7f09a973e89/test/fake-timers-test.js#L3703 which seems rather promising :)

One more thing I found out - there are a few tests that actually add performance to the toFake array, I guess I just didn't notice it, and honestly, I can't really seem to understand how they work exactly.

https://github.com/sinonjs/fake-timers/blob/ce85feaadc34a354488392b21757d7f09a973e89/integration-test/fake-clock-integration-test.js#L39-L58

The thing is - if I we let users use toFake: ["performance"] should we throw an error when performanceNowPresent is false?

[fatso83]

I'll start with the last question, since that was the strangest one. performanceNowPresent is about feature detection in the runtime in the test suite. It has nothing to do with what users do or not. Therefore I do not understand how the question makes sense 😄

If what you really mean is the performancePresent constant, then that is something else, as that is a flag in the fake timers code itself. We have previously followed the guideline of "fail early" and not "ignore in silence" and I think that holds here as well. If users are asking us to fake an API the runtime does not support, then I think we should notify them it does not make sense by throwing.

but I don't know how to deal with the fact that hrTime() function would be undefined.

I have no idea what you mean here either :-| The only hrtime() functions I know of are the ones in process and the one we define ourselves at line 995. The latter is always defined and is not affected by this at all.

Generally speaking, when a user chooses not to fake performance then clock.performance.now shouldn't be created?

Seems right.

there are a few tests that actually add performance to the toFake array

I cannot see how this is correct. I have searched all of fake-timers-test.js for toFake and none of them has a reference to "performance". The closest is "hrtime". You dumped a link to some tests in fake-clock-integration-test.js, but those tests were quite unrelated to what you were writing about, so not sure what that was about ... ?

Maybe what you think about is this line:

     var clock = withGlobal.install({ toFake: timers }); 

When you inspected that in the debugger you might have seen something about "performance" in that array. No one has explicitly put that in there. If you look a few lines further up you can see on line 31-32 what is going:

        withGlobal = FakeTimers.withGlobal(jsdomGlobal);
        timers = Object.keys(withGlobal.timers);

Here, they install fake timers on the jsDomGlobal (instead of the normal global). Then they do a "shorthand" to see what was actually installed (and thus available) by listing out the keys of those installed timers. So if some of the performance timers were available in JSDOM, then they will of course be part of that array, even if we don't support using them as input later on (we also don't actually do anything with them).

[itayperry]

Thank you so much for taking the time to reply so extensively - there are clearly a lot more things that I should learn about the project, I do not wish to burden members and to be too much in need of hand-holding (although it's exactly what I'm doing, I'm afraid). TBH - understanding most of the code (or what's happening in general) is way harder than I thought - but I still wanna try and resolve this (and many other issues) 🙈

You dumped a link to some tests... When you inspected that in the debugger you might have seen something about "performance" in that array

That's exactly what happened.

I think that holds here as well. If users are asking us to fake an API the runtime does not support, then I think we should notify them it does not make sense by throwing.

Thanks!

The only hrtime() functions I know of are the ones in process and the one we define ourselves at line 995. The latter is always defined and is not affected by this at all.

About the hrtime(): It is defined inside createClock() but now that performance can be added to the toFake array I wanna move logic to the install() function - there, hrtime() is undefined but clock.performance.now requires it.

[fatso83]

You have to start somewhere 🙂 I haven't written more than a tiny subset, mostly bug fixes, and I need to reacquaint myself every time. Luckily, it's relatively little code and pretty straightforward. You just need a lot of jumping back and forth 😉

I don't understand the need to move hrtime anywhere. It's used to setup the clock and the toFake array is separate from this. It's all about this tiny for loop: https://github.com/sinonjs/fake-timers/blob/ce85feaadc34a354488392b21757d7f09a973e89/src/fake-timers-src.js#L1616

[itayperry]

That's part of the code I wanted to move into the loop:

https://github.com/sinonjs/fake-timers/blob/ce85feaadc34a354488392b21757d7f09a973e89/src/fake-timers-src.js#L1542-L1546

But from what I understand this (specifically) shouldn't be moved? btw - const hrt = hrtime(); is what drove me crazy lol

[fatso83]

I do not understand why it needs to be moved. Especially into the loop. We have to distinct phases

1. Creation of the clock. Here we create timers that might or might not be used.
2. Hijacking of the various timer methods. Here you check which timers are available and only install the ones that are.

So your only changes would need be in the loop (or after it), AFAIK. Just check if toFake contains "performance"` and hijack all the methods when it is present.

[itayperry]

1. Creation of the clock. Here we create timers that might or might not be used.

2. Hijacking of the various timer methods. Here you check which timers are available and only install the ones that are.

Thank you :)

That's the change in createClock() function (clock.performance.now stays):

        if (performancePresent) {
            clock.performance = Object.create(null);
            clock.performance.now = function FakeTimersNow() {
                var hrt = hrtime();
                var millis = hrt[0] * 1000 + hrt[1] / 1e6;
                return millis;
            };
        }

that's the addition to the loop in the install() function:

                if (
                    clock.methods[i] === "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;
                        }
                    });
                }
                hijackMethod(_global, clock.methods[i], clock);

All tests pass of course because everything is the same. Does this make sense?

EDIT: code is now colored 🎉

[fatso83]

I think this seems to make sense. One could argue whether it makes sense to have a field called methods on Clock, if it actually does not contain methods, but more like an array of "mockables" that might or might not be timers and just as well might refer to an object in the global scope.

To make the logic a bit clearer, I think I would move all of that outside the loop and just do something like:

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

Remember to add a test for your changes.

P.S. What is the if (name !== "now") { guard about? What do you do about it? Is it handled some other place? And is it really on the prototype or a static?

[itayperry]

I didn't know I could color my code (like in Stack Overflow), thank you for showing me that this feature exists!!

One could argue whether it makes sense to have a field called methods on Clock, if it actually does not contain methods, but more like an array of "mockables" that might or might not be timers and just as well might refer to an object in the global scope.

I thought about it too! Perhaps I should change the name someday?

P.S. What is the if (name !== "now") { guard about? What do you do about it? Is it handled some other place? And is it really on the prototype or a static?

If you're asking that it probably means I still didn't do it right.. and that I don't understand exactly what's happening. I saw that if I don't exclude now it overrides this code (inside createClock()):

clock.performance.now = function FakeTimersNow() {
    var hrt = hrtime();
    var millis = hrt[0] * 1000 + hrt[1] / 1e6;
    return millis;
};

and breaks many tests. It isn't handled in some other place I'm afraid. I'll keep working on it.

BTW - Thank you a million times for your undying patience and extremely elaborated replies - I don't take that for granted :)

[itayperry]

ohhh FakeTimers.install({ toFake: anything }); should always still replace global performance.now right? In that case, no wonder all tests pass lol I never checked what happens to performance.now when a user does not include performance in the toFake array but performanceNowPresent is true. Did I finally get it now?

[fatso83]

I don't have the answers in my head, so I needed to examine the code to answer you 😄. That's why it took some time.

ohhh FakeTimers.install({ toFake: anything }); should always still replace global performance.now right?

As you see in the code, when toFake is empty, the default is to add all timers known to us, by looping through the timers field: https://github.com/sinonjs/fake-timers/blob/ce85feaadc34a354488392b21757d7f09a973e89/src/fake-timers-src.js#L1598 Among which are performance: https://github.com/sinonjs/fake-timers/blob/ce85feaadc34a354488392b21757d7f09a973e89/src/fake-timers-src.js#L936

So yes, we should mock performance by default - like all other timers.

Did I finally get it now?

I don't know 😄 The code does not mock performance if you leave it out from config.toFake. Was not sure what you thought about that.

Just generally, if you wonder about something, ask the code. That's what I do when you ask ;) Just write a little driver to verify current behavior:

const lolex = require("@sinonjs/fake-timers");
const origPerfNow = performance.now;

function isSame(){
console.log(
  "performance.now the same after `install()`?",
  origPerfNow === performance.now
); 
}

console.log(performance.now()); // 43.575292110443115
const clock = lolex.install();
console.log(performance.now()); // 0

isSame(); // false

clock.uninstall();
lolex.install({toFake: ["setTimeout"]})

isSame(); // true

[itayperry]

Thank you @fatso83 :)

Just write a little driver...

A good time to ask you - how do you use your local copy of fake-timers? I usually start a new Node Server project and use npm-link or currently just a React project with a package that allows imports outside of src.

I checked it now as well, and I can see that performance.now isn't being hijacked if not in the toFake array (and the toFake array isn't empty). So thank you for your help!

BTW, did you see my previous comment? I put the if (name !== "now") {} guard because I didn't want this code to be overwritten:

clock.performance.now = function FakeTimersNow() {
    var hrt = hrtime();
    var millis = hrt[0] * 1000 + hrt[1] / 1e6;
    return millis;
};

and then the performance.now related tests won't fail.

I thought about it and the changes I did in the code didn't really change much.. all tests pass and everything is almost exactly the same.

As in making it possible to add "performance" to the config.toFake options array

Perhaps it's already possible? I just tried it in a React project and it worked. Maybe I just need to make a test that throws an error if someone puts it in the toFake array but performance isn't present? Other than that I don't really know what else is necessary.

I understand that this should stay because if we don't hijack then there's no point for this to run:

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

What are your thoughts about this?

[fatso83]

Perhaps it's already possible

Yes, it is