-
Notifications
You must be signed in to change notification settings - Fork 1k
Migrate snapshot tests to RTL #10642
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
base: develop
Are you sure you want to change the base?
Conversation
This is the origianl changes proposed by cursor I will make my changes on top of 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.
Pull Request Overview
This PR migrates 13 React component test files from Enzyme snapshot testing to React Testing Library (RTL), representing a comprehensive shift from implementation-focused to user-behavior-focused testing. The migration improves test maintainability, accessibility coverage, and user interaction testing while removing brittle snapshot dependencies.
Key Changes:
- Migrated 13 test files covering forms, charts, date components, and UI elements from Enzyme to RTL
- Created comprehensive test helper utilities for common testing patterns
- Removed all corresponding snapshot files to eliminate test brittleness
- Added detailed migration documentation with patterns and best practices
Reviewed Changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
Form.test.js | Migrated form error handling and validation tests to RTL with behavioral assertions |
CommonForm.test.js | Converted form field validation and interaction tests to user-focused RTL patterns |
Actions.test.js | Updated form button tests to verify user interactions and accessibility |
MessageBox.test.js | Migrated icon and message display tests to RTL with semantic querying |
SearchInput.test.js | Converted to RTL with focus management and user input interaction testing |
ShortDateTime.test.js | Migrated date formatting tests with i18n integration and async handling |
RelativeDateTime.test.js | Updated relative time formatting tests with proper i18n context |
IsoDate.test.js | Converted ISO date formatting tests with timezone handling |
Loader.test.js | Migrated loading state tests to verify conditional rendering behavior |
AlertBody.test.js | Updated alert content tests to focus on accessibility and user-visible content |
BarChart.test.js | Migrated chart rendering tests with service integration verification |
DonutChart.test.js | Converted chart visualization tests to behavioral patterns |
ModalProgressBar.test.js | Updated modal and progress tracking tests with accessibility focus |
testHelpers.js | Created comprehensive testing utilities for Redux, i18n, and common assertions |
ENZYME_TO_RTL_MIGRATION_GUIDE.md | Added detailed migration documentation and patterns |
expect(element).toBeInTheDocument(); | ||
|
||
// Should not crash and should render something | ||
expect(container.firstChild).toBeTruthy(); |
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 references an undefined variable 'container'. It should reference 'screen.container' instead.
expect(container.firstChild).toBeTruthy(); | |
expect(screen.container.firstChild).toBeTruthy(); |
Copilot uses AI. Check for mistakes.
|
||
// Test that warning icon element exists with correct classes | ||
const container = screen.getByText('warning message').closest('div'); | ||
const icon = container.querySelector('.pficon-warning-triangle-o'); |
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 CSS class name 'pficon-warning-triangle-o' is inconsistent with the 'warning' icontype prop. Verify that the icon class mapping is correct for the warning type.
Copilot uses AI. Check for mistakes.
|
||
expect(wrapper).toMatchSnapshot(); | ||
// Test that chart container is rendered | ||
const chartContainer = screen.container.querySelector('[id*="chart"], .c3, .donut-chart, [class*="chart"]'); |
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 querySelector uses multiple fallback selectors which makes the test brittle. Consider using a more specific data-testid or role-based approach for better maintainability.
const chartContainer = screen.container.querySelector('[id*="chart"], .c3, .donut-chart, [class*="chart"]'); | |
const chartContainer = screen.getByTestId('donut-chart-container'); |
Copilot uses AI. Check for mistakes.
This is the original changes proposed by cursor
I will make my changes on top of it.