Dom-testing-library: Throw an error if query* is used and an element is found.

Created on 4 May 2020 · 9Comments · Source: testing-library/dom-testing-library

The only reason that query* queries exist are so you can assert that something is not in the document. Otherwise you should be using the get* variants.

On top of this, here's how it's normally used:

expect(screen.queryByRole('textbox', {name: /input/i})).not.toBeInTheDocument()

And the warning message we get with that is something like:

expected document not to contain element, found <input id="input" type="text" /> instead

Which isn't quite as helpful as what we could do if query* threw when it found an element:

    TestingLibraryElementError: Found an accessible element with the role "textbox" and name `/input/i`

    Here are the accessible roles:

      document:

      Name "":
      <body />

      --------------------------------------------------
      textbox:

      Name "Input":
      <input
        id="input"
        type="text"
      />

    <body>
      <div>
        <label
          for="input"
        >
          Input
        </label>
        <input
          id="input"
          type="text"
        />
      </div>
    </body>

I'm just not sure that the name of the function communicates that the intent is to not find anything. So for that reason, it may be better to just change nothing (or maybe add logging the document.body in the toBeInTheDocument assertion in jest-dom).

Thoughts?

needs discussion

Source

kentcdodds

Most helpful comment

I like the idea of throwing if query* was used on existing elements. I'd say you almost always want the full error message instead of non-descriptive "cannot read property "getAttribute" of null".

Regarding

await user.sees({role: 'heading'})

The naming is important here. I don't think it's viable to implement a method that verifies this reliably (even if we do consider an actual browser environment).

I think we had some prior issues where users were confused that .toBeVisible would not consider e.g. offsetHeight.

eps1lon on 4 May 2020

👍2

All 9 comments

I think we'd need a different name. Query is called that because it is similar to querySelector

alexkrolick on 4 May 2020

👍2

Yeah, I feel the same way. Adding yet another variant would be a mistake though I think, so maybe this should be something jest-dom improves.

kentcdodds on 4 May 2020

I was thinking maybe either the assertions from jest-dom, or the actions from user-event should get more built-in querying powers. I kind of prefer the user-event path because it isn't specific to Jest/Jasmine assertions.

await user.click({role: 'button', name: 'hi'})
await user.sees({role: 'heading'})
await user.sees.not({role: 'button})

alexkrolick on 4 May 2020

That's an interesting idea. Hmmmm... Would love to hear what other's think about that idea.

kentcdodds on 4 May 2020

I like the idea of throwing if query* was used on existing elements. I'd say you almost always want the full error message instead of non-descriptive "cannot read property "getAttribute" of null".

Regarding

await user.sees({role: 'heading'})

The naming is important here. I don't think it's viable to implement a method that verifies this reliably (even if we do consider an actual browser environment).

I think we had some prior issues where users were confused that .toBeVisible would not consider e.g. offsetHeight.

eps1lon on 4 May 2020

👍2

Makes sense. I didn't spend much time on that idea but I have seen some competing test utils that combine actions and querying that make for a pretty compact syntax especially for e2e flows.

As queryAll is currently the basis for all the other queries, and the helper function to generate get* and find* variants is exposed as a public API, I don't see how we would make that change unless singular query behaves differently than query all (which seems confusing)

alexkrolick on 4 May 2020

neverByRole instead? I don’t like too much but it gives the idea that you want to find nothing

marcosvega91 on 4 May 2020

The only reason that query* queries exist are so you can assert that something is not in the document

I think that fact makes it a bit confusing when you are getting started with RTL.

Maybe it would make sense to throw the error as you suggested and rename the function to align more to its purpose.

assertNotPresentByRole
verifyNotPresentByLabelText

or something like that

dougbacelar on 5 May 2020

Honestly I really don't think we should do anything about this. I think it would cause more harm than good.

The suggested extra utilities feel too high level for this package at the moment so I think I'll close this for now. If someone wants to make another package to play around with the ideas suggested feel free and eventually maybe they can make it into the core. Thanks!

kentcdodds on 10 May 2020

Was this page helpful?

0 / 5 - 0 ratings