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

Update to Jest 25 #18480

Merged
merged 3 commits into from
Apr 3, 2020
Merged

Update to Jest 25 #18480

merged 3 commits into from
Apr 3, 2020

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Apr 3, 2020

Revert of #18376. Re-applies #17896.

Bumped the version to include fixes for jestjs/jest#9531 and the setter issue @sebmarkbage pointed out (jestjs/jest#9745).

Verified our matchers work now.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Apr 3, 2020
@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 3, 2020

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 9402084:

Sandbox Source
white-moon-4i9o5 Configuration

@sizebot
Copy link

sizebot commented Apr 3, 2020

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against 9402084

@sizebot
Copy link

sizebot commented Apr 3, 2020

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against 9402084

@sebmarkbage
Copy link
Collaborator

Can you explain the broken test fix? I don’t get it.

@gaearon
Copy link
Collaborator Author

gaearon commented Apr 3, 2020

It's complicated.

The test was originally written in #7397. For non-text input types, mobile browsers didn't honor setAttribute('value', ...) alone. So we need to assign .value first (and reset it) for later setAttribute('value', ...) to be respected. Note that the PR mentions defaultValue but that's a red herring. It was about defaultValue React prop but we were using setAttribute to implement it.

Then in #11534 we changed the implementation to actually use .defaultValue = instead of setAttribute. But we still needed this to happen after a .value assignment. Otherwise (presumably) it wouldn't be respected. I don't know if this was verified. This test (second one in fd69c23#diff-ec833522fe8ba426ff9bfe13e609a6e9) was missing a getter so it was causing some comparison to bail and reflected the actual order incorrectly. So the value mock in the test was fixed to store a value and return it. Although it was still kind of broken because it didn't proxy to the real jsdom implementation. In either case, the purpose of this test was to verify we set .value first and we setAttribute later. (I don't actually know why setAttribute gets called there, but I presume it's because setting .defaultValue triggerred it in jsdom, and we didn't have a separate .defaultValue mock).

Then in https://github.com/facebook/react/pull/17896/files#diff-ec833522fe8ba426ff9bfe13e609a6e9, we upgraded jsdom. That got rid of the bogus setAttribute logs. I presume it's because .defaultValue is no longer implemented in terms of setAttribute in jsdom. However, the test has also lost its purpose because it no longer verifies .value is assigned before .defaultValue. Because it doesn't mock .defaultValue at all. So the logs just disappeared.

Then the upgrade was reverted. As a part of https://github.com/facebook/react/pull/18376/files#diff-ec833522fe8ba426ff9bfe13e609a6e9 we temporarily added those bogus setAttribute assertions back, but then we skipped this test. I presume Andrew wasn't sure what's up with it.

Now I'm reverting Andrew's skipping. I've added .defaultValue mock to validate what the test was originally written to validate. That is, that we set the .value first. Again, I don't actually know if the original problem affected .defaultValue at all, or if it was confined to setAttribute. So maybe this isn't necessary at all.

After I unskipped and fixed it, I've noticed it fails with the disableAttributeSyncing flag. This is because I've made that flag run on CI in #18461. It didn't run before, but now it runs in variant tests. It failed because it was still asserting setAttribute gets called. But jsdom doesn't call setAttribute for .defaultValue assignments anymore. So I fixed the assertion there, too.

By the way, this bug got fixed in both Chrome and Safari in 2018 so it's possible we can remove this. #7233 (comment)

@gaearon gaearon merged commit a8f2165 into facebook:master Apr 3, 2020
@gaearon gaearon deleted the jest-rev branch April 3, 2020 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants