Run React 17 + 18 tests on CI #9942
Conversation
alessbell
force-pushed
the
run-react-17-18-tests
branch
from
5ed772f
to
6c0a60e
Compare
alessbell
changed the title
.circleci/config.yml
Outdated
| @@ -62,4 +75,7 @@ workflows: | |||
| Build and Test: | |||
| jobs: | |||
| - Filesize | |||
| - Tests | |||
| - Tests-React: | |||
There was a problem hiding this comment.
I realize this name is misleading since many of our existing tests don't involve React at all - we could filter out .ts tests into a separate job (or vice versa)
There was a problem hiding this comment.
I know this is piling on more work (sorry! 😬) but I do like the idea of splitting our React tests out as a separate job from the rest of our tests. The way this is broken out right now, it really does make it look like we're just running React based tests, which might throw off future contributors. Especially when people see the test:ci:react-17 and test:ci:react-18 scripts in the package.json; at a first quick glance it isn't clear that the test:ci:react-17 script is the one that's running all of our non-React tests. I think having circle jobs like Tests-Core and Tests-React, and scripts like test:ci:core, test:ci:react-17 and test:ci:react-18 might be more clear.
alessbell
force-pushed
the
run-react-17-18-tests
branch
from
6c0a60e
to
2ea8b98
Compare
There was a problem hiding this comment.
This is awesome @alessbell - thanks very much for working on this!
First a quick comment about the current structure. I know this would be more work, but I'm wondering if it might be better if we flipped things around so the React 18 tests don't have the version based suffix, and our <= React 17 tests do. React is only moving forward which means the non version based suffix tests will only get more and more out of date with the current structure. Flipping things around so useMutation.test.tsx represents our current tests (for the current dev dep version of React, which we'd update to React 18) and useMutation.17.test.tsx means tests for older specific version of React, might put us on a better path moving forward.
The above being said though, I'm wondering how these changes will play out from an ongoing project maintenance point of view. Doesn't this mean that we'll be asking people making code changes to add similar tests in multiple versions of the same test file (e.g. useMutation.test.tsx and useMutation.18.test.tsx)? This might get quickly out of hand especially for new contributors who probably won't notice that we need tests in 2 places. If this is just an unavoidable consequence of the cutover to React 18 and we can't avoid it, so be it. I just think we should be extra super sure that there isn't another workaround that would avoid having us maintain duplicate copies of our tests, for different React versions. Put another way, are we sure the benefits gained from introducing this multi-version React testing strategy outweigh the maintenance cost it will introduce? If we're expecting to see a lot of differences between React 17 and 18 behaviour with our hooks then it could be worth it, but I think we should be as sure as we can before introducing this new approach.
Just some thoughts though - I'd love to hear from @benjamn @MrDoomBringer @jpvajda as well. Thanks again!
.circleci/config.yml
Outdated
| @@ -62,4 +75,7 @@ workflows: | |||
| Build and Test: | |||
| jobs: | |||
| - Filesize | |||
| - Tests | |||
| - Tests-React: | |||
There was a problem hiding this comment.
I know this is piling on more work (sorry! 😬) but I do like the idea of splitting our React tests out as a separate job from the rest of our tests. The way this is broken out right now, it really does make it look like we're just running React based tests, which might throw off future contributors. Especially when people see the test:ci:react-17 and test:ci:react-18 scripts in the package.json; at a first quick glance it isn't clear that the test:ci:react-17 script is the one that's running all of our non-React tests. I think having circle jobs like Tests-Core and Tests-React, and scripts like test:ci:core, test:ci:react-17 and test:ci:react-18 might be more clear.
|
Thanks for the review @hwillson! Agree with the points you raised, especially the comment about maintenance cost. I think there may be a much better way to do this: for example, That project doesn't seem to have the problem with the breaking RTL hooks API change that we do, but we only use |
alessbell
force-pushed
the
run-react-17-18-tests
branch
from
2b77aca
to
6e4c3de
Compare
…lo-client into run-react-17-18-tests
There was a problem hiding this comment.
Oooh I love this approach @alessbell - thanks very much! 🙇
Just one quick question for a bit of clarification, but I'm 👍 on this for sure. Thanks again!
It looks like some combination of the changes in #9928 and #9942 exposed a long running issue caused by using `crypto-hash` in our `@apollo/client/link/persisted-queries` tests. `crypto-hash` spawns a thread the first time it's called, and keeps that thread alive until the end of program execution, if worker threads are available (which they are for our tests since they're running in CI via Node). Unfortunately there appears to be some kind of a race condition between `crypto-hash` `unref`ing its worker and Jest reporting a successful test suite exit, which leads to Jest reporting the following (which is seen by running jest with the `--detectOpenHandles` setting enabled): ``` Jest has detected the following X open handles potentially keeping Jest from exiting: ``` This then appears to be leading to failed tests in CI. We can look further into what's causing this race condition between `crypto-hash` and Jest, but just dropping the use of `crypto-hash` is likely a faster solution. We just need to verify that a hashing function can be successfully set and used with the persisted queries link, which means we can replace `crypto-hash` with our own simple function that doesn't use worker threads. This commit does just that by introducing a super simple custom `sha256` function that leverages Node `crypto`, and uses it instead. Fixes #9982
It looks like some combination of the changes in #9928 and #9942 exposed a long running issue caused by using `crypto-hash` in our `@apollo/client/link/persisted-queries` tests. `crypto-hash` spawns a thread the first time it's called, and keeps that thread alive until the end of program execution, if worker threads are available (which they are for our tests since they're running in CI via Node). Unfortunately there appears to be some kind of a race condition between `crypto-hash` `unref`ing its worker and Jest reporting a successful test suite exit, which leads to Jest reporting the following (which is seen by running jest with the `--detectOpenHandles` setting enabled): ``` Jest has detected the following X open handles potentially keeping Jest from exiting: ``` This then appears to be leading to failed tests in CI. We can look further into what's causing this race condition between `crypto-hash` and Jest, but just dropping the use of `crypto-hash` is likely a faster solution. We just need to verify that a hashing function can be successfully set and used with the persisted queries link, which means we can replace `crypto-hash` with our own simple function that doesn't use worker threads. This commit does just that by introducing a super simple custom `sha256` function that leverages Node `crypto`, and uses it instead. Fixes #9982
It looks like some combination of the changes in apollographql#9928 and apollographql#9942 exposed a long running issue caused by using `crypto-hash` in our `@apollo/client/link/persisted-queries` tests. `crypto-hash` spawns a thread the first time it's called, and keeps that thread alive until the end of program execution, if worker threads are available (which they are for our tests since they're running in CI via Node). Unfortunately there appears to be some kind of a race condition between `crypto-hash` `unref`ing its worker and Jest reporting a successful test suite exit, which leads to Jest reporting the following (which is seen by running jest with the `--detectOpenHandles` setting enabled): ``` Jest has detected the following X open handles potentially keeping Jest from exiting: ``` This then appears to be leading to failed tests in CI. We can look further into what's causing this race condition between `crypto-hash` and Jest, but just dropping the use of `crypto-hash` is likely a faster solution. We just need to verify that a hashing function can be successfully set and used with the persisted queries link, which means we can replace `crypto-hash` with our own simple function that doesn't use worker threads. This commit does just that by introducing a super simple custom `sha256` function that leverages Node `crypto`, and uses it instead. Fixes apollographql#9982
This PR adds the ability to run our test suite against both React versions 17 and 18.
It employs the same approach as
react-redux: using jest'sprojectsfeature + installing multiple versions as dev dependencies and namespacing them (e.g."react-17": "npm:react@^17") so they can be mapped usingmoduleNameMapper. The only downside to this approach seems to be having to runnpm i --legacy-peer-depssince package.json now contains multiple conflicting versions that error out when runningnpm iwith npm 8.This PR includes the following changes:
jest.config.jsto run three groups of tests withnpm run test:@types/react/@types/react-dom@^18@jest-environment nodefor ssr testsOn my m1 mac, all 3 "projects" run in ~60s:
On CI they took ~200s:
Next steps
Many of the failing tests seem like examples of this failure mode, so we'll have to decide what to do with them. I also propose we remove
@testing-library/react-hooksfrom the four files using it in future work and use RTL'srenderinstead to avoid forking the tests, since@testing-library/react-hooks'srenderHookAPI was moved into RTL@v13 which is not compatible with React 17.Checklist: