Separation of concerns in getByX / queryByX / etc. API

[davidje13]

Describe the feature you'd like:

While considering options for testing-library/jest-dom#106, I found that the core API of dom-testing-library could be improved to make it more extensible. Currently each query defines 6 functions (getByX, getAllByX, queryByX, queryAllByX, findByX, findAllByX). This means that implementing new queries requires a helper method (buildQueries), requires exporting 6 functions, and if binding to an element, any required method must be bound (problematic for sub-libraries such as react-testing-library).

This can be dramatically simplified with a fairly minor (and backwards-compatible) addition; I suggest a new API inspired by selenium's By pattern:

getBy(element, testId('foo'))
queryBy(element, testId('foo'))
getBy(element, attribute('foo', 'bar'))
findBy(element, attribute('foo', 'bar'), { timeout: 1000 })
// (etc)

// bound to an element:
getBy(testId('foo'))
findBy(attribute('foo', 'bar'), { timeout: 1000 })

Queries can be defined as so:

// here defined by calling through to the existing queryAllByAltText function
export const altText = (value, ...options) => ({
  description: `with the alt text ${value}`,
  queryAll: (container) => queryAllByAltText(container, value, ...options),
})

This has numerous advantages:

• APIs such as react-testing-library's render would no-longer need to take a queries option; it would always bind these 6 methods, and any query can be used via an import
• Defining new queries is significantly easier
• The separation of arguments makes them easier to manage. For example, an attribute query can take arguments as (name, value, ...options), allowing role = attribute.bind(null, 'role') to just work (the current implementation does the same but this makes the attribute query unusable by itself because it cannot take an element as its first parameter). This separation also makes configuration clearer (e.g. findBy options are clearly for findBy, rather than for the specific query)
• The API can easily be extended to allow overriding behaviour where required. For example, labelText would not need to define all custom methods to override its mismatch error message; it could define an error message lambda on the object which is used automatically if present (see possible implementation below)
• This is fully compatible with the existing implementation and the two can exist side-by-side (it is even possible to fully define each API using the other to avoid duplication)

There are some disadvantages:

• Users must explicitly import any query which they use (though in many ways this is an advantage)
• Some of the proposed names may have conflicts with variable names in user code (e.g. title, text, role, testId), although users can alias the imports if needed

Suggested implementation:

The following "query runners" can be added to the existing default queries (in the long-run these could be the only bound methods):

export const queryAllBy = (container, query) => {
  const elements = query.queryAll(container)
  if (!elements) {
    return [];
  }
  if (Array.isArray(elements)) {
    return elements
  }
  return Array.from(elements)
}

export const queryBy = (container, query) => {
  const elements = queryAllBy(container, query)
  if (elements.length > 1) {
    throw getMultipleElementsFoundError(
      query.describeMultipleError ? query.describeMultipleError(container) : `Found multiple elements ${query.description}.`,
      container,
    )
  }
  return elements[0] || null
}

export const getAllBy = (container, query) => {
  const elements = queryAllBy(container, query)
  if (!elements.length) {
    throw getElementError(
      query.describeMissingError ? query.describeMissingError(container) : `Unable to find any element ${query.description}.`,
      container,
    )
  }
  return elements
}

export const getBy = (container, query) => {
  const element = queryBy(container, query)
  if (!element) {
    throw getElementError(
      query.describeMissingError ? query.describeMissingError(container) : `Unable to find an element ${query.description}.`,
      container,
    )
  }
  return element
}

export const findAllBy = (container, query, waitOptions) => waitForElement(
  () => getAllBy(container, query),
  waitOptions,
)

export const findBy = (container, query, waitOptions) => waitForElement(
  () => getBy(container, query),
  waitOptions,
)

Then queries can be defined using their existing implementations:

export const altText = (value, ...options) => ({
  description: `with the alt text ${value}`,
  queryAll: (container) => queryAllByAltText(container, value, ...options),
})

export const displayValue = (value, ...options) => ({
  description: `with the value ${value}`,
  queryAll: (container) => queryAllByDisplayValue(container, value, ...options),
})

export const labelText = (value, ...options) => ({
  description: `with the label text ${value}`,
  queryAll: (container) => queryAllByLabelText(container, value, ...options),
  describeMissingError: (container) => {
    const labels = queryAllLabelsByText(container, value, ...options)
    if (labels.length) {
      return `Found a label with the text of: ${text}, however no form control was found associated to that label. Make sure you're using the "for" attribute or "aria-labelledby" attribute correctly.`
    } else {
      return `Unable to find a label with the text of: ${text}`
    }
  },
})

export const attribute = (name, value, ...options) => ({
  description: `by [${name}=${value}]`,
  queryAll: (container) => queryAllByAttribute(name, container, value, ...options),
})

export const placeholderText = (value, ...options) => ({
  description: `with the placeholder text ${value}`,
  queryAll: (container) => queryAllByAttribute('placeholder', container, value, ...options),
})

export const role = attribute.bind('role')

export const testId = (...args) => attribute(getConfig().testIdAttribute, ...args)

export const text = (value, ...options) => ({
  description: `with the text '${value}'`,
  queryAll: (container) => queryAllByText(container, value, ...options),
  describeMissingError: () => `Unable to find any element with the text '${value}'. This could be because the text is broken up by multiple elements. In this case, you can provide a function for your text matcher to make your matcher more flexible.`,
})

export const title = (value, ...options) => ({
  description: `with the title ${value}`,
  queryAll: (container) => queryAllByTitle(container, value, ...options),
})

And since it's so easy to define new queries, smaller helpers could be added, such as:

export const textFragment = (value, ...options) => text(value, { exact: false, ...options })

Describe alternatives you've considered:

The exact naming could be tweaked. I considered get / getAll / query / queryAll / find / findAll, moving the by to byTitle / byAttribute / etc., which is closer to Selenium's choice, but get, query and find are even more likely to conflict!

The original motivation behind this was to allow matchers (see the referenced issue). For example a matcher expect(element).toContainElementWith(testId('foo')) could be written, and it would get all the information it needs to generate useful error messages in all situations. An alternative solution was found to that problem, but I thought this API change was useful enough on its own to warrant suggesting.

Teachability, Documentation, Adoption, Migration Strategy:

This can co-exist with the existing API, though I would recommend phasing out the old API as it will be possible to significantly simplify the codebase.

User migration is trivial:

import { findByTitle } from 'dom-testing-library'
findByTitle(component, 'foo')
// becomes
import { findBy, title } from 'dom-testing-library'
findBy(component, title('foo'))

// queryByX options:
import { queryByTitle } from 'dom-testing-library'
queryByTitle(component, 'foo', {}, { timeout: 1000 })
// becomes
import { queryBy, title } from 'dom-testing-library'
queryBy(component, title('foo'), { timeout: 1000 })

// bound:
getAllByLabelText('woo')
// becomes
import { labelText } from 'dom-testing-library'
getAllBy(labelText('woo'))

[bcarroll22]

Thanks for the super detailed post!

Maybe I’m missing something, but I’m not sure I see how this simplifies the API for the majority use case. I think the stance of the team is that the built-in queries are generally all that you need and it’s very easy to start testing implementation details if you go too far past them. The somewhat cumbersome nature of implementing custom queries, in my mind, is intentional.

But all of that aside, I have a question in follow up. Thinking about it from the perspective of the folks who only use those the built in queries, do you think maybe this proposal actually potentially complicates the API?

[davidje13]

I'd say it would still be a simplification. Maybe it needs a few more imports, but conceptually it becomes easier to see what's going on.

A good example is findByTitle with a custom timeout:

Current API:

findByTitle(element, 'foo', {}, { timeout: 1000 })

What's that {} about? The problem is that the byTitle part takes a title and optional options. But if you want to specify query options, you must specify those title options. This means that it is non-trivial to replace getByTitle(element, 'foo') with a find: I have to know how many arguments the byTitle part consumes and fill them in with defaults.

Proposed API:

findBy(element, title('foo'), { timeout: 1000 })

No magic parameters, and it's obvious where to put options for title matching if I want them vs. where to put options for the finding. (I listed this in my original post although I got find and query mixed up in the example)

This simplification also manifests in typed languages. It would be very easy to define strict types for the proposed functions which will always be present (one of the advantages of users explicitly importing any query which they use), whereas the current API will fail to infer types automatically when binding.

[davidje13]

Also I understand the reluctance to make it too easy to write custom queries, but this is something which can be very important in tests. A few I can think of right now:

// look up table cell by column header and row number
getBy(tableCell('my column header', 7))

// find the currently focused input element
getBy(isFocused())

// find all underlined text (maybe checks `getComputedStyle` to see if each element resolves as underlined)
getAllBy(underlined())

Just because it could be abused doesn't mean it should be hard to do! Hacks will find a way anyway. Might as well optimise for users who are trying to follow the suggested guidelines.

[kentcdodds]

I'm mostly unavailable for the next few days so I can't really look at this carefully until I'm back. But my first impression is that there's not enough of a benefit to justify making this change. Even though it would technically be backwards compatible, it would also enable multiple ways to do the same thing which I like to avoid because it makes it harder on users.

[bcarroll22]

I thought about this some more over the weekend, and my take on it is that this is plenty easy to do with a custom render method if your team really feels the need for these types of queries.

My fears are still:

1. It makes it too easy to create custom queries which will then encourage implementation testing
2. It complicates the API for the majority of users

As @kentcdodds just said in another issue that was just closed, the limits of the testing-library API are intentional and considered a feature. Especially in native, this is a dangerous precedent because it would make it very easy to test by selecting any props sent to a native element rather than just the few users know about. Interested to hear if you have thought about it any more Kent?

[kentcdodds]

No more thoughts.

[kentcdodds]

I'm pretty against this. I feel like it's a more complicated API personally.

[kentcdodds]

Thank you for taking the time to open this. You could definitely build a "plugin" for react-testing-library and if it picks up a lot of usage then we can reconsider. Good luck!