WordPress / openverse

Openverse is a search engine for openly-licensed media. This monorepo includes all application code.
https://openverse.org
MIT License
244 stars 195 forks source link

Update e2e tests to not rely on specific text copy, instead use translation tokens #619

Closed sarayourfriend closed 1 month ago

sarayourfriend commented 2 years ago

Problem

Take this test as an example:

https://github.com/WordPress/openverse-frontend/blob/26af972f90f3f9b8081131d8c61a2aefb8329991/test/e2e/search.spec.js#L29

If we try to run it in another language that is translated, it will fail. But we shouldn't have to write unique tests for every language we'd like to test. Likewise, if the copy is changed then the test will fail. This is even worse for languages outside of the token language of English because the copy is controlled externally and can change at any time, thus causing the e2e tests to fail if we write them against another language (which we should, for example, write them for RTL languages and other non-latin alphabet languages).

Description

Create some method for the e2e tests to only rely on static tokens for the selectors rather than actual page copy that can change arbitrarily (when compared to the behavior, which should stay the same unless intentionally changed and should not change due to a simple copy update).

Using the translation token might be way around this but I'm not sure how that would work for RTL languages or testing non-latin alphabet languages.

Alternatives

Use data-test-id style selectors instead of copy or tokens. Maybe that's better when there aren't any other accessibility based selectors to use.

Additional context

Implementation

zackkrida commented 2 years ago

It'd be great to document somewhere a specific hierarchy of selector quality for tests. For example, use aria role, then use other aria properties, then use label text, and so on.

Additionally, could we write a utility that gets the correct localized strings from our translation files for the purposes of testing? For example, the above would become:

await expect(page.locator(`text=${magicFakei18nHelper('result-relevancy-key')}`)).toHaveCount(1)
krysal commented 2 years ago

I will still prefer the text copy over the data-test-id, as the Testing Library says:

Outside of forms, text content is the main way users find elements

Their docs have exactly what you're asking for @zackkrida: https://testing-library.com/docs/queries/about#priority

If the text changes most likely the component interaction will change too. If it works for English we can assume it will also work for other languages, or most likely it will require a specific test setup, so I don't think we need to make this change general.

zackkrida commented 2 years ago

https://testing-library.com/docs/queries/about/#priority

Ah, great find @krysal! I was looking in the 'Guiding principles' section but couldn't find this. Awesome.

sarayourfriend commented 2 years ago

Makes sense! I really don't like the data-test-id approach anyway.

The helper Zack mentioned seems like it would be nice to have though, in particular if we wanted to be able to run the same test against RTL locales (especially because visual regression testing will likely rely on the same host of e2e test utilities for navigating the UI).

zackkrida commented 2 years ago

I wonder if there's a vue-i18n internal helper we could borrow for this:

https://github.com/intlify/vue-i18n-next/tree/master/packages

sarayourfriend commented 4 months ago

I'm combing through older issues, and wanted to see if this was still needed. A lot of the places we select by text in the e2e tests use regexs instead of strings, which took me a while to realise.

There are a lot of examples like this:

https://github.com/WordPress/openverse/blob/1f6be8289d095570fb859ada435e6711f4ca7151/frontend/test/playwright/e2e/all-results-analytics.spec.ts#L105

We can replace these with the t utility from test/playwright/utils/i18n.

sarayourfriend commented 2 months ago

@obulat is this issue still needed? I wasn't sure 🤔

obulat commented 2 months ago

I was planning to audit the tests and either create a PR or close it. Hope to get this done in the next 2 weeks