sinonjs / fake-timers

Fake setTimeout and friends (collectively known as "timers"). Useful in your JavaScript tests. Extracted from Sinon.JS
BSD 3-Clause "New" or "Revised" License
794 stars 103 forks source link

`performance.mark` doesn't exist in node@16 when using jsdom globals #420

Closed msluther closed 11 months ago

msluther commented 2 years ago

What did you expect to happen?

performance.mark is a function

What actually happens

performance.mark is undefined

How to reproduce

Minimal repro is available here: https://github.com/msluther/sinon-timers-jsdom-bug

Effectly though it's just this in node@16:

require('global-jsdom/register'); // This setups up jsdom and copies properties from `window` up to global, including a minimal Performance type
const sinon = require('sinon');
global.performance.mark(); // works
sinon.useFakeTimers();
global.performance.mark(); // throws as `mark` is now `undefined`

This is related to this issue: #394

From my comment on that issue:

Hrm. So, the fix works but makes the assumption that the functions on global.Performance.prototype are a superset of global.performance.* functions. This is currently not the case when using node@16 and jsdom. jsdom is injecting a global.Performance object based on the minimal W3C spec for hr-time.

This isn't necessarily a bug with either library, but feels like a gap between the two.

However, @sinon/fake-timers ends up replacing a good global.performance.mark with undefined in this situation. Should the code here use both prototypes to make sure this is the case or maybe prefer the global.performance over global.Performance?

The work around that I posted above still works in this case as long as its injected prior to sinon & jsdom, so I'm not blocked by any means, but just requires making sure that happens in my tests everywhere.

benjamingr commented 2 years ago

While it's not strictly a bug I think it's very reasonable to support this use case and the fix shouldn't be too hard.

zbyte64 commented 11 months ago

This is currently impacting me, but on node v18. What would an acceptable unit test be for this?

zbyte64 commented 11 months ago

From my own manual testing, swapping the order here: https://github.com/sinonjs/fake-timers/blob/b568150a78c41dc89c8fd73186dc8fa10b1b2c6e/src/fake-timers-src.js#L1755-L1760 is enough to resolve the issue for me anyways.

benjamingr commented 11 months ago

@zbyte64 feel free to open a PR

fatso83 commented 11 months ago

PR merged 🥳 Thanks a bunch