chore(Dropdown next) add testing #8446
Conversation
|
Preview: https://patternfly-react-pr-8446.surge.sh A11y report: https://patternfly-react-pr-8446-a11y.surge.sh |
dominik-petrik
force-pushed
the
nextDropdownTests
branch
from
48f35ed to
c71aca7
Compare
wise-king-sullyman
left a comment
wise-king-sullyman
left a comment
There was a problem hiding this comment.
🔥 🔥 🔥 really awesome work on this! Just a few small comments.
|
|
||
| const dropdownChildren = <div>Dropdown children</div>; | ||
|
|
||
| test('renders dropdown', async () => { |
There was a problem hiding this comment.
It seems like all of the non userEvent tests shouldn't need to be async?
| test('renders passed toggle element', async () => { | ||
| render(<Dropdown toggle={toggleRef => toggle(toggleRef)}>{dropdownChildren}</Dropdown>); | ||
|
|
||
| expect(await screen.findByRole('button')).toBeVisible(); |
There was a problem hiding this comment.
| expect(await screen.findByRole('button')).toBeVisible(); | |
| expect(await screen.findByRole('button', { name: 'Dropdown' })).toBeVisible(); |
Nit: I would probably add the name option here. Just so that we know the test isn't a false positive, since we can't do that the typical way because of toggle being a required prop.
| test('passes zIndex to popper', async () => { | ||
| render( | ||
| <Dropdown zIndex={100} toggle={toggleRef => toggle(toggleRef)}> | ||
| {dropdownChildren} | ||
| </Dropdown> | ||
| ); | ||
|
|
||
| expect(await screen.findByText('zIndex: 100')).toBeVisible(); | ||
| }); | ||
|
|
||
| test('passes isOpen to popper', async () => { | ||
| render( | ||
| <Dropdown isOpen toggle={toggleRef => toggle(toggleRef)}> | ||
| {dropdownChildren} | ||
| </Dropdown> | ||
| ); | ||
|
|
||
| expect(await screen.findByText('isOpen: true')).toBeVisible(); | ||
| }); | ||
|
|
||
| test('passes onSelect callback', async () => { | ||
| const user = userEvent.setup(); | ||
|
|
||
| const onSelect = jest.fn(); | ||
| render( | ||
| <Dropdown onSelect={onSelect} toggle={toggleRef => toggle(toggleRef)}> | ||
| {dropdownChildren} | ||
| </Dropdown> | ||
| ); | ||
|
|
||
| const trigger = await screen.findByText('Mock item'); | ||
| await user.click(trigger); | ||
|
|
||
| expect(onSelect).toBeCalledTimes(1); | ||
| }); |
There was a problem hiding this comment.
Can you add default behavior tests for these props?
| /* test('onOpenChange is not called when not passed', async () => { | ||
| const user = userEvent.setup(); | ||
| const onOpenChange = jest.fn(); | ||
|
|
||
| render( | ||
| <Dropdown isOpen={true} toggle={toggleRef => toggle(toggleRef)}> | ||
| {dropdownChildren} | ||
| </Dropdown> | ||
| ); | ||
|
|
||
| const dropdown = await screen.findByRole('button'); | ||
| await user.click(dropdown); | ||
| await user.click(document.body); | ||
|
|
||
| await user.click(dropdown); | ||
| await user.keyboard('{Tab}'); | ||
|
|
||
| await user.click(dropdown); | ||
| await user.keyboard('{Escape}'); | ||
|
|
||
| expect(onOpenChange).not.toBeCalled(); | ||
| }); */ |
There was a problem hiding this comment.
Rather than have this test commented out I would use test.skip, makes it less likely to be forgotten.
|
|
||
| expect(screen.getByText('description: Test description')).toBeVisible(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Is there a particular reason that you didn't test itemId?
|
|
||
| test('matches snapshot', () => { | ||
| const { asFragment } = render( | ||
| <DropdownItem className="custom-class" description="Test description"> |
There was a problem hiding this comment.
You'll probably want to add a ouiaId prop here.
| /* | ||
| pressing enter key on a button calls a click event internally | ||
| so testing for a button click should be suficitient | ||
| */ |
There was a problem hiding this comment.
Thanks for adding this note!
There was a problem hiding this comment.
| /* | |
| pressing enter key on a button calls a click event internally | |
| so testing for a button click should be suficitient | |
| */ | |
| /* | |
| pressing enter or space key on a button calls a click event internally | |
| so testing for a button click should be sufficient | |
| */ |
There is one difference between clicking via mouse/touch and "clicking" via keyboard, that being with keyboard Enter/Space the first non-disabled dropdown item should receive focus. I don't believe that's something we can test via Cypress, though, since it's reliant on the detail property of the event object.
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
This is looking nice! Other than Austin's comments, had a couple more below
packages/react-integration/cypress/integration/dropdownnext.spec.ts
Outdated
Show resolved
Hide resolved
| /* | ||
| pressing enter key on a button calls a click event internally | ||
| so testing for a button click should be suficitient | ||
| */ |
There was a problem hiding this comment.
| /* | |
| pressing enter key on a button calls a click event internally | |
| so testing for a button click should be suficitient | |
| */ | |
| /* | |
| pressing enter or space key on a button calls a click event internally | |
| so testing for a button click should be sufficient | |
| */ |
There is one difference between clicking via mouse/touch and "clicking" via keyboard, that being with keyboard Enter/Space the first non-disabled dropdown item should receive focus. I don't believe that's something we can test via Cypress, though, since it's reliant on the detail property of the event object.
dominik-petrik
force-pushed
the
nextDropdownTests
branch
from
ddafffd to
9323963
Compare
|
Can you make the base of you PR "v5" instead of "main" please. Main is now feature frozen. |
5 participants
What: Closes #7964
Additional issues: #8479