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.

Sign up for GitHub

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

Jump to bottom

Downgrade Jest to 24 #18376

Merged
merged 1 commit into from
Mar 24, 2020
Merged

Downgrade Jest to 24 #18376

merged 1 commit into from
Mar 24, 2020

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Mar 24, 2020

There's a bug where toEqual throws when comparing readonly properties, which affects our JSX matchers.

jestjs/jest#9575

The bug is fixed in master, but not yet released.

Until then, downgrading to 24.

@acdlite acdlite requested a review from gaearon March 24, 2020 17:05
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Mar 24, 2020
@gaearon
Copy link
Collaborator

gaearon commented Mar 24, 2020

Not sure it's going to be that easy due to jsdom changes. The upgrade was rather involved: #17896.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 24, 2020 •
edited
Loading

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit a622dad:

Sandbox Source
objective-star-qctjz Configuration

@acdlite
Copy link
Collaborator Author

acdlite commented Mar 24, 2020

Wonderful.

@acdlite
Copy link
Collaborator Author

acdlite commented Mar 24, 2020

@SimenB Do you mind doing a patch release for jestjs/jest#9575?

@sizebot
Copy link

sizebot commented Mar 24, 2020 •
edited
Loading

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against a622dad

@sizebot
Copy link

sizebot commented Mar 24, 2020 •
edited
Loading

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against a622dad

@acdlite
Revert "Upgrade to jest 25 (facebook#17896)"
a622dad 
This reverts commit cf00812.

The changes to the test code relate to changes in JSDOM that come with Jest 25:

* Several JSDOM workarounds are no longer needed.
* Several tests made assertions to match incorrect JSDOM behavior (e.g. setAttribute calls) that JSDOM has now patched to match browsers.
  * https://codesandbox.io/s/resets-value-of-datetime-input-to-fix-bugs-in-ios-safari-1ppwh
* JSDOM no longer triggers default actions when dispatching click events.
  * https://codesandbox.io/s/beautiful-cdn-ugn8f
* JSDOM fixed (jsdom/jsdom#2700) a bug so that calling focus() on an already focused element does not dispatch a FocusEvent.
* JSDOM now supports passive events.
* JSDOM has improved support for custom CSS properties.
  * But requires jsdom/cssstyle#112 to land to support webkit prefixed properties.
@acdlite
Copy link
Collaborator Author

acdlite commented Mar 24, 2020

Fortunately cf00812 reverts pretty cleanly. I thought it was a big diff but it's mostly only yarn.lock changes.

@acdlite acdlite mentioned this pull request Mar 24, 2020
Merged
@sebmarkbage
Copy link
Collaborator

sebmarkbage commented Mar 24, 2020 •
edited
Loading

Did you test if the bugfix actually fixes all the things? The fix doesn't seem fully correct to me. E.g. what happens if we implement a setter that throws instead of freezing. Which we do sometimes.

@acdlite
Copy link
Collaborator Author

acdlite commented Mar 24, 2020

It fixes comparing JSX. I tested with toMatchRenderedOutput. What are the other things?

@acdlite
Copy link
Collaborator Author

acdlite commented Mar 24, 2020

Oh you mean the fix in master, not the revert. I didn't test that, no.

@sebmarkbage
Copy link
Collaborator

Comparing SyntheticEvent might fail.

react/packages/legacy-events/SyntheticEvent.js

Line 271 in 30a998d

warn(action, 'This is effectively a no-op');

@acdlite acdlite merged commit fc7835c into facebook:master Mar 24, 2020
@SimenB
Copy link
Contributor

SimenB commented Mar 24, 2020

@SimenB Do you mind doing a patch release for jestjs/jest#9575?

I don't have publish access, only FB employees do. You can try to ping @scotthovestadt or @DiZy internally? Or @cpojer might be able to

@gaearon
Copy link
Collaborator

gaearon commented Mar 25, 2020

Sorry for inconvenience @SimenB, I just followed up with our team and you should have access to all packages now.

@SimenB
Copy link
Contributor

SimenB commented Mar 25, 2020

Cool, thanks! I've published 25.2.0 now.

Did you test if the bugfix actually fixes all the things? The fix doesn't seem fully correct to me. E.g. what happens if we implement a setter that throws instead of freezing. Which we do sometimes.

This probably isn't fixed though, maybe open up an issue with a test that fails which should pass?

@gaearon
Copy link
Collaborator

gaearon commented Apr 2, 2020

Current blocker: jestjs/jest#9745

@SimenB
Copy link
Contributor

SimenB commented Apr 3, 2020

25.2.7 published with fix for ^. Thanks for the help landing it @gaearon!

gaearon added a commit to gaearon/react that referenced this pull request Apr 3, 2020
@gaearon
Revert "Revert "Upgrade to jest 25 (facebook#17896)" (facebook#18376)"
ea41676 
This reverts commit fc7835c.
@gaearon gaearon mentioned this pull request Apr 3, 2020
Merged
gaearon added a commit that referenced this pull request Apr 3, 2020
@gaearon
Update to Jest 25 (#18480)
a8f2165 
* Revert "Revert "Upgrade to jest 25 (#17896)" (#18376)"

This reverts commit fc7835c.

* Other fixes

* Fix a broken test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gaearon gaearon gaearon approved these changes

@bvaughn bvaughn bvaughn approved these changes

Assignees
No one assigned
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@acdlite @gaearon @sizebot @sebmarkbage @SimenB @bvaughn @facebook-github-bot