Rtl upgrade #218
Rtl upgrade #218
Conversation
AlexanderKaran
commented
Nov 17, 2023
AlexanderKaran
commented
- Upgraded all the tests from enzyme to react testing library to make upgrading to React 18 easier
- Updated every test to RTL
- Moved towards more testing patterns supported by RTL
| @@ -90,8 +90,8 @@ | |||
| "@codeshift/test-utils": "^0.3.0", | |||
| "@codeshift/utils": "^0.2.0", | |||
| "@testing-library/jest-dom": "^5.7.0", | |||
| "@testing-library/react": "^10.0.4", | |||
| "@types/enzyme": "^3.10.5", | |||
| "@testing-library/react": "^10.4.9", | |||
There was a problem hiding this comment.
nit: are we able to upgrade to 14.0.0 (or whatever newest version is without anything breaking? Wonder if we can get best of both worlds ;)
There was a problem hiding this comment.
Tried to install the latest version; however it throws an error due to React16 being in the dependent tree
npm install --save-dev @testing-library/[email protected]
npm ERR! code ERESOLVE
npm ERR! ERESOLVE unable to resolve dependency tree
npm ERR!
npm ERR! While resolving: [email protected]
npm ERR! Found: [email protected]
npm ERR! node_modules/react
npm ERR! dev react@"^16.10.2" from the root project
npm ERR!
npm ERR! Could not resolve dependency:
npm ERR! peer react@"^18.0.0" from @testing-library/[email protected]
npm ERR! node_modules/@testing-library/react
npm ERR! dev @testing-library/react@"14.1.2" from the root project
npm ERR!
npm ERR! Fix the upstream dependency conflict, or retry
npm ERR! this command with --force or --legacy-peer-deps
npm ERR! to accept an incorrect (and potentially broken) dependency resolution.
npm ERR!
npm ERR!
npm ERR! For a full report see:
npm ERR! /Users/akaran/.npm/_logs/2023-11-22T02_25_00_700Z-eresolve-report.txt
There was a problem hiding this comment.
ah makes sense, RTL v14 is dependent on. react v18
|
Looks good! I'll ping someone else to approve but ✅ from me (some nits, but they are nice to haves, not blocking) |
Thanks for the review will fix up the comments you mentioned today |
| @@ -548,12 +548,12 @@ describe('RouterStore', () => { | |||
| const RouteName = () => <>{useRouteName()}</>; | |||
|
|
|||
| const route = { | |||
| component: () => null, | |||
| component: () => <p>home</p>, | |||
There was a problem hiding this comment.
Any particular reason why this was changed?
There was a problem hiding this comment.
The approach to testing is very different in RTL vs enzyme. The biggest being "test from the perspective of a user". I had to change the test to work by checking for what was rendered.
| @@ -567,7 +567,7 @@ describe('RouterStore', () => { | |||
| expect.objectContaining({ route }), | |||
| undefined | |||
| ); | |||
| expect(wrapper.html()).toEqual('home'); | |||
There was a problem hiding this comment.
I wonder why was html==='home'? I thought component: () => null, should render null
There was a problem hiding this comment.
I am not sure about the old tests, but in the new one, I am rendering something with the text home to ensure things are correctly rendered.
There was a problem hiding this comment.
nice!
@MonicaOlejniczak if you're happy with it - are you able to merge? I don't have perms 😮💨
