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

fix styles not being added when using toHaveStyle() #3308

Merged
merged 1 commit into from Oct 30, 2020

Conversation

Lazyuki
Copy link
Member

@Lazyuki Lazyuki commented Oct 13, 2020

Fixes: #3297

I checked out every commit to find what caused this, and it seems that this commit and this commit were the culprit. It seems like having window as either false or undefined makes jest-dom's toHaveStyle function fail by not rendering any styles at all. I imagine this has something to do with how jest interacts with the window object.

@probablyup I don't exactly know the reason behind these commits so I hope this doesn't break things elsewhere. I tested the build locally and I can confirm that this will fix toHaveStyle

@Lazyuki
Copy link
Member Author

Lazyuki commented Oct 30, 2020

Hi @probablyup @kitten sorry to bother you but this is preventing us and I believe quite a few people from upgrading to 5.2.0 since the tests cannot be run with it.

@probablyup
Copy link
Contributor

@Lazyuki you'll need to switch your base branch to legacy-v5

@Lazyuki Lazyuki changed the base branch from master to legacy-v5 Oct 30, 2020
@Lazyuki
Copy link
Member Author

Lazyuki commented Oct 30, 2020

Done.

Copy link
Contributor

@probablyup probablyup left a comment

Choose a reason for hiding this comment

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

Looking at the code I don't think this will break anything, so long as people don't have a window object in their SSR environment that has an HTMLElement property. That's the heuristic we use for our IS_BROWSER check.

@christemple
Copy link

Looking at the code I don't think this will break anything, so long as people don't have a window object in their SSR environment that has an HTMLElement property. That's the heuristic we use for our IS_BROWSER check.

I might be stupid here, but doesn't JSDOM replicate the window in a node environment for jest tests by default?
I just added a console log to my Jest config and it says it does exist:

console.log('HTMLElement in window: ', 'HTMLElement' in window);
=> HTMLElement in window:  true

Is this unwanted behaviour @probablyup?

Only reason I'm asking is because my tests are now failing as a result of this change (v5.2.1) using react testing library + jest

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.

Breaks toHaveStyle assertions in v5.2.0
3 participants