toBeInTheDOM matcher only ensures instanceof HTMLElement, expected to check that the element is attached to a full DOM tree

[sompylasar]

• dom-testing-library version: 1.1.0
• node version: N/A
• npm (or yarn) version: N/A

Relevant code or config

https://github.com/kentcdodds/dom-testing-library/blob/f77f943a1887756372993eaf6ee3b054f2f58b91/src/jest-extensions.js#L18-L26

https://github.com/kentcdodds/dom-testing-library/blob/f77f943a1887756372993eaf6ee3b054f2f58b91/src/jest-extensions.js#L63-L85

(copy of the linked code)

function checkHtmlElement(htmlElement) {
  if (!(htmlElement instanceof HTMLElement)) {
    throw new Error(
      `The given subject is a ${getDisplayName(
        htmlElement,
      )}, not an HTMLElement`,
    )
  }
}

// ...

const extensions = {
  toBeInTheDOM(received) {
    if (received) {
      checkHtmlElement(received)
    }
    return {
      pass: !!received,
      message: () => {
        const to = this.isNot ? 'not to' : 'to'
        return getMessage(
          matcherHint(
            `${this.isNot ? '.not' : ''}.toBeInTheDOM`,
            'element',
            '',
          ),
          'Expected',
          `element ${to} be present`,
          'Received',
          received,
        )
      },
    }
  },

---

What you did:

1. Used .toBeInTheDOM() in a test with an element disconnected from the DOM, like here; the test passed, surprisingly.
2. Reviewed the source code of jest-extensions.js

What happened:

Discovered that toBeInTheDOM does not check that the element is attached to a full DOM tree.

Reproduction repository:

This repository's tests.

Problem description:

toBeInTheDOM, according to its name, has to ensure that the given element is in the DOM, i.e. attached to the full DOM tree, not hanging as a standalone element or in a standalone subtree.

Suggested solution:

Find out if the element is actually attached to a DOM document via element.ownerDocument.contains(element), and report an assertion failure if it's not.

[sompylasar]

toBeInTheDOM can also optionally accept a container element which should be used instead of the ownerDocument (the container should have the same ownerDocument).

[kentcdodds]

That sounds good to me. Perhaps we should deprecate the current one and add a new one in its place. Something like this?

expect(node).toBeAChildOf(parentNode)

Either that or do as you suggested and just make it an optional argument of toBeInTheDOM. I think I'll be easy to sway either way.

[antsmartian]

@sompylasar Agree, that's bug to me. To me we can keep the same name as toBeInTheDOM, but accepts the optional argument too.

[gnapse]

I did notice this inconsistency while working in #1. Essentially .toBeInTheDOM is a glorified .toBeInstanceOfClass(HTMLElement).

The weird thing about adding a container argument is that when using the query* selectors in this library, you're pretty much guaranteed that if something is returned, it'll be in "the DOM".

expect(queryByText(container, 'OK')).toBeInTheDOM(container);

// The same happens with toBeAChildOf
expect(queryByText(container, 'OK')).toBeAChildOf(container)

The only way the above expectations can fail is if the element is not found, and therefore null. There's no way it can exist being an HTMLElement, and not be in "the DOM".

However, I also agree that this does not solve the problem of this custom matcher not really doing what it is supposed to do. And there's no guarantee that it has to always be used with these query* selectors.

[sompylasar]

Here's where I needed it to fail if the element happened to not be appended because of a bug: https://github.com/sompylasar/dom-testing-library/blob/5c4061834cf96bd8b878bf1a0ccc355da7d4c42e/src/__tests__/waitForElements.js#L94

I checked for parentNode, but for more deep tree it would be more convenient with a custom assertion.

[gnapse]

Reopened this issue in testing-library/jest-dom#3 so we can continue the discussion over there.

[sompylasar]

@gnapse Thanks, we can close this one then in favor of yours testing-library/jest-dom#3