Is relying on @testing-library's get* methods to throw and fail a test an anti-pattern?

[sarayourfriend]

I am working on a PR to add eslint-plugin-jest to our ESLint configuration. It has lots of helpful rules and I've caught a handful of tests that really did not have assertions (!!). However, it also emits lots of warnings for tests with no expect that are relying on the get* functions from testing-library to throw if the nothing is found. I've written tests like this, so I understand the rationale, especially because the additional assertion can feel somewhat redundant.

However, as I've been modifying the tests to resolve the ESLint warnings, I've started to wonder whether relying on the get* functions in this way is actually an anti-pattern. On the one hand, it is tempting to do so for the following reasons:

1. The functions throw if they don't find anything
2. Because of that, an assertion like expect(getByText(...)).toBeDefined() feels quite redundant

However, I have the following concerns with this approach that make me think it might be an anti-pattern:

1. It requires a somewhat nuanced knowledge of testing library's behaviour to understand why tests that only call a get* function with no expect are "valid tests". "Valid tests" here meaning that they will fail if the program does not behave in the expected way due to the functions throwing an error. That fact that those functions throw (and that that behaviour is being depended on) isn't made obvious by the API itself (the functions aren't named get*OrThrowIfNotFound). We could fix that by adding comments everywhere but that's just as tedious as a redundant assertion and would require a disable pragma for the eslint rule on each test that did that.
2. It doesn't explicitly say what the test expects. If the test merely expects the presence of an element that matches the selector, the find* or query* methods paired with a .not.toBeNull() assertion is clearer and far more obvious.
3. It is too easy to forget which of the testing-library functions throw. It is too easy to write a test that just calls find* instead of get* with no assertion made. It is also easy for reviewers to get confused or not notice the very subtle discrepancy. Once that gets committed, we suddenly have a test that literally tests nothing (or at least one intended assertion that isn't being run).

@obulat @dhruvkb since y'all work in the frontend the most. Additionally, I'd like to ping @AetherUnbound as someone who is reviewing frontend PRs. I have especially in mind the fact that we should make it as easy as possible to not write incorrect unit tests because we can't rely on expertise and nuanced knowledge of the libraries we use.

[sarayourfriend]

In addition to switching to not use get* as an implicit assertion, I think we should also try to switch to TS for unit tests where we can, especially to catch the difference between the find* and query* methods! I always forget which one returns a Promise and .not.toBeNull() will always pass on a Promise, rendering the test useless.

[obulat]

I agree with your reasoning about the get* methods being an antipattern, @sarayourfriend. The workaround that you used in #2199 with expect.notToThrow is a particularly good example of why we should replace these methods with the more standard query and find and explicitly assert what we expect.

[zackkrida]

Agreed, lets move forward with this 👍

[sarayourfriend]

Thanks y'all!

[sarayourfriend]

Olga and Zack have agreed that this isn't a good approach. I've created six issues to update the modules that use non-explicit assertions based on get*. They are contained in this milestone: https://github.com/WordPress/openverse/milestone/13

Each are medium priority based on the potential for misuse/misunderstanding. The milestone includes a note for why v-tabs is excluded despite the fact that it extends the assertion function names (it does so appropriately). The v-popover.spec.js issue includes a caveat along the same lines.