Migrate ArticlePagination to RTL. #824
Conversation
|
Demo starting at https://react-components-824.demos.haus |
| <ArticlePagination | ||
| nextLabel="Consectetur adipisicing elit" | ||
| nextURL="#next" | ||
| previousLabel="Lorem ipsum dolor sit amet" | ||
| previousURL="#previous" | ||
| /> | ||
| ); | ||
| expect(wrapper).toMatchSnapshot(); | ||
| expect(screen.getByRole("contentinfo")).toMatchSnapshot(); |
There was a problem hiding this comment.
One thing that I don't like about RTL is selectors like this.
How do I know that "ArticlePagination" is has contentinfo role? It's nowhere in the component code. My guess is it has this role because it's a "footer" element. But using querySelector("footer") would at least be explicit about it. In here we are pretending to be using semantic selector (because RTE forces us to it), but all we have is an indirect node query with confusing role that no one will know where it comes from.
Maybe for the cases like this (when we need root element of the component) we should have some testing ID convention in place? So all components will have some testing id on root element?
Thoughts @huwshimi @petermakowski ?
There was a problem hiding this comment.
Seems like there is a way to get to the container element at least: https://testing-library.com/docs/react-testing-library/api/#container-1
so when doing snapshots maybe we could use that instead of getting root element via implicit role?
const { container } = render(…);
expect(container).toMatchSnapshot();
Still, it won't solve the problem if we really want to access the root element of the component (and check class names on it or something).
There was a problem hiding this comment.
In here we are pretending to be using semantic selector (because RTE forces us to it), but all we have is an indirect node query with confusing role that no one will know where it comes from.
Testing library operates on the same roles that are exposed to the user. You can check the role of an element by using logRoles.
Alternatively you can navigate the UI using a screen reader or inspect using developer tools and you will get the same results.
Maybe for the cases like this (when we need root element of the component) we should have some testing ID convention in place? So all components will have some testing id on root element?
No need to use a test id to get the root element of the component, you can use container.firstChild.
https://testing-library.com/docs/react-testing-library/api/#container-1
There was a problem hiding this comment.
The downside of using the container rule is that at the moment we'd have to disable both the no container and no dom access linting rules for each test that we use it in.
I'll fix this PR up with whatever we decide in: #847.
There was a problem hiding this comment.
I'll approve and merge it as is for now, not to block RTL migration any longer.
But once we have all the tests in place would be good to get back, decides any rules/good practices and follow up according to them.
Done
QA
n/a