Fixes #38971 - refactor Slot.test.js to react testing library #10804
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the Slot.test.js test suite from enzyme/IntegrationTestHelper to React Testing Library (RTL). The refactoring modernizes the testing approach by replacing snapshot tests with more maintainable query-based assertions.
Key Changes
- Migrated all test cases from enzyme to React Testing Library
- Removed snapshot-based assertions in favor of RTL queries (
findByText,queryByText,getByText) - Removed the "render multiple fills" test as it's covered by other tests in the suite
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
Slot.test.js |
Refactored test suite to use RTL, replacing enzyme mounts and snapshots with async queries and DOM assertions |
Slot.test.js.snap |
Removed entire snapshot file as tests no longer rely on snapshots |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| it('default slot', () => { | ||
| const wrapper = integrationTestHelper.mount( | ||
| renderWithStore( | ||
| <Slot id="slot-4" multi> | ||
| <SlotComponent text="Default Value" /> | ||
| </Slot> | ||
| ); | ||
|
|
||
| wrapper.update(); | ||
| expect(wrapper.find('Slot')).toMatchSnapshot(); | ||
| expect(screen.getByText('Default Value')).toBeInTheDocument(); |
There was a problem hiding this comment.
The test 'default slot' should be marked as async since it may need to wait for React state updates, or it should use 'await screen.findByText' instead of 'screen.getByText' for consistency with other tests. Using 'getByText' synchronously may cause flakiness if the component takes time to render.
There was a problem hiding this comment.
I dont think this change is needed
webpack/assets/javascripts/react_app/components/common/Slot/Slot.test.js
Outdated
Show resolved
Hide resolved
webpack/assets/javascripts/react_app/components/common/Slot/Slot.test.js
Outdated
Show resolved
Hide resolved
webpack/assets/javascripts/react_app/components/common/Slot/Slot.test.js
Outdated
Show resolved
Hide resolved
webpack/assets/javascripts/react_app/components/common/Slot/Slot.test.js
Outdated
Show resolved
Hide resolved
webpack/assets/javascripts/react_app/components/common/Slot/Slot.test.js
Outdated
Show resolved
Hide resolved
999f2eb to
7923267
Compare
webpack/assets/javascripts/react_app/components/common/Slot/Slot.test.js
Outdated
Show resolved
Hide resolved
7923267 to
ec957a8
Compare
adamlazik1
left a comment
There was a problem hiding this comment.
LGTM. I don't have permissions to merge but I from my side it is ready to be merged.
removed
render multiple fillsas its tested in other tests in the file