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.

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

window.performance.mark becomes undefined after timer install #136

Closed
oliviertassinari opened this issue Oct 28, 2017 · 25 comments
Closed
Assignees
Labels

Comments

@oliviertassinari
Copy link

oliviertassinari commented Oct 28, 2017

I have noticed the following behavior in the browser:

import { useFakeTimers } from 'sinon';

console.log('1', window.performance.mark) // '1', function mark() { ... }
clock = useFakeTimers();
console.log('2', window.performance.mark) // '2', undefined

window.performance became a function after calling useFakeTimers. I believe that the issue is coming from this line:
https://github.com/sinonjs/lolex/blob/c08ff362a47e9786bc62da6c37175838bb7bd21e/src/lolex-src.js#L406-L408

One simple workaround is the following:

-clock = useFakeTimers();
+clock = useFakeTimers({
+  toFake: ["setTimeout", "clearTimeout", "setImmediate", "clearImmediate","setInterval", + "clearInterval", "Date"]
+});
@oliviertassinari oliviertassinari changed the title performance issue performance API issue Oct 28, 2017
@oliviertassinari
Copy link
Author

I had to do some digging, the error on my side was coming from this line: https://github.com/facebook/react/blob/v16.0.0/src/renderers/shared/fiber/ReactDebugFiberPerf.js#L81.

@fatso83
Copy link
Contributor

fatso83 commented Oct 30, 2017

Verified. Here is the test with a browserified transpiled output that can be copy-pasted directly into Chrome for verification. Tested with master (6476935).

test.js.txt
transpiled.js.txt

@fatso83 fatso83 added the bug label Oct 30, 2017
@fatso83 fatso83 changed the title performance API issue window.performance.mark becomes undefined after timer install Nov 7, 2017
@benjamingr
Copy link
Member

@oliviertassinari wanna take a shot at a PR at this? Otherwise I can probably take a stab

@oliviertassinari
Copy link
Author

@benjamingr I'm already pretty busy with Material-UI. Sorry, it's not a priority for me.

@benjamingr
Copy link
Member

@oliviertassinari no need to justify or apologize - it's entirely fine if it's not a priority for you - I was merely offering :)

@benjamingr
Copy link
Member

@fatso83 I still can't assign myself to issues by the way :D I'll take a stab next week hopefully

@medialwerk
Copy link

Is there anyone working at this issue at the moment? :-)

@benjamingr
Copy link
Member

sorry, completely forgot about it - can take a look early next week

@medialwerk
Copy link

Thank you very much!

@risinglf
Copy link

risinglf commented Jan 9, 2018

Hi guys, news about this issue? thanks

@fatso83
Copy link
Contributor

fatso83 commented Jan 9, 2018

@risinglf Feel free to have a go. It's a very minor fix that you can probably make a PR for in 30 mins.

@remojansen
Copy link

As a temporary solution, I used the following function:

function useFakeTimers() {
    const timer = sinon.useFakeTimers();
    performance.mark = () => void 0;
    performance.clearMarks = () => void 0;
    performance.measure = () => void 0;
    performance.clearMeasures = () => void 0;
}

const timer = useFakeTimers();

It works with:

  • react@16.2.0
  • sinon@4.2.0
  • enzyme-adapter-react-16@1.1.1
  • enzyme@3.3.0

@austintackaberry
Copy link

This temporary solution did not work for me using:

  • react@16.2.0
  • sinon@4.2.2
  • enzyme-adapter-react-16@1.1.1
  • enzyme@3.3.0

However, @oliviertassinari's workaround did work for me.

@benjamingr
Copy link
Member

Going to tackle this now

@benjamingr
Copy link
Member

I added a test case as:

describe("issue #136", function () {
    if (performancePresent) {
        it("should clear performance.mark after uninstall", function () {
            var oldMark = performance.mark;
            var clock = lolex.install();
            assert.same(performance.mark, oldMark);
            clock.uninstall();
            assert.same(performance.mark, oldMark);
        });
    }
});

but it passed without any changes - what am I not understanding?

@tarjei
Copy link

tarjei commented Apr 3, 2018

Hi, any news on this issue?

@tarjei
Copy link

tarjei commented Apr 3, 2018

This is what I'm seeing:

In my test:

beforeEach(function() {
    console.log('pm0:', performance.mark)
    this.clock = sinon.useFakeTimers()
    console.log('pm1:', performance.mark)
  })
  afterEach(function() {
    console.log('pm2:', performance.mark)
    this.clock.restore()
    console.log('pm3:', performance.mark)
  })

Output:

LOG: 'pm0:', function mark() { ... }
LOG: 'pm1:', undefined
LOG: undefined
  Note
    ✖ should run updates every 100 seconds. 
LOG: 'pm2:', undefined
LOG: 'pm3:', function mark() { ... }
LOG: 'pm0:', function mark() { ... }
LOG: 'pm1:', undefined
    ✖ should clear the contentQueue on unmount
LOG: 'pm2:', undefined
LOG: 'pm3:', function mark() { ... }

So sinon is setting performance.mark to undefined for some reason.

@tarjei
Copy link

tarjei commented Apr 4, 2018

So sinon is setting performance.mark to undefined for some reason.

So maybe the testcase should look like this:

describe("issue #136", function () {
    if (performancePresent) {
        it("should clear performance.mark after uninstall", function () {
            var oldMark = performance.mark;
            var clock = sinon.useFakeTimers();
            assert.same(performance.mark, oldMark);
            assert.equals(typeOf performance.mark , 'function'); // I do not know this assert api, but you get the gist
    
            clock.uninstall();
            assert.same(performance.mark, oldMark);
        });
    }
});

@benjamingr
Copy link
Member

@tarjei are you able to reproduce it with lolex alone?

@tarjei
Copy link

tarjei commented Apr 4, 2018

@benjamingr I didn't try as it seemed to me to be related to sinon - esp since your testcase works.

How do I run the tests with performancePresent btw? It seems that the performance object is missing when I run npm test.

@CoderK
Copy link
Contributor

CoderK commented Apr 5, 2018

I think I know what the cause is. May I have fix this issue?

@tarjei
Copy link

tarjei commented Apr 5, 2018

@CoderK I suspect that a PR would be most welcome :) 👍

@CoderK
Copy link
Contributor

CoderK commented Apr 5, 2018

Ok, I’m gonna make a patch soon!

@CoderK
Copy link
Contributor

CoderK commented Apr 6, 2018

I created pull request, #160. If there is someone who has admin permissions, please review my PR.

@CurtisHumphrey
Copy link
Contributor

Until #160 is approved, I just did this before starting any tests

import lolex from 'lolex'
const org_install = lolex.install
lolex.install = () => {
  return org_install({
    toFake: ['setTimeout', 'clearTimeout', 'setImmediate', 'clearImmediate', 'setInterval', 'clearInterval', 'Date'],
  })
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

10 participants