Breaks on Node 16

[hexeberlin]

Currently, the tests are failing in node 16. Figure out why, fix it, and adjust GitHub actions workflow to use node 16 instead of 10.

[fatso83]

@mroderick Thoughts on this (ref 0b1834a)? Just came across this. Might tangentially touch work currently done by @itayperry .

$ npm run test-node
...
  3 failing

  1) FakeTimers
       stubTimers
         should let performance.mark still be callable after FakeTimers.install() (#136):
     AssertionError: [refute.exception] Expected not to throw but threw TypeError (global.performance.mark is not a function)

What happens is that the install() call somehow messes up performance.mark. Not sure why that should be different on Node 16, but @benjamingr is quite close on development happening in that space, I believe.

[benjamingr]

Node.js 16 has global performance so it makes sense it's behaving differently than older versions here.

[fatso83]

What does that mean? Does it means it is not present on global anymore? We detect its presence before installing the timers on Node 16 as well, so not really getting it.

[mroderick]

In node 16

> typeof Performance
'undefined'
> typeof global.performance
'object'
> typeof performance
'object'

[mroderick]

@mroderick Thoughts on this (ref 0b1834a)?

@hexeberlin and I made all the sinon repositories use GitHub Actions in the same day. In the parent commit b3557ef, you can see this. Unfortunately, we couldn't run the fake-timers tests in Node 16, so we settled for 10-14 and filed a bug to fix it later.

[itayperry]

This returns true today when running the test that @fatso83 has mentioned:
I guess this is what's causing the problem - it is expected to be true in order to stub performance:

fake-timers/src/fake-timers-src.js

Lines 154 to 156 in 93f3f62

	const hasPerformancePrototype =
	_global.Performance &&
	(typeof _global.Performance).match(/^(function|object)$/);

typeof Performance
'undefined'

Was it available on node 14 and deleted on node 16...? (I don't really get it).
@benjamingr

[fatso83]

@itayperry The stuff you did in #400 seemed to touch upon this. Want to merge that before we look more into this?

[synaestheory]

This returns true today when running the test that @fatso83 has mentioned: I guess this is what's causing the problem - it is expected to be true in order to stub performance:

fake-timers/src/fake-timers-src.js

Lines 154 to 156 in 93f3f62

	const hasPerformancePrototype =
	_global.Performance &&
	(typeof _global.Performance).match(/^(function|object)$/);

typeof Performance
'undefined'

Was it available on node 14 and deleted on node 16...? (I don't really get it). @benjamingr

The major difference between node 14 and node 16 appears to be the introduction of global.performance in v16.

In v14, performancePresent resolves to undefined but in v16 it resolves to true.

One other delta that I noticed while stepping through the code was that hijackMethod is also called for global.performance in v16, where it is not in v14. I'm not suggesting that this is correct/incorrect behavior, just noting it as a delta.

EDIT:
Adding some additional interesting notes that could help to guide a potential solution. I have a strong suspicion that this related to the properties on the global performance object being lost. performance.mark exists in node 16, as does performance.now:

Welcome to Node.js v16.13.0.
> performance.mark
[Function: mark]
> performance.now
[Function: now]

However, neither of them are enumerable/writeable, and the performance member itself on global is enumerable:

> Object.getOwnPropertyDescriptor(performance, 'now')
undefined
> Object.getOwnPropertyDescriptor(performance, 'mark')
undefined
> Object.getOwnPropertyDescriptor(global, 'performance')
{
  value: Performance {
    nodeTiming: PerformanceNodeTiming {
     // trimmed...
    },
    timeOrigin: 1635973273676.253
  },
  writable: true,
  enumerable: true,
  configurable: true
}

When hijackMethod executes on node16 with the value "performance" for the method argument, this is the branch of code that gets executed (originalPerfDescriptor exists but originalPerfDescriptor.get does not):
https://github.com/sinonjs/fake-timers/blob/master/src/fake-timers-src.js#L927-L929
This results in the global.performance object only having the stubbed now method and hadOwnProperty = true.
With global.performance.mark and global.performance.now being non-enumerable, they are not restored to the global.performance member when uninstall is called.

I hope this helps to debug the issue.

[itayperry]

@fatso83

Want to merge that before we look more into this?

Yes, of course! I'd be happy to do it :) I'll speed it up.

[fatso83]

@itayperry Any chances of you looking into this? Seems you already did all the work 😄

[itayperry]

@fatso83
On it :) I'll probably ask for some advice, I remember there were also issues with performance.mark.
I'm looking into this 👾

[itayperry]

Hi, I don't know if I'm doing this right.. but I'm using nvm (and also fnm), and when I try nvm use 14 or nvm use 15 everything works - when I try nvm use 16 I don't get 3 failed tests but 67 failed tests.

npm run test-node:

I guess I'm doing something wrong.

Okay, update (Jan 25th): this issue was opened before the latest merged PR - #400 (which is mine I'm afraid) - it's just a small condition that caused the other 64 to fail, not a problem, I'll fix it :)

fake-timers/src/fake-timers-src.js

Line 1675 in 1b1ac4a

} else if (config.toFake.includes("performance")) {

There are truly only 3 failed tests on Node 16. I'm working on it :)

[itayperry]

About these 2 tests:
a)

fake-timers/test/fake-timers-test.js

Line 3758 in 1b1ac4a

it("should not alter the global performance properties and methods", function () {

b)

fake-timers/test/fake-timers-test.js

Line 3778 in 1b1ac4a

it("should replace the getEntries, getEntriesByX methods with noops that return []", function () {

They both use Performance (to be more accurate Performance.prototype) which does not exist in Node 16+.
I did a small research and found this class called: InternalPerformance in Node's source code:

InternalPerformance.prototype.constructor = Performance.prototype.constructor;
ObjectSetPrototypeOf(InternalPerformance.prototype, Performance.prototype);

https://github.com/nodejs/node/blob/e2e2bc83c01b19a6601a3018d1790fbd2d4fa7de/lib/internal/perf/performance.js#L103-L104

performance: new InternalPerformance()

https://github.com/nodejs/node/blob/e2e2bc83c01b19a6601a3018d1790fbd2d4fa7de/lib/perf_hooks.js#L36

but I'm afraid I can't use it / access it.. it would be nice if we could use InternalPerformance.prototype instead of Performance.prototype.

What do you think we should do with these specific tests? Can we just skip them when in Node env?

@fatso83 @benjamingr

[msluther]

I made a stupid workaround that happens to work in my particular use case if anyone else is hitting this issue and wants to give it a try. I basically just made my own global fake Performance class and proxied to performance. It's dirty and hacky but it allowed my tests to continue working and I'm not blocked on upgrading to node@16.

I've got this in my unit test setup that gets pulled in prior to anyone calling useFakeTimers

global.Performance = global.Performance || class Perf {
  clearMarks() { return global.performance.clearMarks(); }
  clearMeasures() { return global.performance.clearMeasures(); }
  clearResourceTimings() { return global.performance.clearResourceTimings(); }
  getEntries() { return global.performance.getEntries(); }
  getEntriesByName() { return global.performance.getEntriesByName(); }
  getEntriesByType() { return global.performance.getEntriesByType(); }
  mark() { return global.performance.mark(); }
  measure() { return global.performance.measure(); }
  now() { return global.performance.now(); }
  setResourceTimingBufferSize() { return global.performance.setResourceTimingBufferSize(); }
  toJSON() { return global.performance.toJSON(); }
};

[benjamingr]

Hey sorry this has been open for a while and I need this to work in my project so I'm going to go ahead and fix it - let me know if you want to sync on other stuff to work on.

[itayperry]

@benjamingr
It would take me much longer than it took you to resolve it, so I get it. It's ok 🌼
Plus, I didn't know if it were right to skip these 2 tests I was writing about..

Anyway, thank you for your help in #412!!

[benjamingr]

Can be closed :)

[msluther]

It looks like this will need to be bumped over in nise as well as I am still hitting this issue using fake timers via sinon. I'll open an issue there.

$ npm ls @sinonjs/fake-timers
my-library@0.2.0 /Users/me/my-library
└─┬ sinon@13.0.0
  ├── @sinonjs/fake-timers@9.0.0
  └─┬ nise@5.1.0
    └── @sinonjs/fake-timers@7.1.2

[msluther]

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]

@msluther to understand your use case better - you are using a library (e.g. for testing) that sets your global object to a JSDom window?

If so - I think it's probably reasonable to account for then yes - can you open a separate issue?

[msluther]

Yep, thats correct its basically this in my test setup: require('global-jsdom/register') and that ends up with the minimal version of global.Performance type, but leaves global.performance. I'll open another issue. Thanks