testing-library / jest-dom

:owl: Custom jest matchers to test the state of the DOM
https://testing-library.com/docs/ecosystem-jest-dom
MIT License
4.41k stars 397 forks source link

getRootNode not returning Document for element matched with findByText query #404

Open christemple opened 3 years ago

christemple commented 3 years ago

Relevant code or config:

// [1] Sporadically fails
expect(await screen.findByText('hello')).toBeInTheDocument();

// vs.

// [2] Passes every time without fail
waitFor(() => {
  expect(screen.getByText('hello')).toBeInTheDocument();
})

What you did:

Used expectation pattern [1] over [2]

What happened:

Sporadic failures Received element is not visible (element is not in the document)

Problem description:

Essentially this check fails: https://github.com/testing-library/jest-dom/blob/main/src/to-be-in-the-document.js#L11 because sporadically getRootNode returns the React node that got matched itself rather than the Document.

Which means that findBy... succeeds in returning a matched node, however when verifying if this node is in the Document with .toBeInTheDocument() it fails.

Suggested solution:

I don't know enough about the internals of when the root node changes from the React node to the Document node. I know for sure the root node eventually changes to Document, hence why the waitFor [2] pattern works.

Reproducing is difficult because of it's sporadic nature, but I'm essentially creating this issue to find out if others are being affected by this, and also to learn a bit more about the internals that could be resulting in this issue.

In the meantime I'll be using the waitFor[2] expectation pattern from now, but I'm worried that I may forget about this issue and have sporadic failures again in the future since pattern [1] is the first that comes to mind.

berlysia commented 2 years ago

Hi, I also faced this problem and got some small reproducible patterns, and some memorization solve them.

working sample: https://codesandbox.io/s/jest-dom-404-repro-mmb2s

implementation is also folded here ```tsx import { useEffect, useReducer, useState } from "react"; export function Container({ Inner }) { const [visible, setVisible] = useState(false); const [, rerender] = useReducer((x) => !x); useEffect(() => { if (!visible) { // this MUST use task queue. sync OR microtask cannot repro(in my test setTimeout(() => { setVisible(true); }, 100); } }, [visible]); useEffect(() => { if (visible) { rerender(); } }, [visible]); return
{visible && }
; } // export function InstantComponent() { // FaCC or HoC easily falls into this situation // memorize this const TmpComp = (props) => ; return (
foo
); } // export function KeyedComponent() { // memorize this const key = Date.now().toFixed() + "$" + (Math.random() * 10000).toFixed(); return (
foo
); } ```

It seems that the element~s~ of the first result are detached from the tree. Are you in the same situation?

christemple commented 2 years ago

This is amazing, thanks so much for your help @berlysia, I'm going to find some time today to find the culprit components and implement your suggestions :+1:

DylanSpOddball commented 2 years ago

We've also run into this issue; it can be reliably reproduced in https://github.com/CMSgov/easi-app/tree/e05031401a79414a38244094642c05b0e5c58a19, the failing test is src/views/SystemList/index.test.tsx -> System List View -> when there are requests -> displays a table. The repro isn't minimal, unfortunately.

It'd be useful if this could be solved within jest-dom/testing-library, so the expect(await [query]) pattern can be used in tests without changing the implementation of the component(s) being tested.

pstachula-dev commented 2 years ago

I have similar issues in many tests

gnapse commented 2 years ago

because sporadically getRootNode returns the React node that got matched itself rather than the Document

Hmmm, this seems to be the key of the issue. However, we do not control getRootNode. It's from jsdom. I wonder if either jsdom or the way findByText works has something to do with this. I do not see much that we can do from jest-dom's perspective.

I tried this for now: I forked the codesandbox example provided by @berlysia above (see https://github.com/testing-library/jest-dom/issues/404#issuecomment-952179213). Then I focused on one of the failing tests alone, and modified it in this way:

  test.only('findBy', async () => {
    const { debug } = setup() // modified setup to return render too
    const element = await screen.findByText('foo')
    debug()
    console.log('root node', element.getRootNode())
    console.log('parent', element.parentElement)
    expect(element).toBeInTheDocument()
  })

This is the output I get in the console:

image

Not sure what to make of it. The element appears placed correctly in the DOM tree, but for some reason its parentElement is null, and calling its getRootElement() returns the element itself, instead of the document where it lives.

I'm honestly not sure right now how .toBeInTheDocument's implementation is at fault here. Suggestions are welcome.

tyteen4a03 commented 2 years ago

Any updates on this ticket? I can confirm I am still getting flaky tests on our Jenkins and we're not really sure what's going on. The waitFors don't seem to be helping either.

dpirkle-naut commented 1 year ago

I am running into this as well. It's 100% reproducible. Flakey only in the sense that the first test works as expected, but subsequent tests all fail. Any of them that are run in isolation will pass.

If I have 2 identical tests that do this:

    render(<TestFilter/>);
    const available = await screen.findByRole('list', { name: /available/i });
    expect(available).toBeVisible()

When run together, the first test will pass, and the second one will fail. When it fails, it's failing in toBeInTheDocument when it tests element.ownerDocument === element.getRootNode(...). The call to getRootNode returns the top-level element rather than the document. Strangely, if I look at the HTML inside element.ownerDocument.body, I see that the expected HTML has been rendered in there.

If I use waitFor instead of await, as was suggested at the top of this thread, then all tests pass.

ericbiewener commented 1 year ago

Why is the element.ownerDocument === element.getRootNode(...) check required rather than a more straighforward document.contains(element)? And for that matter, why is this checkHtmlElement call needed as well?

I'm considering implementing my own toBeInTheDocument matcher for my codebase as a workaround to this issue, but I'd like to understand the implications of just doing a basic document.contains() check.

berlysia commented 1 year ago

:memo: My first reproducible code (in https://github.com/testing-library/jest-dom/issues/404#issuecomment-952179213 ) is now fixed with:

:memo: https://github.com/CMSgov/easi-app/tree/e05031401a79414a38244094642c05b0e5c58a19 also works on my machine, with update deps described above.

Is anyone still experiencing this issue, with latest deps?

dpirkle-naut commented 1 year ago

After updating our dependencies the problem we were experiencing disappeared.

tomdglenn91 commented 1 year ago

Unfortunately I can't use react testing library v13 due to our project being frozen on v12. I have been suffering this exact issue and is sadly causing me to lose confidence in my tests. Like you, I get sporadic failures and my own digging found me at the same conclusion this thread has, with getRootNode sometimes returning the Document and sometimes returning the DOM element of the node being tested.

Hopefully not stating the obvious here or being naive, but seems to be if renders happen after I do my assertions.

I added this:

    console.log('getting');

    const content = await screen.findByText(/Test/i);

    console.log(content.ownerDocument);
    console.log(content.getRootNode({ composed: true }));

    console.log('got');

And the 2 logs are next to each other, as I was wondering if a rerender would 'detach' the element I got from the document, but that makes no sense if these 2 logs output next to each other.

Running jest with --no-cache changes the failure rate to mostly fail rather than mostly pass, not sure if that's a red herring, or..

Does anyone have an understanding as to how and why getRootNode changes?

berlysia commented 1 year ago

I did another research for previous comment. I've found that the detached element is easy to get and hard to notice.

Here is my latest research. https://codesandbox.io/s/peaceful-thunder-h7tfmx

This mini-app says two points:

  1. Switching between React components generates different elements. Even if the final DOM structure is consistent.
  2. When the parent React component switches, the child components generate different elements. Even if children rendered by the same component.

And we already know that components declared in render cause component switching.

So the following patterns are suspicious:

1. Inline component: Components declared in render function

function Inline() {
  const Needle = () => <span>needle</span>;
  return (
    <div>
      <Needle />
    </div>
  );
}

Real world example: Row renderer of table-like component

full code(codesandbox)

const SEED = [1, 2, 3];
const USER = 2;

export function App() {
  const seed = SEED; // given table values
  const user = useAsyncValue(USER); // something like `fetch("/user")`
  const rowRenderer = useCallback(
    function Row({ id }: { id: number }) {
      return (
        <span>
          id: {id}
          {id === 2 && ", needle"}
          {id === user && ", me"}
        </span>
      );
    },
    [user]
  );

  return (
    <div>{seed ? seed.map((x) => rowRenderer({ id: x })) : "(empty)"}</div>
  );
  // `x => <rowRenderer id={x} / >` will achieve same result
}

If you like, change the version on the Dependencies tab and check it out.

It would be nice to be able to extract the variables in the parent scope, reference them using props, and move the component to the top level.

2. Shared component: Components shared by two or many parent component

function Needle() {
  return <span>needle</span>;
}
function A() {
  return <div><Needle /></span>
}
function B() {
  return <div><Needle /></span>
}
function App() {
  return <div>{Math.random() < 0.5 ? <A/> : <B/>}</div>
}

This pattern also cause detached elements. Real world example is not ready, but a SPA router could belong to this pattern.

lilasquared commented 5 months ago

This appears to be a much bigger issue in react 18 vs 17. I've got hundreds of tests failing due to toBeInTheDocument after upgrading even though the element is clearly present. It seems like there should be a way for await findBy* to wait more before it returns the element it finds, as it seems to find one before its fully placed in the dom, which I think is why waitFor works 100% of the time. Considering the recommendation has always been to use await findBy* over waitFor(() => getBy*) its a bit of a bummer that its busted with new versions of react for fairly trivial use cases 😞

thiagoctrbs commented 1 week ago

@lilasquared I'm also in the process of upgrading to React 18 and bumped into this exact same problem :(