Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c49b867
feat: Improve performance of findBy*/findAllBy* by not generating inf…
timjb Oct 31, 2021
b81a964
Merge branch 'main' into pr/improve-findBy-performance
eps1lon Jan 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions src/__tests__/role.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import {configure, getConfig} from '../config'
import {getQueriesForElement} from '../get-queries-for-element'
import {waitFor} from '../wait-for'
import {render, renderIntoDocument} from './helpers/test-utils'

test('by default logs accessible roles when it fails', () => {
Expand Down Expand Up @@ -356,11 +357,11 @@ test('does not include the container in the queryable roles', () => {
})

test('has no useful error message in findBy', async () => {
const {findByRole} = render(`<li />`)
const {getByRole} = render(`<li />`)

await expect(findByRole('option', {timeout: 1})).rejects.toThrow(
'Unable to find role="option"',
)
await expect(
waitFor(() => getByRole('option'), {timeout: 1}),
).rejects.toThrow('Unable to find role="option"')
})

test('explicit role is most specific', () => {
Expand Down
22 changes: 22 additions & 0 deletions src/queries/label-text.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,12 +191,34 @@ const findAllByLabelText = makeFindQuery(
// @ts-expect-error -- See `wrapAllByQueryWithSuggestion` Argument constraint comment
[labelText: Matcher, options?: SelectorMatcherOptions]
>(getAllByLabelText, getAllByLabelText.name, 'findAll'),
wrapAllByQueryWithSuggestion(
(container, text, options) => {
const elements = queryAllByLabelText(container, text, options)
if (elements.length === 0) {
throw new Error('no element found')
}
return elements
},
getAllByLabelText.name,
'findAll',
),
)
const findByLabelText = makeFindQuery(
wrapSingleQueryWithSuggestion<
// @ts-expect-error -- See `wrapAllByQueryWithSuggestion` Argument constraint comment
[labelText: Matcher, options?: SelectorMatcherOptions]
>(getByLabelText, getAllByLabelText.name, 'find'),
wrapSingleQueryWithSuggestion(
(container, text, options) => {
const elements = queryAllByLabelText(container, text, options)
if (elements.length !== 1) {
throw new Error('no element or more than one elements found')
}
return elements[0]
},
getAllByLabelText.name,
'find',
),
)

const getAllByLabelTextWithSuggestions = wrapAllByQueryWithSuggestion<
Expand Down
39 changes: 37 additions & 2 deletions src/query-helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,11 +118,16 @@ function makeGetAllQuery<Arguments extends unknown[]>(
// this accepts a getter query function and returns a function which calls
// waitFor and passing a function which invokes the getter.
function makeFindQuery<QueryFor>(
getter: (
getterWithInformativeErrorMessage: (
container: HTMLElement,
text: Matcher,
options: MatcherOptions,
) => QueryFor,
getterWithQuickToGenerateErrorMessage: (
container: HTMLElement,
text: Matcher,
options: MatcherOptions,
) => QueryFor = getterWithInformativeErrorMessage,
) {
return (
container: HTMLElement,
Expand All @@ -132,9 +137,17 @@ function makeFindQuery<QueryFor>(
) => {
return waitFor(
() => {
return getter(container, text, options)
// Don't waste time generating an informative error message since they will be discarded
// anyway.
return getterWithQuickToGenerateErrorMessage(container, text, options)
},
{container, ...waitForOptions},
).then(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
      },
    )
  }
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

x => x,
() => {
// Try one last time, this time generating an informative error message in case of failure
return getterWithInformativeErrorMessage(container, text, options)
},
)
}
}
Expand Down Expand Up @@ -242,9 +255,31 @@ function buildQueries(

const findAllBy = makeFindQuery(
wrapAllByQueryWithSuggestion(getAllBy, queryAllBy.name, 'findAll'),
wrapAllByQueryWithSuggestion(
(container, text, options) => {
const elements = queryAllBy(container, text, options)
if (elements.length === 0) {
throw new Error('no element found')
}
return elements
},
queryAllBy.name,
'findAll',
),
)
const findBy = makeFindQuery(
wrapSingleQueryWithSuggestion(getBy, queryAllBy.name, 'find'),
wrapSingleQueryWithSuggestion(
(container, text, options) => {
const elements = queryAllBy(container, text, options)
if (elements.length !== 1) {
throw new Error('no element or more than one elements found')
}
return elements[0]
},
queryAllBy.name,
'find',
),
)

return [
Expand Down