Skip to content

add simple integration test with jsdom#169

Merged
fatso83 merged 2 commits intosinonjs:masterfrom
SimenB:integration-jsdom
May 13, 2018
Merged

add simple integration test with jsdom#169
fatso83 merged 2 commits intosinonjs:masterfrom
SimenB:integration-jsdom

Conversation

@SimenB
Copy link
Copy Markdown
Member

@SimenB SimenB commented May 8, 2018

Includes both #162 and #168, plus an integration test using JSDOM.

This doesn't really work though, as window.performance is readonly in JSDOM. This matches the spec (or, at least it matches the text on MDN), but it does not match the behavior of Chrome.

The second commit skips faking performance, and the test passes. I don't like that, though.

window.performance is readonly, so instead of using simple assignment, this PR uses Object.defineProperty.

@SimenB
Copy link
Copy Markdown
Member Author

SimenB commented May 8, 2018

I can look into having mochify skip the test same as node 4 if you want to keep the test.

@SimenB
Copy link
Copy Markdown
Member Author

SimenB commented May 8, 2018

window.performance might have to be copied more cleanly, like Date is: https://github.com/sinonjs/lolex/blob/8966fde17a402f1fc88c05ac3398303c53cd7882/src/lolex-src.js#L121

@mantoni
Copy link
Copy Markdown
Member

mantoni commented May 9, 2018

See my change here for the window.performace issue: https://github.com/sinonjs/lolex/pull/162/files

@SimenB
Copy link
Copy Markdown
Member Author

SimenB commented May 9, 2018

@mantoni Yes, that part is included in the filter function I added.

The issue now (at least in this PR) is that window.performance is readonly, so hijackMethod explodes. Unless I'm misunderstanding something

@mantoni
Copy link
Copy Markdown
Member

mantoni commented May 9, 2018

A sorry. The silly app I was viewing this in shows it as an issue instead of a PR 🙄

@SimenB SimenB force-pushed the integration-jsdom branch 5 times, most recently from 460795c to 89c4524 Compare May 9, 2018 06:35
@SimenB
Copy link
Copy Markdown
Member Author

SimenB commented May 9, 2018

@fatso83 any way of triggering the integration tests from a PR? I wonder if the last commit fixes the issue

@SimenB SimenB force-pushed the integration-jsdom branch from 89c4524 to e2d76c4 Compare May 9, 2018 07:00
@fatso83
Copy link
Copy Markdown
Contributor

fatso83 commented May 11, 2018

@SimenB I don't think we have a way of triggering the full suite of Saucelabs tests on the PRs. It's been a long time, but I think the reason why we chose not to do it was that it would open us up to exposing the secret key (or something like that) by aggressors adding PRs that would read out environment variables. Or something like that ... @mantoni might remember it better 😊

Anyway, I see you have your email listed on your profile, so I'll send you what you need so you can do it. In other news ™️ the last commit failed on PhantomJS.

@mantoni
Copy link
Copy Markdown
Member

mantoni commented May 11, 2018

This is a Travis security restriction, unfortunately.

I have a general question regarding this RP: Wouldn't switching to the latest Mochify with headless Chrome make a JSDom test superfluous? Or is JSDom something we want to have explicitly tested?

@fatso83
Copy link
Copy Markdown
Contributor

fatso83 commented May 11, 2018

@mantoni Given the current work that is done on Jest to try and integrate lolex, which would touch a lot of users, I think it makes sense to support such a big target.

@SimenB SimenB force-pushed the integration-jsdom branch from e2d76c4 to 354c389 Compare May 12, 2018 04:32
@SimenB
Copy link
Copy Markdown
Member Author

SimenB commented May 12, 2018

@fatso83 @benjamingr this now passes CI, and npm run test-cloud 🙂

@SimenB SimenB force-pushed the integration-jsdom branch from 0d48dea to b8b1b50 Compare May 12, 2018 09:39
@mantoni
Copy link
Copy Markdown
Member

mantoni commented May 12, 2018

👍 Looks good to me.

@@ -0,0 +1,57 @@
/**
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for forgetting this, but could you just skip the comment here about copyright and all that?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@SimenB SimenB force-pushed the integration-jsdom branch from b8b1b50 to 8ec6170 Compare May 12, 2018 10:20
@fatso83 fatso83 merged commit 1bf1d09 into sinonjs:master May 13, 2018
@SimenB SimenB deleted the integration-jsdom branch May 13, 2018 08:06
@SimenB
Copy link
Copy Markdown
Member Author

SimenB commented May 13, 2018

A release of this would be awesome 😀

fatso83 added a commit that referenced this pull request Nov 9, 2021
Saw this was missing when reviewing DefinitelyTyped/DefinitelyTyped#56961

Bit funny that it has not been added until now, given it was added way back in #106 😄 (And later #169 #248 and others).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants