-
Notifications
You must be signed in to change notification settings - Fork 139
MM-67214: Upgrade to React 18 and migrate tests to React Testing Library #1267
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
Conversation
026919c to
bb5cdbf
Compare
bb5cdbf to
a4be6df
Compare
| let args = props.searchIssues.mock.calls[0][0]; | ||
| expect(args).toEqual({ | ||
| fields: 'customfield_10011', | ||
| jql: 'project=KT and issuetype=10000 ORDER BY updated DESC', | ||
| q: '', | ||
| instance_id: 'https://something.atlassian.net', |
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.
It seems that we lost the check for the args here and in the one below we are not checking for all of them?
| ); | ||
| }); | ||
|
|
||
| expect(ref.current).toBeDefined(); |
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.
Should we check other elements directly? Seems that just checking that the component exists is lowering the quality of the test compared to snapshot
This applies to all cases below where the snapshot has been removed.
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.
The issues are with internal validation that runs when the component is fully mounted. This is done when validator collects validation functions from child components. The issue is that with deep rendering child components register their validators and when handleCreate is called validation fails because the child components aren't fully set up.
The tests were designed for shallow rendering which doesn't run full lifecycle. I would propose to add new tests as we develop new features and fix bugs, as direct rewrite would be very difficult and time consuming atm. but if you think it can be prioritised I can do it :)
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.
@marianunez I updated what I could in order to replicate the old tests, I think I covered a pretty good chunk of them!
| jest.clearAllMocks(); | ||
| }); | ||
|
|
||
| test('should match snapshot', async () => { |
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.
The test name should also be updated if no snapshots are being used
| "react-redux": "7.2.9", | ||
| "react-select": "3.2.0", | ||
| "react-redux": "8.1.3", | ||
| "react-select": "5.10.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.
This is a large jump for the react-select so we need to make sure we have a testing pass on any dropdowns to check for any regressions here @ogi-m
avasconcelos114
left a comment
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.
Added a couple of comments of my own but I would +1 on the renaming tests that no longer are related to snapshot testing
I'll likely need to spend more time to look through the changes in the modal tests
When it comes to the data_selectors, considering they're basically HoCs that only change the search logic and value preselection, I agree that full rendering feels like way more trouble than it's worth at this stage -- though maybe we can have a backlog task added to add tests on the BackendSelector itself
| const renderWithRedux = (ui: React.ReactElement, initialState = defaultMockState) => { | ||
| const store = mockStore(initialState); | ||
| return { | ||
| store, | ||
| ...render( | ||
| <IntlProvider locale='en'> | ||
| <Provider store={store}>{ui}</Provider> | ||
| </IntlProvider>, | ||
| ), | ||
| }; | ||
| }; |
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.
It feels like these repeated instances of renderWithRedux could be replaced with the one that was added as a part of test-utils.tsx, or was there a reason for them to have their own declaration in here and jira_epic_selector.test.tsx (among others)?
| const mockTheme = { | ||
| centerChannelColor: '#333333', | ||
| centerChannelBg: '#ffffff', | ||
| buttonBg: '#166de0', | ||
| buttonColor: '#ffffff', | ||
| linkColor: '#2389d7', | ||
| errorTextColor: '#fd5960', | ||
| }; |
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.
Nit: This is another thing that could be a constant provided by test utils to reduce repetition
9526517 to
caef7db
Compare
avasconcelos114
left a comment
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.
Things are looking quite tidy now :) I think this leaves us in a good spot to continue bolstering the testing suite as we make new contributions
|
@marianunez @ogi-m kind reminder to review |
ogi-m
left a comment
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.
LGTM!
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
Summary
Upgraded React from 16.14.0 to 18.2.0
Upgraded react-dom from 16.8.6 to 18.2.0
Upgraded react-redux from 7.2.9 to 8.1.3
Upgraded react-select from 3.2.0 to 5.10.0
Upgraded react-intl from 4.5.0 to 6.8.0
Removed Enzyme and migrated all tests to React Testing Library.
Updated Babel configuration for React 18 automatic JSX runtime.
Fixed react-select type imports for v5 compatibility.
Ticket Link
https://mattermost.atlassian.net/browse/MM-67214