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

Removed react-16.8 from allowed_failures in travis #2122

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AlokTakshak
Copy link

@AlokTakshak AlokTakshak commented May 11, 2019

Closes #2113.

@AlokTakshak AlokTakshak changed the title Removed react-16.8, react-16.8.5 from allowed_failures in travis Removed react-16.8 from allowed_failures in travis May 16, 2019
@AlokTakshak
Copy link
Author

@ljharb
I have removed react 16.8 from allowed_failures

.travis.yml Outdated
@@ -33,7 +33,7 @@ matrix:
- node_js: "lts/*"
env: REACT=16.8.3
- node_js: "lts/*"
env: REACT=16.8 RENDERER=16.7
env: REACT=16.8 RENDERER=16.8
Copy link
Member

Choose a reason for hiding this comment

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

this is breaking the purpose of the test, which is to test react 16.8 with the 16.7 renderer.

Suggested change
env: REACT=16.8 RENDERER=16.8
env: REACT=16.8 RENDERER=16.7

Copy link
Author

Choose a reason for hiding this comment

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

But we need corresponding version of react and react-dom if there is version mismatch - hooks test will fail as writtern on react website one of reason for invalid hooks is
Mismatching Versions of React and React DOM

Copy link
Member

Choose a reason for hiding this comment

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

react and react-dom will both be 16.8 with this. RENDERER is about react-test-renderer, which doesn't necessarily have to match.

Copy link
Author

Choose a reason for hiding this comment

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

ok , then i can use render, fireEvent, getByTestId for updating the failing test cases?

Copy link
Member

Choose a reason for hiding this comment

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

The work that needs to be done to close the linked issue is to determine if the existing tests are valid - if so, the actual code needs to be fixed; or, if they're not valid, change them so they are valid.

Copy link
Author

Choose a reason for hiding this comment

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

@ljharb I was going through a lot of articles the react-test-renderer before 16.8.5 have problem with shallow rendering for hooks , as they won't re-render after setting State , and we are using same the same in Adaptors so shallow won't work for version below 16.8.5,
now what we should do, modify the adaptor?

Copy link
Member

Choose a reason for hiding this comment

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

yes, ideally we'd modify the react 16 adapter to have fallback behavior for an older renderer version.

Copy link
Member

Choose a reason for hiding this comment

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

@AlokTakshak any interest in pursuing this fix?

Copy link
Author

Choose a reason for hiding this comment

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

I want to but i don't have any clue of how to fix it

Copy link
Member

Choose a reason for hiding this comment

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

My thinking is that the react 16 adapter would add some way (only for react-test-renderer prior to 16.8.5) to catch useState calls and force a rerender manually.

@codecov
Copy link

codecov bot commented Dec 21, 2020

Codecov Report

Merging #2122 (3654397) into master (0e39e1f) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2122   +/-   ##
=======================================
  Coverage   96.12%   96.12%           
=======================================
  Files          49       49           
  Lines        4004     4004           
  Branches     1123     1123           
=======================================
  Hits         3849     3849           
  Misses        155      155           

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 0e39e1f...3654397. Read the comment docs.

@ljharb ljharb force-pushed the master branch 4 times, most recently from 2227326 to 0d5ead7 Compare December 21, 2020 07:46
@ljharb ljharb force-pushed the master branch 3 times, most recently from 43eb75e to 39e6b1f Compare November 3, 2022 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

hooks: fix failing tests with mismatched renderer
2 participants