Skip to content
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

Merged
merged 7 commits into from
Oct 27, 2023
Merged

Conversation

jenny-s51
Copy link
Contributor

What: Closes #9531

@jenny-s51 jenny-s51 changed the title chore(ChipGroup): test revamp chore(ChipGroup): update tests Aug 29, 2023
@patternfly-build
Copy link
Contributor

patternfly-build commented Aug 29, 2023

Copy link
Contributor

@thatblindgeye thatblindgeye left a 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

Comment on lines 61 to 67
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();
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

@thatblindgeye thatblindgeye left a 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

jenny-s51 and others added 3 commits October 27, 2023 11:45
@thatblindgeye thatblindgeye merged commit a5e0326 into patternfly:main Oct 27, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Chip group test revamp
4 participants