chore(Checkbox): updated tests #9756
Conversation
|
Preview: https://patternfly-react-pr-9756.surge.sh A11y report: https://patternfly-react-pr-9756-a11y.surge.sh |
packages/react-core/src/components/Checkbox/__tests__/Checkbox.test.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/components/Checkbox/__tests__/Checkbox.test.tsx
Outdated
Show resolved
Hide resolved
| test('Does not render a label by default', () => { | ||
| render(<Checkbox id="test-id" />); | ||
|
|
||
| expect(screen.queryByLabelText('test label')).not.toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
This would apply to the description and body tests as well, but I'm always unsure of what tests like this are expecting just because technically they're passing, but they'd also pass even if label="another test label" were passed in. So less that a label doesn't render and more that a label with this specific test doesn't render.
At the same time I'm not sure how pretty an alternative would be. Using something like getByRole(checkbox).parentElement.querySelector(.${styles.checkLabel}) may work but is lengthy.
Probably splitting hairs too much so I don't think I'd really block over it, but curious what you think.
There was a problem hiding this comment.
Yeah I don't have a hardline stance on this and happy to remove if you think they're unnecessary, but my reasoning for doing these kinds of tests is that it ensure that assertion in the following test (where we expect the label to exist) is actually valid and not giving a false positive.
There was a problem hiding this comment.
Since it'd accept a regex, what about expect(screen.queryByLabelText(/\w+/)).not.toBeInTheDocument();? We don't really care what the label text actually is, just whether it's rendered, so this would result in a failure if any sort of label text were to be found (rather than currently a failure only occurs if label text that is "test label" is found). Probably wouldn't work for the body and description tests, though.
There was a problem hiding this comment.
Yeah I'm not against that idea at all.
….test.tsx Co-authored-by: Eric Olkowski <70952936+thatblindgeye@users.noreply.github.com>
There was a problem hiding this comment.
Regarding the convo above, I'd agree about keeping those tests in, I think it's more how we're testing for things for me. I'm also not sure if some alternatives I mentioned above (or using snapshots) are much better, so the above isn't a blocker for me and we can always revisit it if need be.
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
What: Closes #9529
Additional issues: