Migrate from react-test-renderer to @testing-library/react
#4540
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4540 +/- ##
========================================
Coverage 39.09% 39.09%
========================================
Files 852 852
Lines 36845 36845
Branches 5762 6012 +250
========================================
Hits 14404 14404
Misses 21923 21923
Partials 518 518
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Summary
This PR systematically migrates the entire test suite from react-test-renderer to @testing-library/react, affecting 23+ test files across the Quilt catalog application. The migration replaces renderer.create().toJSON() calls with render() and updates snapshot assertions to use container.firstChild instead of the full component tree. This is a comprehensive modernization effort that standardizes the testing approach across components including form elements (BucketPreferences, QuiltSummarize), UI components (BreadCrumbs, BucketIcon, Buttons), container components (Search Results, Bucket queries), and utility components (MetaTitle, Logo).
The migration maintains existing test coverage and behavior while adopting a more modern testing philosophy that focuses on DOM-based testing rather than React's internal component tree structure. All existing Jest mocks remain functional, and the change primarily affects the rendering mechanism rather than test logic. The package.json update removes the react-test-renderer and @types/react-test-renderer dependencies while relying on the already-present @testing-library/react v12.1.5.
This migration aligns the codebase with current React testing best practices, as @testing-library/react is now the community standard for React component testing. The library encourages testing components as users would interact with them rather than testing implementation details, and provides better debugging tools and DOM-based utilities.
Confidence score: 2/5
- This PR has significant implementation issues that could cause test failures and requires careful review
- Score lowered due to critical bugs in error handling tests, async testing logic issues, and potential snapshot compatibility problems
- Pay close attention to
catalog/app/containers/Bucket/Queries/Athena/model/state.spec.tsxandcatalog/app/components/FileEditor/QuiltConfigEditor/QuiltSummarize/QuiltSummarize.spec.tsx
24 files reviewed, 5 comments
| }) | ||
|
|
||
| it('should render Bucket directory', () => { | ||
| jest.mock('react-redux') |
There was a problem hiding this comment.
logic: This jest.mock('react-redux') call inside the test function won't work as expected. Mocks should be at module level or use mockImplementation within tests.
| jest.mock('react-redux') | |
| it('should render Bucket directory', () => { | |
| const { container } = render( | |
| <TestBucket> | |
| <RowActions to="/b/bucketA/tree/dirB/" prefs={defaultPrefs} onReload={noop} /> | |
| </TestBucket>, | |
| ) | |
| expect(container.firstChild).toMatchSnapshot() | |
| }) |
| const textField = container.querySelector('#text-field') | ||
| expect(textField).not.toBeNull() | ||
| }) |
There was a problem hiding this comment.
logic: This assertion inside waitFor() is unusual. Typically, waitFor() should wait for a condition to be true, not make assertions. Consider using expect(textField).toBeInTheDocument() or just checking if (textField) return true.
| const textFields = container.querySelectorAll('#text-field') | ||
| expect(textFields.length).toBeGreaterThan(1) | ||
| }) |
There was a problem hiding this comment.
logic: Same issue here - assertions inside waitFor() can cause unexpected behavior. The condition should return a boolean for waitFor() to work correctly.
| expect(input.props['data-error']).toBeFalsy() | ||
| const input = findGteInput(container) | ||
| expect(input.value).toBe('42') | ||
| expect(input.getAttribute('data-error')).toBe('false') |
There was a problem hiding this comment.
logic: Comparing to 'false' string instead of falsy check - should verify this matches the actual component behavior
No consistent performance gain, unfortunately. Slightly cleaner snapshots.
But, at least, we don't use a deprecated package.