Add support for testing with react@17 #1115

AugustinLF · 2022-09-13T13:16:59Z

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 of react-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 for React.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.

mdjastrzebski · 2022-09-13T13:58:47Z

yarn.lock

Are these upgrades related to the PR, if no, them let's exclude them.

(left overs from initial debugging, will remove)

mdjastrzebski · 2022-09-13T14:07:55Z

@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 react + rest-test-renderder + react-native to specified version in place, then run yarn install & yarn test. Not sure how feasible is that but if so, we could even have a list of of supported R/RTR/RN versions.

Also can we use the latest Jest with older versions of R/RN? I think yes, but we'll need to check that.

AugustinLF · 2022-09-14T13:56:25Z

@mdjastrzebski went with your suggestion, let me know how you feel about this solution :)

src/__tests__/render.test.tsx

AugustinLF · 2022-09-14T14:07:56Z

scripts/test_react_17

+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


I hardcoded dependencies, but I assume we should be able to test other combination if we want, like different jest versions

Will this overwrite existing versions of react?

BTW these should be devDeps as currently we put them as such

Will this overwrite existing versions of react?

Yes.

BTW these should be devDeps as currently we put them as such

Will fix that

mdjastrzebski · 2022-09-15T12:26:31Z

.circleci/config.yml

          at: ~/react-native-testing-library
      - run: |
          yarn flow
+  test:react:17:


I would suggest running this after the normal tests

mdjastrzebski · 2022-09-15T12:28:48Z

scripts/test_react_17

+cp yarn.lock tmp.yarn.lock
+cp package.json tmp.package.json


Suggested change

cp yarn.lock tmp.yarn.lock

cp package.json tmp.package.json

cp yarn.lock yarn.lock.backup

cp package.json package.json.backup

mdjastrzebski · 2022-09-15T12:33:33Z

scripts/test_react_17

@@ -0,0 +1,10 @@
+#!/bin/sh -e


env seems to be recommended approach

Suggested change

#!/bin/sh -e

#!/usr/bin/env bash

mdjastrzebski · 2022-09-15T12:34:49Z

package.json

    "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"


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?

BTW since we're invoking test:ci underneath, so maybe test:ci:react-native-0.68.3? Wdyt?

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^^')

mdjastrzebski · 2022-09-15T12:35:36Z

scripts/test_react_17

+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


lets use the latest patch release from 0.68.x

Suggested change

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

mdjastrzebski

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

mdjastrzebski · 2022-09-15T12:37:40Z

package.json

    "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"


Let's put this test:xx next to other test:xx scripts

mdjastrzebski · 2022-09-15T13:09:14Z

scripts/test_react_17

+yarn test:ci
+
+mv tmp.package.json package.json
+mv tmp.yarn.lock yarn.lock


BTW shouldn't we call yarn install again to restore node modules to match yarn.lock?

AugustinLF · 2022-09-16T14:20:20Z

@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 act related changes for react@18 :)

scripts/test_react_17

mdjastrzebski · 2022-09-26T11:09:11Z

🎉 This PR is included in version 11.2.0 🎉

AugustinLF requested a review from mdjastrzebski September 13, 2022 13:16

AugustinLF force-pushed the execute-tests-on-react-17 branch from 6b2a222 to 6f81dc2 Compare September 13, 2022 13:25

mdjastrzebski reviewed Sep 13, 2022

View reviewed changes

Add support for testing with react@17

80308d4

AugustinLF force-pushed the execute-tests-on-react-17 branch from 6f81dc2 to 80308d4 Compare September 14, 2022 11:25

Test with react@17 through a script updating dependencies

2732180

AugustinLF force-pushed the execute-tests-on-react-17 branch from 740e0dd to 2732180 Compare September 14, 2022 12:49

Use test:ci command also for react@17

7185fc8

AugustinLF marked this pull request as ready for review September 14, 2022 13:56

AugustinLF commented Sep 14, 2022

View reviewed changes

src/__tests__/render.test.tsx Show resolved Hide resolved

AugustinLF commented Sep 14, 2022

View reviewed changes

mdjastrzebski reviewed Sep 15, 2022

View reviewed changes

Minor adjustements to script

446420b

mdjastrzebski reviewed Sep 16, 2022

View reviewed changes

scripts/test_react_17 Outdated Show resolved Hide resolved

Update scripts/test_react_17

43768b2

mdjastrzebski approved these changes Sep 16, 2022

View reviewed changes

mdjastrzebski merged commit 10e69c5 into callstack:main Sep 16, 2022

AugustinLF deleted the execute-tests-on-react-17 branch September 18, 2022 14:18

	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

Add support for testing with react@17 #1115

Add support for testing with react@17 #1115

Uh oh!

Conversation

AugustinLF commented Sep 13, 2022

Summary

Approach

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdjastrzebski commented Sep 13, 2022

Uh oh!

AugustinLF commented Sep 14, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdjastrzebski Sep 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdjastrzebski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AugustinLF commented Sep 16, 2022

Uh oh!

Uh oh!

mdjastrzebski commented Sep 26, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mdjastrzebski Sep 15, 2022 •

edited

Loading