feat: Improve performance of findBy*/findAllBy* by not generating informative error messages

[timjb]

What: Improve performance of findBy*/findAllBy* by not generating informative error messages in the function passed to waitFor. The work of generating these error messages almost entirely wasted because all of them, except possibly the last one (if the test is red), will be discarded by waitFor. Instead, if the call to waitFor failed, another attempt to find the element(s) is made, this time generating a useful error message if it also fails.

Why: I've noticed that in the codebase I am working on, significant CPU time is spent generating useful error messages that are later discarded. In particular, I've found that in one particular test suite the function prettyDOM takes approximately ~100ms per execution. (The tested React component has a lot of DOM nodes since it embeds an instance of AG Grid.)

How: I've added a second argument to makeFindQuery which is supposed to be a getter function just like the first argument, but may not provide useful error messages in the case of failure, which is used in the call to waitFor. For backwards compatibility I'm using the first argument as a default. If the call to waitFor fails, then the first argument is used instead.

Checklist:

• Documentation added to the
  docs site
• Tests (no new tests necessary)
• TypeScript definitions updated (they don't need updating)
• Ready to be merged
  

Alternative: This PR implements just one possible approach to avoid the unnecessary work of generating informative error messages only for them to be discarded later. Another approach could build on _disableExpensiveErrorDiagnostics (introduced in #590):

• Only generate the potentially expensive parts (e.g. prettyDOM) of error messages if _disableExpensiveErrorDiagnostics=false.
• Adapt waitFor to use a strategy similar as implemented for findBy*/findAllBy* in this PR: First try calling runWithExpensiveErrorDiagnosticsDisabled(callback) and see whether this terminates without an error, but after the timeout occurs try once more without runWithExpensiveErrorDiagnosticsDisabled to get an informative error message in case of failure.

One advantage of this alternate approach would be that this would also decrease CPU usage when using getBy/getAllBy inside waitFor, like in the following code:

await waitFor(() => {
  expect(getByTestId("foo")).toHaveAttribute("href", "https://example.com");
});

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit b81a964:

Sandbox	Source
react-testing-library-examples	Configuration

[timdeschryver]

I like the idea of using then here, but I think we could make this change easier to implement?
Because suggest is an option, I was thinking we could use this parameter, and only enable it on the last try (in the then block).

Also, do you know how much of an impact this has for the execution time/resources?

function makeFindQuery<QueryFor>(
  getter: (
    container: HTMLElement,
    text: Matcher,
    options: MatcherOptions,
  ) => QueryFor,
) {
  return (
    container: HTMLElement,
    text: Matcher,
    options: MatcherOptions,
    waitForOptions: WaitForOptions,
  ) => {
    return waitFor(
      () => {
        // don't suggest while we're trying to find the element
        return getter(container, text, {...options, suggest: false})
      },
      {container, ...waitForOptions},
    ).then(
      success => success,
      rejected => {
        // last try if the user wants a suggestion
        const showSuggestion = getConfig().throwSuggestions
        if (showSuggestion) {
          return getter(container, text, {
            ...options,
            suggest: true,
          })
        }

        return rejected
      },
    )
  }
}

[timjb]

Hi @timdeschryver, thanks for your feedback!

I don't think that using suggestions makes sense, because suggestions behave completely opposite to query error messages:

• You get a suggestion when your query successfully matches at least one element (and you also have throwSuggestions enabled).
• You get an error message when your query matches no elements.

Following your proposal would mean turning suggestions always off except on that last try for the findBy*/findAllBy* queryies, completely ignoring the throwSuggestions setting.

We could of course introduce a new field elaborateErrorMessage to MatcherOptions (first set to false, then to true like suggest) and use that instead to decide how much effort should be put into producing a nice error message. One thing that I do not like about that is that I cannot imagine a user which would want to set this option to false. As a user I would always want the error messages that I get to see to be as detailed and informative as possible and I would also always want the library to not put much effort into producing error messages that I won't get to see. Therefore putting this option into the public API seems wrong to me.

Regarding the impact on execution time/resources: I am not sure how to properly measure that. The impact will also depend strongly on the size of the DOM that is rendered in the test (the larger the DOM the larger the impact). I believe that I could come up with an example where the DOM has a similar size to the case that I've seen at work and this change speeds up the test by 1-5 seconds.

I want to add that there is one additional benefit to this change: It also improves reliability of test cases which render a huge DOM. Suppose that a findBy* query doesn't return a result on the first attempt. Without this change the library would produce an error message. Because of the size of the DOM this will take a considerable amount of time. If this amount of time is longer than the timeout milliseconds, then no second attempt will be made. Therefore the test will succeed or fail depending on whether the first attempt is successful or not. This can easily result in a blinker test. This change fixes this problem by making sure that producing the error message is cheap (until timeout milliseconds have elapsed).

[timdeschryver]

Ah, I see thanks for the elaboration @timjb.
You're right, I was looking at the wrong path.

I would assume that if we go for the "fix" by using then to execute the query for a last time (and with diagnostics) that the option _disableExpensiveErrorDiagnostics can be deprecated and removed in a future version because it won't be as process-heavy as before. This, regardless of the implementation details (using two queries as proposed in this PR, or by adding a new option as with the suggestion, or something else ...). Does this match with your thoughts?

I'm not very familiar with these internals of DTL, but this seems like a reasonable enhancement and a better fix than the original _disableExpensiveErrorDiagnostics because the user gets a faster/better message.
What do you think @eps1lon?

[timjb]

In my opinion, _disableExpensiveErrorDiagnostics shouldn't be removed even after this PR is merged because doing so would negatively impact the performance of the following code:

await waitFor(() => {
    expect(screen.getByRole("checkbox")).toBeEnabled();
});

In the top comment in this PR I have described an alternative approach that builds upon _disableExpensiveErrorDiagnostics rather than abolishing it. This approach would not just improve the performance of code using findBy*/findAllBy*, but of all code that uses element queries inside waitFor, which is why I am in favor of that alternative approach. The reason I didn't implement it right away is that it only occurred to me after I implemented the current version and I wanted to get some feedback first before rewriting it.