WordPress / openverse-frontend

The gateway to the Openverse. Openverse is a search tool for CC-licensed and public domain content across the internet.
https://wordpress.org/openverse
MIT License
72 stars 64 forks source link

[Feature] Explore using testing-library instead of the custom render function #139

Closed sarayourfriend closed 2 years ago

sarayourfriend commented 3 years ago

Problem

The custom render function is fine and works great, but testing-library establishes consistent patterns for testing with accessibility in mind. For example, rather than using just any selector to find elements in a test container, you're encouraged (via the API) to retrieve elements by accessible attributes like role or the text value of an element. Instead of retrieving elements by an fake testing id for example, you would select the element via its role or by the rendered text. This is all made simple and natural through the screen utility that testing-library exports.

This makes our tests work much more closely to how people actually use our components (navigating by role and text on the screen/being read out).

testing-library also has a userEvent utility that can more closely simulate user triggered events. For example, a click event is also accompanied by a mousedown and mouseup event, but calling wrapper.trigger('click') does not simulate that fact. userEvent.click does.

Alternatives

Just sticking with the current implementation but focusing on using accessible attributes for selecting elements. I think this is fine but is harder to enforce and teach about. Whereas testing-library already has these concepts built into it making it more natural to follow.

Additional context

I don't think we'd need to just replace all the existing tests with ones written in testing-library. It could be a slow process to convert all the existing tests to use it, but it doesn't have to happen all at once if we decided to go with this approach.

testing-library is a thin wrapper around the @vue/test-utils, just like the current render function we have. In that way, the API for the render function would remain the same, but the return value would be different, instead returning the testing-library container AND rendering the actual HTML into JSDom, rather than acting over the faked out Vue instances. This, again, makes our tests more closely match how they're actually used by users when they're rendered into a page.

Implementation

obulat commented 3 years ago

I think this would be great for accessibility for the end users, and for developers to take accessibility into consideration in every change we make. I'm very excited to try testing-library!

dhruvkb commented 3 years ago

@sarayourfriend does #185 also close this? Alternatively, could we update this ticket to track the integration of testing-library into all tests?

sarayourfriend commented 3 years ago

@dhruvkb what do you think is best? If we're okay moving forward with testing-library then perhaps a separate issue to track the integration into all of the tests would be best? It'll require some extra work to re-write all those tests but it could be worth it if we want to consolidate around a single solution for testing.

On the other hand, if those tests already work, we could just recommend testing-library moving forward instead of retroactively refactoring everything to a different library.

What do you think?

dhruvkb commented 3 years ago

We clearly agree about using testing-library for all component tests moving forward. It makes sense to use a library that's built from the ground up around an accessibility-focused mindset.

For the existing tests, I think it's good to move them to testing-library gradually to both ensure good a11y and, as you put it, to "consolidate around a single solution for testing". We're about to go into a design overhaul soon so I'd recommend all PRs touching an existing component to migrate the tests as well?

When we have consensus from @obulat and @krysal, we should close this issue.

sarayourfriend commented 3 years ago

Just wanted to note that there are some limitations to testing-library that we'd have to overcome somehow. For example, testing-library doesn't expose the vm property from the @vue/test-utils wrapper, meaning we can't use workarounds like in this test suite for special Nuxt properties:

https://github.com/WordPress/openverse-frontend/blob/eff5cc6b0e50282720f737dd5ff1890f678a1188/test/unit/specs/components/download-button.spec.js#L43-L47

For the meantime, we won't be able to fully move away from using @vue/test-utils but most test suites should be convertable I think 🤞

krysal commented 3 years ago

I totally agree with this move. I've been reading the library docs these days and its guiding principle makes so much sense:

The more your tests resemble the way your software is used, the more confidence they can give you.

Updating each component whenever it's possible to use it seems right to me, a progressive change while also we get to know the methods it provides and the best way to use them.

zackkrida commented 2 years ago

Closing this, as we've successfully switched to testing-library for most of our new tests.