chore(Alert): updated tests to new standards#8114
Conversation
|
Preview: https://patternfly-react-pr-8114.surge.sh A11y report: https://patternfly-react-pr-8114-a11y.surge.sh |
wise-king-sullyman
force-pushed
the
alert-test-revamp
branch
from
e92fbab to
0516cea
Compare
nicolethoen
left a comment
nicolethoen
left a comment
There was a problem hiding this comment.
so, in the back of my mind I'm aware that right now, the a11y tests to flag a warning that we are putting an aria-label on a div without a valid role. And in the case of alert, the aria-label is the same text as the title of the alert (by default at least). So it'd be considered a breaking change at this point to remove the default aria-label, because of the unit tests in product, and now these unit tests.
I'm not sure what that means for this approach. When/if we implement a fix to avoid the warning, then we'll also need to update these unit tests and I guess that's okay. I just wanted to document the thought process i guess.
And yeah, outside of that, maybe we can make the timeouts shorter whenever possible - like instead of 3000 milliseconds, use 100 milliseconds or something like that?
| await user.hover(alert); | ||
|
|
||
| act(() => { | ||
| jest.advanceTimersByTime(8000); |
There was a problem hiding this comment.
It may be a little less pure, but could we possibly use a shorter timer than the default here? it'd be nice to keep as many timeouts as possible as short as possible in our unit tests.
There was a problem hiding this comment.
I could be mistaken, but I believe that Jest's fake timer functionality here actually allows the timers to advance in non-real time, so the 8000ms here should actually be passing pretty much instantly.
| 'Exclamation triangle icon mock' | ||
| ); | ||
| jest.mock('@patternfly/react-icons/dist/esm/icons/info-circle-icon', () => () => 'Info circle icon mock'); | ||
| jest.mock('@patternfly/react-icons/dist/esm/icons/bell-icon', () => () => 'Bell icon mock'); |
There was a problem hiding this comment.
🤯 good to know this is how it works!
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
This looks great, awesome work on all of this! I had a couple of changes below (should be the first two comments), then a few comments asking the same question. Let me know what you think!
packages/react-core/src/components/Alert/__tests__/AlertToggleExpandButton.test.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/components/Alert/__tests__/AlertToggleExpandButton.test.tsx
Outdated
Show resolved
Hide resolved
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
5 participants
What: Closes #7670
Additional issues:
I also resolved a few small bugs in 749bf18 which I discovered in
AlertActionCloseButtonandAlertToggleExpandButtonwhile building out the test suites.