-
Notifications
You must be signed in to change notification settings - Fork 353
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
chore(ChipGroup): update tests #9547
Conversation
Preview: https://patternfly-react-pr-9547.surge.sh A11y report: https://patternfly-react-pr-9547-a11y.surge.sh |
packages/react-core/src/components/Chip/__tests__/ChipGroup.test.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/components/Chip/__tests__/ChipGroup.test.tsx
Outdated
Show resolved
Hide resolved
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.
Some comments below, additionally can we add tests for the following:
- passing a custom className works
- checking the chip group has an aria-label by default when
categoryName
is not passed - passing a custom aria-label works
- a basic snapshot test
- check that mocked functions are called for the onClick and onOverflowChipClick props
test('chip group with category and tooltip', () => { | ||
render( | ||
<ChipGroup categoryName="A very long category name"> | ||
<Chip>1.1</Chip> | ||
</ChipGroup> | ||
); | ||
expect(screen.getByText('A very long category name')).toBeVisible(); |
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.
This would probably be better as an integration test to check that hovering/focusing the category name triggers the Tooltip. Right now I don't believe it'll really test whether the tooltip is visible/working; we could possibly try mocking the tooltip similar to what the 3rd(?) to last Truncate test is doing by checking that the text position: top
is rendered.
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.
Good point @thatblindgeye, I do agree this would be better suited as an integration test. The method the Truncate test uses to mock the tooltip doesn't seem to work here.
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.
Latest updates look great, thanks for making them! Just had a few more quick comments below
packages/react-core/src/components/Chip/__tests__/ChipGroup.test.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/components/Chip/__tests__/ChipGroup.test.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/components/Chip/__tests__/ChipGroup.test.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Eric Olkowski <[email protected]>
What: Closes #9531