-
Notifications
You must be signed in to change notification settings - Fork 264
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
Add support for testing with react@17 #1115
Add support for testing with react@17 #1115
Conversation
6b2a222
to
6f81dc2
Compare
yarn.lock
Outdated
version "9.0.0" | ||
resolved "https://registry.yarnpkg.com/@react-native-community/cli-platform-android/-/cli-platform-android-9.0.0.tgz#c21b26f456c568687c0e58a6e42ba8b11b607b8a" | ||
integrity sha512-4Rp5OUZW/7Qc9hyCd+ZEikuu2k9dW3ssu6KzWygbVc9wY80i4GQmvjfsiUi21o3uPDvL4KUMANmnQqoTOIcVMA== | ||
"@react-native-community/cli-platform-android@^9.0.0", "@react-native-community/cli-platform-android@^9.1.0": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these upgrades related to the PR, if no, them let's exclude them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(left overs from initial debugging, will remove)
@AugustinLF I like the idea of this PR, so that we are able to test our compatibility with React 17/older React Native. I wonder however if copying files is the right approach, maybe we could have a (shell/sed/etc) script that would update Also can we use the latest Jest with older versions of R/RN? I think yes, but we'll need to check that. |
6f81dc2
to
80308d4
Compare
740e0dd
to
2732180
Compare
@mdjastrzebski went with your suggestion, let me know how you feel about this solution :) |
scripts/test_react_17
Outdated
cp yarn.lock tmp.yarn.lock | ||
cp package.json tmp.package.json | ||
|
||
yarn add react@17.0.2 react-test-renderer@17.0.2 react-native@0.68.1 --ignore-scripts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hardcoded dependencies, but I assume we should be able to test other combination if we want, like different jest versions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this overwrite existing versions of react
?
BTW these should be devDeps
as currently we put them as such
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this overwrite existing versions of react?
Yes.
BTW these should be devDeps as currently we put them as such
Will fix that
.circleci/config.yml
Outdated
@@ -49,6 +49,13 @@ jobs: | |||
at: ~/react-native-testing-library | |||
- run: | | |||
yarn flow | |||
test:react:17: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest running this after the normal tests
scripts/test_react_17
Outdated
cp yarn.lock tmp.yarn.lock | ||
cp package.json tmp.package.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cp yarn.lock tmp.yarn.lock | |
cp package.json tmp.package.json | |
cp yarn.lock yarn.lock.backup | |
cp package.json package.json.backup |
scripts/test_react_17
Outdated
@@ -0,0 +1,10 @@ | |||
#!/bin/sh -e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
env seems to be recommended approach
#!/bin/sh -e | |
#!/usr/bin/env bash |
package.json
Outdated
@@ -82,7 +82,8 @@ | |||
"build:ts": "tsc --build tsconfig.release.json", | |||
"build:ts:watch": "yarn build:ts --watch --preserveWatchOutput", | |||
"build": "yarn clean && yarn build:js && yarn build:ts && yarn copy-flowtypes", | |||
"prepare": "yarn build" | |||
"prepare": "yarn build", | |||
"test:react:17": "scripts/test_react_17" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are RN TL maybe we should rather name our test script with React Native version, e.g. test-react-native-0.68.3
, @AugustinLF wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW since we're invoking test:ci
underneath, so maybe test:ci:react-native-0.68.3
? Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are RN TL, but I would argue that here we are actually testing the react differences, not as much the react-native ones (and we're still using react, I'd argue the problem is that @testing-library/react
should be renamed @testing-library/react-dom
^^')
scripts/test_react_17
Outdated
cp yarn.lock tmp.yarn.lock | ||
cp package.json tmp.package.json | ||
|
||
yarn add react@17.0.2 react-test-renderer@17.0.2 react-native@0.68.1 --ignore-scripts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets use the latest patch release from 0.68.x
yarn add react@17.0.2 react-test-renderer@17.0.2 react-native@0.68.1 --ignore-scripts | |
yarn add react@17.0.2 react-test-renderer@17.0.2 react-native@0.68.3 --ignore-scripts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the current approach with modifying package.json and restoring changes afterwards. The current solution looks pretty solid. I've added some suggestions for improvint the reliability
package.json
Outdated
@@ -82,7 +82,8 @@ | |||
"build:ts": "tsc --build tsconfig.release.json", | |||
"build:ts:watch": "yarn build:ts --watch --preserveWatchOutput", | |||
"build": "yarn clean && yarn build:js && yarn build:ts && yarn copy-flowtypes", | |||
"prepare": "yarn build" | |||
"prepare": "yarn build", | |||
"test:react:17": "scripts/test_react_17" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's put this test:xx
next to other test:xx
scripts
scripts/test_react_17
Outdated
yarn test:ci | ||
|
||
mv tmp.package.json package.json | ||
mv tmp.yarn.lock yarn.lock |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW shouldn't we call yarn install
again to restore node modules to match yarn.lock
?
@mdjastrzebski I implemented all your suggestions (or answered in the comments), let me know if you need anything else. If not I think we can merge this so I can start working on the |
🎉 This PR is included in version 11.2.0 🎉 |
Summary
Follow-up on the discussion that happened in #1093 (comment). If we want to change the way we work with act and be more aligned with RTL, we either need to drop support to
react@17
(which I'd like to avoid doing), or we need to increase our confidence that we're not breaking older versions.I don't think we really need to cover for
react@<=16
because tests mostly behave the same way, but when trying to work on bumping react in the codebase I work on, a lot of tests break for subtle reasons.Approach
I went with a fairly simple approach where I copy the whole test/code base to be able to run any PR changes on both platforms (which prevents us from using the
examples/
approach that reaches to a published version of RNTL).Since different versions of
react
also implies different versions ofreact-native
, we have a problem where some tests can't work in both cases. Currently the issue seems to be only with snapshots. I don't have any brilliant idea, a solution would be to do runtime check forReact.version
and have a snapshot by version. We could replace those specific snapshots with something that works in both versions, but I think we'll run in this problem regardless.