add toContainQuerySelector matcher for better errors

[davidje13]

Describe the feature you'd like:

Currently the recommendation for checking if an element exists is:

const htmlElement = document.querySelector('[data-testid="html-element"]')
const nonExistantElement = document.querySelector('does-not-exist')

expect(htmlElement).toBeInTheDocument()
expect(nonExistantElement).not.toBeInTheDocument()

(abridged from the docs)

But this returns pretty useless errors if the condition is not met. It would be much more useful to combine the querySelector with the existence check, such as:

expect(document).toContainQuerySelector('[data-testid="html-element"]')
expect(document).not.toContainQuerySelector('does-not-exist')

(this is also much shorter syntax)

This is comparable to toContainMatchingElement from the enzyme-matchers project.

Suggested implementation:

I've tried to match the code style of this project, but might have missed something:

import {matcherHint, printReceived, printExpected} from 'jest-matcher-utils'
import {checkHtmlElement} from './utils'

export function toContainQuerySelector(element, selector) {
  checkHtmlElement(element, toContainQuerySelector, this)
  return {
    pass: Boolean(element.querySelector(selector)),
    message: () => [
      matcherHint(`${this.isNot ? '.not' : ''}.toContainQuerySelector`, 'element', 'selector'),
      '',
      `Expected ${this.isNot ? 'no elements' : 'an element'} matching`,
      `  ${printExpected(selector)}`,
      'Received',
      `  ${printReceived(element)}`,
    ].join('\n'),
  }
}

Describe alternatives you've considered:

• .not.toBe(null) or a hypothetical .toExist(), etc. have similar problems to .toBeInTheDocument: the returned error is not helpful for diagnosis because it does not include the content which was actually observed when an element is missing.

• It might be valuable to be able to count the number of occurrences (e.g. toContainQuerySelector('foo', 4) to check for exactly 4 occurrences, but I can't think of a self-descriptive method name for this, so as-proposed this checks for 1-or-more, or exactly zero.

Teachability, Documentation, Adoption, Migration Strategy:

Recommendation can be updated to:

expect(document).toContainQuerySelector('[data-testid="html-element"]')
expect(document).not.toContainQuerySelector('does-not-exist')

No migration necessary; existing matchers do not need to change.

[gnapse]

Hi, I've been thinking about this, that's the reason for the delayed response. Sorry about that.

I'm hesitant of providing this. The argument for the unhelpful error message when toBeInTheDocument seems compelling, but don't you get also the line of the failed expect?

On the other hand, the downside is that this custom matcher feels to move away from the testing libraries' guiding principle. It encourages testing the internal markup structure rather than encouraging asserting based more on what the user sees perceives. Granted you can still do it today via document.querySelector and toBeInTheDocument, but the more declarative custom matchers do not promote this way of doing things.

That being said, I'm open to hear a stronger case for this. Or maybe the input from @kentcdodds and @alexkrolick would be nice.

[kentcdodds]

I'm opposed.

If you want a good error message for something that has a query then just do this:

expect(getByTestId('html-element')).toBeInTheDocument()

The fact is that if getByTestId can't find something in the document it'll throw an error so you'll never get to the assertion anyway, so I've been doing just this:

getByTestId('html-element')

And that works for me.

If there's a general css query selector you'd like to select on that's not one of the supported queries I would recommend you find a way to use one of the supported queries for maintainability/accessibility reasons. It's pretty easy to workaround if you want to use a general query though:

expect(document.querySelector('does-not-exist')).not.toBeInTheDocument()

That should give you a good error message.

[davidje13]

For me the problem is more around knowing what was actually rendered, not about what element was not found.

For example, suppose I have a list element with an empty state. I expect to see the empty state, but I don't, so the test fails. OK, so I can infer that it didn't render the empty state, but I have no idea why without digging in to code. If instead it said "failed to find 'my-empty-state' in <ul><li>global</li></ul>" then I can say "d'oh, I forgot that it will always contain 'global'; my empty state check needs to apply if the list contains only one item" (silly example).

As you say, the existing error for the negated version is pretty OK already. It's just the non-negated version which is bad.

I'm very uneasy about the naked getByTestId('html-element'). That doesn't read well as code, because there is no hint that an assertion is being made; it just looks like some code left behind by mistake. And in a similar vein, using toBeInTheDocument as a no-op to hide this seems hacky (and it implies that you could negate it with .not., but that won't actually work). The "correct" test would be the rather horrendous:

expect(() => getByTestId('html-element')).not.toThrow(); // err...

// negated: ("expect not to be in the document")
expect(() => getByTestId('html-element')).toThrow(); // what? what is this?!

I understand the concern around using native query selectors rather than the helpers. I quite like selenium's By approach for this, which could be translated to:

expect(document).toContainElement(By.testId('html-element'));
expect(document).toContainElement(By.querySelector('.whatever'));
expect(document).toContainElement(By.name('password'));
// etc.

[davidje13]

You could even try something crazy like this:

expect(getByTestId).toFind('html-element');
expect(getByTestId).not.toFind('html-element');

expect(document.querySelector).toFind('.whatever');
expect(document.querySelector).not.toFind('.whatever');

Where the toFind matcher takes in a function, invokes it with the arguments given at the end, and checks that the result is a HTMLElement. If it is not (or if it throws), it can provide an error message which includes the current DOM structure to aid debugging. It would never throw (unless actually used in an invalid way, such as not giving a function); by failing instead it will always be possible to negate it in a meaningful way.

The main limitation of this approach is that if somebody runs a test against something other than the whole document (i.e. expect(myElement.querySelector).toFind('.whatever');), they would see the structure for the whole document in the error, which might be confusing. Having said that, if searching from a sub-element, it's probably an indication that they need to break up their component somewhere.

Also I'm not sure if it would be necessary to bind the querySelector function, but if that is required, it would just serve to encourage use of the getByTestId &co. functions since they're pre-bound.

[alexkrolick]

And in a similar vein, using toBeInTheDocument as a no-op to hide this seems hacky (and it implies that you could negate it with .not., but that won't actually work). The "correct" test would be the rather horrendous:

expect(() => getByTestId('html-element')).not.toThrow(); // err...

expect(() => getByTestId('html-element')).toThrow(); // what? what is this?!

Why not this:

expect(queryByTestId('html-element')).not.toBeInTheDocument()

queryBy* doesn't throw and the assertion should print the DOM

[davidje13]

@alexkrolick true, but the reverse:

expect(queryByTestId('html-element')).toBeInTheDocument()

produces a bad error message if it fails, which is what @kentcdodds was suggesting getByTestId for (since the thrown error in that case is actually helpful).

If you think it should be a helpful error, maybe that means this is actually a bug in toBeInTheDocument? What it shows now is this:

received value must be an HTMLElement or an SVGElement.
    Received has value: null

Which is totally useless.

Overall, it means the current state is rather inconsistent / incomplete:

expect(getByTestId('html-element')).toBeInTheDocument() // works, good error message on failure, but throws rather than failing and the matcher is only present for appearances sake
expect(getByTestId('html-element')).not.toBeInTheDocument() // never works (either fails or throws)

expect(queryByTestId('html-element')).toBeInTheDocument() // works, and does not throw, but (very) bad error message on failure
expect(queryByTestId('html-element')).not.toBeInTheDocument() // works, and does not throw, with a reasonable-enough error message on failure

[alexkrolick]

I think we could safely remove the || here and allow it to return false and print the full error even in the non-.not case:

jest-dom/src/to-be-in-the-document.js

Lines 9 to 11 in b2b4bd3

	if (element !== null || !this.isNot) {
	checkHtmlElement(element, toBeInTheDocument, this)
	}

[gnapse]

First, about these two uses:

expect(getByTestId('html-element')).toBeInTheDocument()
expect(getByTestId('html-element')).not.toBeInTheDocument()

Combining getBy* with toBeInTheDocument does not make sense. getBy* helpers are already meant to raise an error and make your test fail if the element is not found.

Now, on to the other part:

expect(queryByTestId('html-element')).toBeInTheDocument()
expect(queryByTestId('html-element')).not.toBeInTheDocument()

I understand your point, but there's hardly something we can do with toBeInTheDocument when used in that way. If queryByTestId returns null, toBeInTheDocument can hardly do anything at all. It received null instead of an element, so it cannot render any context in the markup to help you.

@alexkrolick's suggestion might help, but I suspect the error message if we let this go through at that point in the code would not be helpful either. Think about it, the matcher was called with element: null. It has nothing to print. Unless it prints out the entire document, so you can see that what you were looking for is not in there (but that's what the matcher's failure is already telling you).

[alexkrolick]

so you can see that what you were looking for is not in there (but that's what the matcher's failure is already telling you).

That's the suggestion, in the same way that getBy prints the DOM when it fails. It helps diagnose the issues much faster than adding debug/prettydom calls.

[gnapse]

Seems like the consensus here was to not add the proposed feature. I'll be closing this for the time being.