chore(Avatar) update tests to new standarts#7829
chore(Avatar) update tests to new standarts#7829nicolethoen merged 3 commits intopatternfly:mainfrom
Conversation
|
Preview: https://patternfly-react-pr-7829.surge.sh A11y report: https://patternfly-react-pr-7829-a11y.surge.sh |
wise-king-sullyman
left a comment
wise-king-sullyman
left a comment
There was a problem hiding this comment.
This is good progress! I have a couple of requests for things I would like to see changed and a handful of requests for additional tests.
| test('renders simple avatar', () => { | ||
| render(<Avatar alt="avatar" data-testid="avatar" src="test.png" border="light" />); | ||
| expect(screen.getByTestId('avatar')).toBeVisible(); |
There was a problem hiding this comment.
This is how I've been doing the "renders" test myself, but since the purpose of this kind of test is to be a sanity check ensuring that the component renders relying on the component to pass the data-testid is actually not the best approach.
Jeff suggested the approach that I went with in the version of this test in my overflow tab tests PR and I think it would be an improvement here as well.
| test('renders with class name pf-m-light when "light" is passed as border prop', () => { | ||
| render(<Avatar alt="avatar" data-testid="avatar" src="test.png" border="light" />); | ||
| expect(screen.getByTestId('avatar')).toHaveClass('pf-m-light'); | ||
| }); |
There was a problem hiding this comment.
I've gone back and forth a bit with how I feel about testing the different values that can be provided for a prop.
In situations like this prop where the different options are very limited and different classes are applied based on the option used I think each option (and the default behavior) should be tested, i.e. a test that neither class is applied by default, a test that the light class is applied with the light option, and a test that the dark class is applied with the dark option.
| test('renders with class name pf-m-lg when "lg" is passed as size prop', () => { | ||
| render(<Avatar alt="avatar" data-testid="avatar" src="test.png" size='lg' />); | ||
| expect(screen.getByTestId('avatar')).toHaveClass('pf-m-lg'); | ||
| }); |
There was a problem hiding this comment.
Similar situation here to the previous comment.
| expect(screen.getByTestId('avatar')).toHaveClass('pf-m-lg'); | ||
| }); | ||
|
|
||
|
|
There was a problem hiding this comment.
Additional things I would like to see tested:
- That it renders with
pf-c-avatar - That the
classNameprop functions - That the
srcprop functions - That the
altprop functions - That non-explicitly declared props (such as
aria-labelin this case) can be applied as expected - A snapshot test of the component to prevent unintended changes of the default behavior
There was a problem hiding this comment.
Also one note: typically we want each test to ideally only include props that are either required or under test, so a lot of the props in use in the tests you have here can be removed.
There was a problem hiding this comment.
Hopefully all the required changes have been resolved. Thanks for the feedback. I am not quite sure about the way the src test is carried out though, let me know what you think.
gitdallas
left a comment
gitdallas
left a comment
There was a problem hiding this comment.
kinda wonder if the snapshot should include several props.. but overall coverage lgtm
wise-king-sullyman
left a comment
wise-king-sullyman
left a comment
There was a problem hiding this comment.
This is seriously great work 💪 there's only one thing I would like to see changed (though it is repeated in almost all of the tests) and then it should be good to go!
Rather than using
<div data-testid="avatar">
<Avatar alt="avatar" />
</div>In all tests I would like to see that only used in the first test, because in that first test we're just trying to establish that something is rendered without having to make any other assumptions. Apologies if I was unclear about that.
For all other tests I would like to see getByRole('img') used to query for the component.
| <Avatar alt="avatar" /> | ||
| </div> | ||
| ); | ||
| expect(screen.getByTestId('avatar').firstChild).toHaveClass('pf-c-avatar', { exact: true }); |
There was a problem hiding this comment.
Using { exact: true } here was a 🔥 idea and something I'll be doing in the future in similar scenarios!
| </div> | ||
| ); | ||
| const image = screen.getByRole('img') as HTMLImageElement; | ||
| expect(image.src).toMatch('test.png'); |
There was a problem hiding this comment.
I think this was a great way of testing that src works properly!
Hey, I have added a few props to snapshot test in most recent commit. |
wise-king-sullyman
left a comment
wise-king-sullyman
left a comment
There was a problem hiding this comment.
🚀 great work on this!
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
* chore(Avatar) update tests to new standarts * add more tests * fix querying components
5 participants
What: Closes #7812