Components: Refactor Button tests to @testing-library/react
#42981
Conversation
tyxla
added
[Type] Enhancement
|
Size Change: +3.78 kB (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
| jest.mock( '../../icon', () => () => <div data-testid="test-icon" /> ); | ||
| jest.mock( '../../tooltip', () => ( { text, children } ) => ( | ||
| <div data-testid="test-tooltip" title={ text }> | ||
| { children } | ||
| </div> | ||
| ) ); | ||
| jest.mock( '../../visually-hidden', () => ( { | ||
| __esModule: true, | ||
| VisuallyHidden: ( { children } ) => ( | ||
| <div data-testid="test-visually-hidden">{ children }</div> | ||
| ), | ||
| } ) ); |
There was a problem hiding this comment.
Ah I hadn't really thought of mocking components! 😆 TIL
There was a problem hiding this comment.
Yeah, it's a nice way to separate concerns and make sure that we're not stepping into the testing grounds of another component.
There was a problem hiding this comment.
FWIW I've left the icon mock because I think it helps reduce noise in the tests and actually improves the test quality, vs trying to figure out how to query an SVG that's aria-hidden.
There was a problem hiding this comment.
Not sure if this is the most correct conversation to reply to, but for reference this is a convo that we had on the difficulties when testing VisuallyHidden:, where we ended up using snapshot testing for it: #42403 (comment)
And this is a conversation where we used the selector option to make sure that the queried element had a certain aria attribute : #42403 (comment)
Just in case they can be useful for this PR too!
| const button = render( <Button /> ).container.firstChild; | ||
|
|
||
| expect( button ).toHaveClass( 'components-button' ); | ||
| expect( button ).not.toHaveClass( 'is-large' ); | ||
| expect( button ).not.toHaveClass( 'is-primary' ); | ||
| expect( button ).not.toHaveClass( 'is-pressed' ); | ||
| expect( button ).not.toHaveAttribute( 'disabled' ); | ||
| expect( button ).not.toHaveAttribute( 'aria-disabled' ); | ||
| expect( button ).toHaveAttribute( 'type', 'button' ); | ||
| expect( button.tagName ).toBe( 'BUTTON' ); |
There was a problem hiding this comment.
I think something like this would be more in line with how we want to write tests in the components package:
| const button = render( <Button /> ).container.firstChild; | |
| expect( button ).toHaveClass( 'components-button' ); | |
| expect( button ).not.toHaveClass( 'is-large' ); | |
| expect( button ).not.toHaveClass( 'is-primary' ); | |
| expect( button ).not.toHaveClass( 'is-pressed' ); | |
| expect( button ).not.toHaveAttribute( 'disabled' ); | |
| expect( button ).not.toHaveAttribute( 'aria-disabled' ); | |
| expect( button ).toHaveAttribute( 'type', 'button' ); | |
| expect( button.tagName ).toBe( 'BUTTON' ); | |
| render( <Button /> ); | |
| const button = screen.getByRole( 'button' ); | |
| expect( button ).toHaveClass( 'components-button' ); | |
| expect( button ).not.toHaveClass( 'is-large' ); | |
| expect( button ).not.toHaveClass( 'is-primary' ); | |
| expect( button ).not.toHaveClass( 'is-pressed' ); | |
| expect( button ).not.toBeDisabled(); | |
| expect( button ).toHaveAttribute( 'type', 'button' ); |
The differences being:
- We try not to use
container.firstChild. This follows official RTL guidance. In this case, it isn't guaranteed that the first child will always be thebutton(could bedivor whatever). tagNameassertions can usually be covered with thescreenquery.- Using a more abstracted jest-dom matcher (
toBeDisabled()) can often be more robust than trying to check specific attributes (disabled/aria-disabled). (It might be that some of the tests in this file do actually need to check specific aria attributes, and that's ok.)
There was a problem hiding this comment.
We try not to use
container.firstChild. This follows official RTL guidance. In this case, it isn't guaranteed that the first child will always be the button (could be div or whatever).
I might be misunderstanding, but I think that rule is talking about something else. The rule you're linking is talking about how we run get and query queries, namely, it suggests that we always call them from screen:
// ❌
const {getByRole} = render(<Example />)
const errorMessageNode = getByRole('alert')
// ✅
render(<Example />)
const errorMessageNode = screen.getByRole('alert')
This doesn't have anything to do with container.firstChild. I actually think that for our test it's valuable to assert that the root element is the button, and if that changes, the test should break.
There is this "common mistake" that is closer to what we're doing here:
// ❌
const {container} = render(<Example />)
const button = container.querySelector('.btn-primary')
expect(button).toHaveTextContent(/click me/i)
// ✅
render(<Example />)
screen.getByRole('button', {name: /click me/i})
but it talks specifically against using container.querySelector() and container.querySelectorAll() which we are consciously avoiding here anyway.
The only reason to avoid this would be if we followed the testing-library/no-node-access ESLint rule, which we currently aren't. And even if we did, this rule does have a allowContainerFirstChild option that allows us to use container.firstChild if we want to.
Sorry for the wall of text 😉 Just wanted to share some of why I've been using that and I've been finding it useful. Sometimes it's just predictable to access the root element of the component, without having to seek a special way to locate it with a query.
I'm happy to always use screen queries, but then, this has its downsides sometimes, especially in components where we have nested elements that match the top elements, or recursive structures.
Just wanted to hear more of what you think about all this.
tagName assertions can usually be covered with the screen query.
I'm not sure that's always the case, though - screen.getByRole( 'button' ) will also find input[type='button'], which is definitely unwanted. The tagName assertion seems more precise to me in this scenario. WDYT?
Using a more abstracted jest-dom matcher (toBeDisabled()) can often be more robust than trying to check specific attributes
Are you saying that toBeDisabled() is enough for us and is preferred to testing both the disabled and aria-disabled attributes separately?
There was a problem hiding this comment.
I might be misunderstanding, but I think that rule is talking about something else.
Ah sorry, I was referring to this part:
The only exception to this is if you're setting the
containerorbaseElementwhich you probably should avoid doing (I honestly can't think of a legitimate use case for those options anymore and they only exist for historical reasons at this point).
I actually think that for our test it's valuable to assert that the root element is the button, and if that changes, the test should break.
The question is — why should the test break if the root element is a div? Would it break behavior? No. Would it break styling? Maybe. So if we're trying to test for styling breakage with RTL, we're kind of using the wrong tool. Does it warrant a DOM snapshot then? I'd say no in this case.
Even stuff like .toHaveClass( 'is-foo' ) is not something we generally do, since it's testing an implementation detail. Ideally, it's something for a visual regression test.
I'm not sure that's always the case, though -
screen.getByRole( 'button' )will also findinput[type='button'], which is definitely unwanted. ThetagNameassertion seems more precise to me in this scenario. WDYT?
We usually have no reason to distinguish between implementations as long as they are accessible, and this is what I understand to be the central ethos of RTL. It's why the first choice of query is getByRole, and there is no getByTagName query or toHaveTagName assertion. So if there is a concrete reason that it has to be a tagName=BUTTON, I would first try to assert for that behavior (e.g. can I pass HTML as children?), and if that isn't possible, add a comment on why it needs to be a <button>.
Are you saying that
toBeDisabled()is enough for us and is preferred to testing both thedisabledandaria-disabledattributes separately?
Apologies, I was under the impression that toBeDisabled checks both, but I guess it doesn't! So in a component like this that needs to specifically check for aria-disabled behavior because of the "focusable disabled" stuff, I understand it makes sense to assert separately and specifically.
There was a problem hiding this comment.
This has been very helpful, thank you ❤️ This helped a lot to clear some confusion in my head and to deeply understand the tradeoffs we're making when using this library.
I've updated the tests to reflect your suggestions - namely, no container usage, no DOM traversing and using screen queries instead.
| render( <Button icon={ plusCircle } label="WordPress" /> ); | ||
|
|
||
| const tooltip = screen.getByTestId( 'test-tooltip' ); | ||
| expect( tooltip ).toBeVisible(); | ||
| expect( tooltip ).toHaveAttribute( 'title', 'WordPress' ); | ||
|
|
||
| const button = screen.getByRole( 'button' ); | ||
| expect( tooltip ).toContainElement( button ); | ||
| expect( button ).toHaveAttribute( 'aria-label', 'WordPress' ); |
There was a problem hiding this comment.
I'm hoping we can manage without mocking Tooltip. For example, this is working for me:
| render( <Button icon={ plusCircle } label="WordPress" /> ); | |
| const tooltip = screen.getByTestId( 'test-tooltip' ); | |
| expect( tooltip ).toBeVisible(); | |
| expect( tooltip ).toHaveAttribute( 'title', 'WordPress' ); | |
| const button = screen.getByRole( 'button' ); | |
| expect( tooltip ).toContainElement( button ); | |
| expect( button ).toHaveAttribute( 'aria-label', 'WordPress' ); | |
| render( <Button icon={ plusCircle } label="WordPress" /> ); | |
| const button = screen.getByRole( 'button', { name: 'WordPress' } ); | |
| button.focus(); | |
| expect( screen.getByText( 'WordPress' ) ).toBeInTheDocument(); |
There was a problem hiding this comment.
You definitely have a point here. However, the problem with this type of testing is that we're not sure where WordPress appeared. It could have been displayed from the Tooltip component from its title prop, but it could also have been displayed from another prop. How can we be sure? What you're suggesting simply checks if the text is there, and doesn't care "how" it was displayed. IMHO it also bleeds into the implementation details of a component that we're not interested to test here. Whether Tooltip displays its title prop somewhere or not, should be subject to its own tests, no? I'm curious to hear what you think about this.
There was a problem hiding this comment.
Yes, I see your point, too. And together with the mock approach, I find it interesting how it's a true "unit test", in a way that I've never seen in React component testing. (I don't have a lot of Enzyme experience actually!) I guess the weakness here is the unreliability of the mock (the way all mocks are). Realistically, I would want my test to fail if the Tooltip API changed or something and Button stopped showing tooltips. But a mocked test wouldn't fail.
IMHO it also bleeds into the implementation details of a component that we're not interested to test here. Whether
Tooltipdisplays itstitleprop somewhere or not, should be subject to its own tests, no?
It's funny because I would say the mocked test is "testing implementation details" 😆 From the RTL mindset, we want to test that, even in icon button cases where the button doesn't have a visible label, the button is accessible by that label, and a visible label text is in the DOM when the button is focused or hovered. That's it. We don't care whether it even uses the Tooltip component as the implementation. (Not saying that it's inherently unuseful to test that a function calls another function with the correct arguments. It's just not what RTL is designed for. Even in the FAQ that discusses avoiding mocks, the exception given as an example is a mock meant for bypassing, not for testing arguments.)
Would it be more palatable if we added a expect( screen.getByText( 'WordPress' ) ).not().toBeInTheDocument(); assertion before the button.focus()?
There was a problem hiding this comment.
I've revisited this and agree with you, so decided to remove the mocks where possible. I've gone that way and now the tests look much better and clearer - especially when we're checking for the tooltip to be hidden first, then after focusing on the button, we're asserting that it's visible.
There was a problem hiding this comment.
Just going to add my point of view to a super interesting conversation.
I am basically aligned with Lena's point of view here — I usually try to avoid mocking components as much as possible, since the spirit of RTL to feels closer to writing integration tests of the component and its dependencies, rather than an isolated unit test (à la Enzyme's shallow render)
I believe that such tests can be much more valuable and can bring a lot more confidence than a strict unit test. To me, the need for a mock is almost a bit of a "code smell"
There was a problem hiding this comment.
Thanks for sharing your point of view. I definitely agree with you all here on most points. I disagree on the "code smell" part though - it's not a code smell IMHO. Rather, we're making a trade-off to test as close as the user experiences the component, in favor of stepping out of the testing responsibility of the current component. You are saying mocks are a code smell, others would say that testing other components is a code smell. I say it's a trade-off, and as long as we're making a conscious decision and we've weighed the trade-offs of it, we're good to go.
| const button = render( <Button describedBy="Description text" /> ) | ||
| .container.firstChild; | ||
|
|
||
| expect( button ).toHaveAttribute( 'aria-describedby' ); | ||
| expect( | ||
| screen.getByTestId( 'test-visually-hidden' ) | ||
| ).toHaveTextContent( 'Description text' ); |
There was a problem hiding this comment.
Something like this would be more robust, and eliminate the need for a VisuallyHidden mock:
| const button = render( <Button describedBy="Description text" /> ) | |
| .container.firstChild; | |
| expect( button ).toHaveAttribute( 'aria-describedby' ); | |
| expect( | |
| screen.getByTestId( 'test-visually-hidden' ) | |
| ).toHaveTextContent( 'Description text' ); | |
| render( <Button describedBy="Description text" /> ); | |
| const button = screen.getByRole( 'button', { | |
| description: 'Description text', | |
| } ); | |
| expect( button ).toBeInTheDocument(); |
There was a problem hiding this comment.
Updated to use your suggestion here - it's a really great example why mocks are not recommended.
|
@mirka thanks a bunch for your feedback 🙌 I've left a couple of questions on it and wanted to pick your brain and hear your thoughts, before I start addressing the feedback. Let me know what you think 🧠 |
|
I enjoy these discussions very much, thank you 😊 |
tyxla
force-pushed
the
refactor/button-tests-rtl
branch
from
796ab8b
to
e2a3a98
Compare
Me too, it's been particularly helpful and I appreciate you taking the time to go deep into that level of detail 😍 |
| it( 'should render with an additional className', () => { | ||
| const button = shallow( <Button className="gutenberg" /> ).find( | ||
| 'button' | ||
| ); | ||
| render( <Button className="gutenberg" /> ); | ||
|
|
||
| expect( button.hasClass( 'gutenberg' ) ).toBe( true ); | ||
| expect( screen.getByRole( 'button' ) ).toHaveClass( 'gutenberg' ); |
There was a problem hiding this comment.
I think that it's a good practice to also check for the existence of the base class (.components-button) in such scenario. It happened to me before, where the className prop was implemented in a way that would wipe out any other class when passed. Checking for the base class would make sure that something like this doesn't happen!
There was a problem hiding this comment.
Beautiful! It looks a lot more RTL-like now, thank you ✨ I like how you put it: "tradeoffs". I think RTL's opinionated guardrails make us laser focus on testing the HTML foundations reliably, so we're kind of forced to use better-suited tools to test things that fall outside of those guardrails (snapshot, vizreg, etc). I'm hoping we can some day add vizreg infrastructure so we can reliably test styles as well.
We'll also be looking into improving the Tooltip component so it's a bit more semantic and not just a random string in the DOM 🙈
What?
We've recently started refactoring
enzymetests to@testing-library/react.This PR refactors the
<Button />component tests fromenzymeto@testing-library/react.Why?
@testing-library/reactprovides a better way to write tests for accessible components that is closer to the way the user experiences them.How?
We're straightforwardly replacing
enzymetests with@testing-library/reactones, usingjest-dommatchers and mocks to avoid testing unrelated implementation details.Testing Instructions
Verify tests pass:
npm run test-unit packages/components/src/button/test/index.jsScreenshots or screencast