chore: Migrate tests to RTL (5) #7055
Conversation
jpuzz0
force-pushed
the
chore/rtl-migrate-bundle-5
branch
from
6e2245c to
ded9924
Compare
|
Preview: https://patternfly-react-pr-7055.surge.sh A11y report: https://patternfly-react-pr-7055-a11y.surge.sh |
jpuzz0
force-pushed
the
chore/rtl-migrate-bundle-5
branch
2 times, most recently
from
38e9f91 to
7730ad0
Compare
jpuzz0
force-pushed
the
chore/rtl-migrate-bundle-5
branch
from
7730ad0 to
528aaca
Compare
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
There were some tests that looked like they could be place into their own test file (the SimpleListGroup and SimpleListItem tests at the end of the SimpleList test file, most of the tests in the ToggleGroup test file). I didn't comment on them specifically since those are things that I figured would be part of the next phase.
Below are mainly nitpicks or asking for clarification.
packages/react-core/src/components/ToggleGroup/__tests__/ToggleGroup.test.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/components/TextInput/__tests__/TextInput.test.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/components/TextArea/__tests__/TextArea.test.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/components/Switch/__tests__/Switch.test.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/components/Select/__tests__/SelectToggle.test.tsx
Outdated
Show resolved
Hide resolved
| <Pagination | ||
| itemCount={40} | ||
| toggleTemplate={'${firstIndex} - ${lastIndex} - ${itemCount} - ${itemsTitle}'} | ||
| data-testid="test-id" |
There was a problem hiding this comment.
Not a blocker for me, but I'm curious why you added this test id here as it doesn't look like it's getting used?
There was a problem hiding this comment.
Just leftover from when I wasn't using asFragment.
| render(<Radio id="check" {...props} aria-label="check" name="check" />); | ||
|
|
||
| userEvent.click(screen.getByRole('radio')); | ||
| expect(props.onChange).toHaveBeenCalled(); |
There was a problem hiding this comment.
This is no longer testing the same thing as it previously did, I understand that we're no longer simulating a change event in the same way, but it should still be getting called with the same arguments right?
| describe('Switch', () => { | ||
| test('switch label for attribute equals input id attribute', () => { | ||
| render(<Switch id="foo" aria-label="Switch label" />); | ||
| expect(screen.getByLabelText('Switch label')).toHaveAttribute('id', 'foo'); |
There was a problem hiding this comment.
I don't think this is quite testing the same thing anymore, since based on the test name I believe that the purpose of this test was ensuring that the htmlFor prop was correctly applied.
| render(<TextAreaBase {...props} aria-label="test textarea" />); | ||
|
|
||
| userEvent.type(screen.getByLabelText('test textarea'), 'new test textarea'); | ||
| expect(props.onChange).toHaveBeenCalled(); |
There was a problem hiding this comment.
As in Radio I don't think this is testing the same thing as it previously did, as it's no longer asserting anything about the arguments being passed.
There was a problem hiding this comment.
Updated this and the other tests mentioned to better reflect what was previously tested.
| expect(view.container).toMatchSnapshot(); | ||
| }); | ||
| userEvent.type(screen.getByLabelText('test input'), 'new test input'); | ||
| expect(props.onChange).toHaveBeenCalled(); |
There was a problem hiding this comment.
Per previous comments I don't think this is still testing the same thing.
There was a problem hiding this comment.
Right, it won't test the same thing regardless because of the difference in changing the inputs between the 2 tests. The onChange for instance is called for every character. I will update to something more inline with the original though.
| ); | ||
|
|
||
| userEvent.click(screen.getByRole('button')); | ||
| expect(props.onChange).toHaveBeenCalled(); |
There was a problem hiding this comment.
Test is no longer testing arguments being passed here as well.
5 participants
What: Closes #7045