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

pass tests in ReactFinalForm.test.js #578

Closed

Conversation

threepointone
Copy link

DO NOT MERGE without reading.

This PR updates react to the new rc, and uses the new async act to silence a warning in ReactFinalForm.test.js

Further, regarding Field.test.js, I rewrote sleep with act, and discovered some issues, and disabled some tests -
'should use isEqual to calculate dirty/pristine': This test goes into an infinite loop; it appears act() has caught an actual bug here.
'should formatOnBlur most updated value' and 'should pass multiple through to custom components' appear to be breaking some assumptions for react-dom, and should probably be fixed.

I've included the warning messages that show for these warnings too.

DO NOT MERGE without reading.

This PR updates react to the new rc, and uses the new async act to silence a warning in ReactFinalForm.test.js

Further, regarding Field.test.js, I rewrote `sleep` with `act`, and discovered some issues, and disabled some tests -
'should use isEqual to calculate dirty/pristine': This test goes into an infinite loop; it appears act() has caught an actual bug here.
'should `formatOnBlur` most updated value' and 'should pass multiple through to custom components' appear to be breaking some assumptions for react-dom, and should probably be fixed.

I've included the warning messages that show for these warnings too.
@threepointone
Copy link
Author

Of note, this also updates testing-library/@react to include this fix testing-library/react-testing-library#407

@codecov
Copy link

codecov bot commented Aug 6, 2019

Codecov Report

Merging #578 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #578   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          16     16           
  Lines         233    233           
  Branches       57     57           
=====================================
  Hits          233    233

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a8f1518...a19f3e7. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Aug 6, 2019

Codecov Report

Merging #578 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #578   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          16     16           
  Lines         233    233           
  Branches       57     57           
=====================================
  Hits          233    233

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a8f1518...a19f3e7. Read the comment docs.

@Andarist
Copy link
Contributor

Andarist commented Aug 6, 2019

Interesting, thanks for this! I'll try to look into those failing test to fix them.

I've looked into act implementation now and it seems that it tries to wait (before resolving itself) until current microtask queue finishes and stuff like that. Could you confirm that this is how it makes the whole thing here possible without act actually wrapping the setState callsite?

@threepointone
Copy link
Author

That is correct, yes. But to clarify, generally/in sync mode, you don't have to directly wrap setState callsites either, anywhere above it (in the function callstack) should be fine.

@Andarist
Copy link
Contributor

Andarist commented Aug 6, 2019

Yeah, sure - i meant exactly that by wrapping setState. Stack introspection is normally not possible in strict mode after all.

Thanks for confirming that, cant wait for async act now 😄

@erikras
Copy link
Member

erikras commented Aug 6, 2019

Sweet, some love from the React team! Thanks, @Andarist and @threepointone. 👍

@Andarist
Copy link
Contributor

Andarist commented Aug 8, 2019

16.9 got released - gonna work on finishing this one soon(-ish). Thanks Sunil for showing how things should be done :)

@Andarist Andarist mentioned this pull request Aug 9, 2019
@erikras
Copy link
Member

erikras commented Aug 19, 2019

Implemented by #581.

@erikras erikras closed this Aug 19, 2019
@lock
Copy link

lock bot commented Sep 18, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Sep 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants