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.

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

Encourage using aria selectors #16

Closed
kentcdodds opened this issue Mar 22, 2018 · 11 comments
Closed

Encourage using aria selectors #16

kentcdodds opened this issue Mar 22, 2018 · 11 comments
Labels
needs discussion Whether changes are needed is still undecided and additional discussion is needed.

Comments

@kentcdodds
Copy link
Member

I've been in a discussion @neoziro on my blog where he suggests using aria- attributes and text content instead of data-testid. He makes great points and I think that it would be great if this library could provide selectors to encourage this behavior. I'm thinking that aria attributes wont always make sense for every test, in which case we could fallback to data-testid, and for localization reasons text content may not make the most sense either, but making it easier to select based on text content could still make sense for some use cases and we could explain what those are.

So, out of this issue, I'd like to see some examples of what it would take to make utilities for selecting based on aria attributes as well as text content. If it works out, then we could recommend the following order of preference for selecting:

  1. Text Content (99% of your users will be using your app by looking at the text content, so if you don't have issues with localization then this should be the best)
  2. aria- attributes (your app should be accessible anyway, and this would encourage that as well).
  3. data-testids because they're way better than using class names, dom structure, or querying by React component classes.

Anyone want to try doing this?

@kentcdodds kentcdodds added the needs discussion Whether changes are needed is still undecided and additional discussion is needed. label Mar 22, 2018
@theKashey
Copy link

theKashey commented Mar 22, 2018

It always was a little problem - what to check and how to select.

  • data-testid is great, as long you could remove them in production.
  • aria- attributes is even better, as long they are describing an element
  • text matches? - awesome, cos check that user will see something expected - is the only thing you have to test.

Meanwhile

  • data-testid does not reflect that the customer will see. They are dead selectors.
  • aria- stuff shall not be used for testing purposes. Aria stuff should be used only to help browser understand or override the default behaviour.

If you need a button - use button, not role. Aria-disabled? Just disabled. In real it is a quite rare condition when you have to use aria, better use correct and semantic markup.

  • text content matches are not very specific, sometimes very flexible, sometimes you need that flexibility.

So - I could 90% agree with you. Text Content to test content, data-testid to test DOM structure, if you need to test it.
Just be a bit more careful with aria stuff. Do not prevent browser to infer the right behaviour by its own.

@kentcdodds
Copy link
Member Author

You bring up good points @theKashey. I worry that if we start encouraging using aria- attributes, people will start slapping them on where they don't belong. If we do text matches, then I'd want our abstraction to be pretty loose (maybe allow for a regex search?).

@kentcdodds
Copy link
Member Author

Alrighty, so I've written out a few utilities that have proven to be pretty useful and sensible. I think I want to put them in the library. I'm sure there are more (especially for more aria- attribute associations like the aria-labelledby one here). Feedback welcome:

function queryByLabelText(container, text, {selector = '*'} = {}) {
  const label = Array.from(container.querySelectorAll('label')).find(l =>
    matches(l.textContent, text),
  )
  if (!label) {
    return null
  }
  if (label.getAttribute('for')) {
    // <label for="someId">text</label><input id="someId" />
    return container.querySelector(`#${label.getAttribute('for')}`)
  } else if (label.getAttribute('id')) {
    // <label id="someId">text</label><input aria-labelledby="someId" />
    return container.querySelector(
      `[aria-labelledby="${label.getAttribute('id')}"]`,
    )
  } else if (label.children.length) {
    // <label>text: <input /></label>
    return label.querySelector(selector)
  }
  throw new Error(`cannot find associated element for label with text ${text}`)
}

function queryByText(container, text, {selector = '*'} = {}) {
  return Array.from(container.querySelectorAll(selector)).find(
    node => !node.children.length && matches(node.textContent, text),
  )
}

function queryByPlaceholder(container, text) {
  return Array.from(container.querySelectorAll('[placeholder]')).find(
    node =>
      node.getAttribute('placeholder') &&
      matches(node.getAttribute('placeholder'), text),
  )
}

function matches(textToMatch, matcher) {
  if (typeof matcher === 'string') {
    return textToMatch.toLowerCase().includes(matcher.toLowerCase())
  } else if (typeof matcher === 'function') {
    return matcher(textToMatch)
  } else {
    return matcher.test(textToMatch)
  }
}

@kentcdodds
Copy link
Member Author

I'm working on this right now, just FYI. We'll have several handy utilities for this stuff when I'm finished :)

@theKashey
Copy link

I’ll prefer to write the usage of a new stuff first.
Create some cases, and try to solve them with a better API and thus find that better API and the better practices.

@kentcdodds
Copy link
Member Author

That's where these utilities came from. I'm applying them to tests in another project. It's cleaning things up nicely 👌

@aciccarello
Copy link

I don't have much experience testing React but @jgwhite gave a talk recently at EmberConf that is related. I wonder if there is a good generalized aria-aware query library that could be used in tests across frameworks.

ember conf talk slide showing a "find control for label" test helper function

@kentcdodds
Copy link
Member Author

Thanks for that link @aciccarello! @jgwhite that was a great talk! That's exactly what I'm trying to accomplish in this issue and your talk was very validating. Thank you!

@jgwhite
Copy link

jgwhite commented Mar 23, 2018

@kentcdodds it is really really exciting (and also validating) to see you working on the same ideas. I’m starting work on a formal RFC for Ember’s testing toolkit next week and I’ll be sure to reference this as prior art!

Thanks to @aciccarello for making the connection 🙌

@kentcdodds
Copy link
Member Author

I'm almost finished with my implementation and improved FAQ/README recommendations. I think that tools which encourage improved practices (both for how we write tests as well as how we write applications) are really important and I'm excited by the future here. I'm also very surprised this isn't already mainstream. These ideas actually feel quite obvious...

kentcdodds pushed a commit that referenced this issue Mar 23, 2018
**What**: Add the following methods

- queryByText
- getByText
- queryByPlaceholderText
- getByPlaceholderText
- queryByLabelText
- getByLabelText

**Why**: Closes #16

These will really improve the usability of this module. These also align much better with the guiding principles 👍

**How**:

- Created a `queries.js` file where we have all the logic for the queries and their associated getter functions
- Migrate tests where it makes sense
- Update docs considerably.

**Checklist**:

* [x] Documentation
* [x] Tests
* [x] Ready to be merged <!-- In your opinion, is this ready to be merged as soon as it's reviewed? -->
* [ ] Added myself to contributors table N/A
kentcdodds pushed a commit that referenced this issue Mar 23, 2018
**What**: Add the following methods

- queryByText
- getByText
- queryByPlaceholderText
- getByPlaceholderText
- queryByLabelText
- getByLabelText

**Why**: Closes #16

These will really improve the usability of this module. These also align much better with the guiding principles 👍

**How**:

- Created a `queries.js` file where we have all the logic for the queries and their associated getter functions
- Migrate tests where it makes sense
- Update docs considerably.

**Checklist**:

* [x] Documentation
* [x] Tests
* [x] Ready to be merged <!-- In your opinion, is this ready to be merged as soon as it's reviewed? -->
* [ ] Added myself to contributors table N/A
kentcdodds pushed a commit that referenced this issue Mar 23, 2018
**What**: Add the following methods

- queryByText
- getByText
- queryByPlaceholderText
- getByPlaceholderText
- queryByLabelText
- getByLabelText

**Why**: Closes #16

These will really improve the usability of this module. These also align much better with the guiding principles 👍

**How**:

- Created a `queries.js` file where we have all the logic for the queries and their associated getter functions
- Migrate tests where it makes sense
- Update docs considerably.

**Checklist**:

* [x] Documentation
* [x] Tests
* [x] Ready to be merged <!-- In your opinion, is this ready to be merged as soon as it's reviewed? -->
* [ ] Added myself to contributors table N/A
kentcdodds pushed a commit that referenced this issue Mar 23, 2018
**What**: Add the following methods

- queryByText
- getByText
- queryByPlaceholderText
- getByPlaceholderText
- queryByLabelText
- getByLabelText

**Why**: Closes #16

These will really improve the usability of this module. These also align much better with the guiding principles 👍

**How**:

- Created a `queries.js` file where we have all the logic for the queries and their associated getter functions
- Migrate tests where it makes sense
- Update docs considerably.

**Checklist**:

* [x] Documentation
* [x] Tests
* [x] Ready to be merged <!-- In your opinion, is this ready to be merged as soon as it's reviewed? -->
* [ ] Added myself to contributors table N/A
@aciccarello
Copy link

I'm glad to see connections made. I'd like to see this pattern brought into test Angular applications as well. 👍 to everyone who has been working on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs discussion Whether changes are needed is still undecided and additional discussion is needed.
Projects
None yet
Development

No branches or pull requests

4 participants