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

queries returned by render are not scoped #1268

Open
david-bezero opened this issue Jan 30, 2024 · 4 comments
Open

queries returned by render are not scoped #1268

david-bezero opened this issue Jan 30, 2024 · 4 comments

Comments

@david-bezero
Copy link

  • @testing-library/react version: 14.1.2
  • Testing Framework and version: Jest 29.7
  • DOM Environment: jsdom 20.0.3

Relevant code or config:

it('uses a consistent scope', () => {
  const MyComponent = () => {
    useEffect(() => {
      const separateElement = document.createElement('div');
      separateElement.textContent = 'hello';
      document.body.append(separateElement);
      return () => separateElement.remove();
    }, []);

    return (
      <div>
        <div>hello</div>
      </div>
    );
  };
  const { container, getByText } = render(<MyComponent />);

  within(container).getByText('hello'); // passes
  getByText('hello');                   // fails (finds 2 elements)
});

What happened:

The first test (within(container).getByText('hello')) passes, and the second (getByText('hello')) fails:

Found multiple elements with the text: hello

Here are the matching elements:

Ignored nodes: comments, script, style
<div>
  hello
</div>

Ignored nodes: comments, script, style
<div>
  hello
</div>

Problem description:

The container and queries returned by render should be consistent with each other: the queries should search within the returned container by default, to avoid pollution from other tests and libraries which attach elements to other parts of the DOM. For users who need to check the wider document, they can continue to use screen.

It is also worth noting that the documentation claims that the queries are "bound", which does not match the current behaviour (since they apply globally across the document).

Suggested solution:

It is possible to make a wrapper function in user-space which works around this, which should be easy to integrate into the core library:

export const renderScoped = (ui: ReactElement, options?: RenderOptions) => {
  const rendered = render(ui, options);
  return {
    ...rendered,
    ...within(rendered.container),
  };
};

This will be a potentially breaking change for users who currently rely on the queries not being scoped, so probably needs to be a "4.2" release.

@MatanBobi
Copy link
Member

Hi @david-bezero, thanks for taking the time to open this one.
I agree that the docs section is a bit misleading but the query doesn't really apply globally.
If you'll pass a container to the render function or a baseElement, we'll use that and the query will be scoped to that.
At the moment, since you're not passing any of these, the query is bound to the body. If you'll create a new element and append it to the head, it won't be queried.
Having said that, I don't think this is a use-case that happens too often. The example you've attached (and thanks for attaching it) is really not a best practice as it has a side effect which changes the body of the component.
Do you have a better example to share?
Thanks again.

@david-bezero
Copy link
Author

The example you've attached is really not a best practice as it has a side effect which changes the body of the component. Do you have a better example to share?

That example matches all our existing unit tests (and the tests in every other project I've seen / worked on!), so if it's not best practice I'd be interested to know what we should be doing differently.

@MatanBobi
Copy link
Member

That example matches all our existing unit tests (and the tests in every other project I've seen / worked on!), so if it's not best practice I'd be interested to know what we should be doing differently.

Do you mean that you have code inside your useEffect that adds elements to the DOM directly by creating an element? Because that's what I was referring to.

@david-bezero
Copy link
Author

david-bezero commented Jan 30, 2024

ah; I assumed you were referring to the sentence before. The useEffect is not too far from our current scenario, but it's coming from a library we're using which internally (and temporarily) attaches an item to the DOM for rendering (with display: none, but these selectors don't omit things based on that)

A slightly more accurate representation taking that into account would be:

it('uses a consistent scope', () => {
  const MyComponent = () => {
    useEffect(() => {
      const separateElement = document.createElement('div');
      separateElement.textContent = 'hello';
      separateElement.style.display = 'none'; // not actually shown to the user
      document.body.append(separateElement);
      return () => separateElement.remove();
    }, []);

    return (
      <div>
        <div>hello</div>
      </div>
    );
  };
  const { container, getByText } = render(<MyComponent />);

  within(container).getByText('hello'); // passes
  getByText('hello'); // fails (finds 2 elements)
});

I believe the library has to attach the elements to be able to measure text (which doesn't work when detached).


Of course the other issue here is that multiple tests can interfere with each other if one fails to tear something down (causing the failure in another, "innocent", test). The most obvious scenario would be if a test fails to unmount its test component (and the developer hasn't set up an afterEach to call cleanup()), then the test passes but another may fail, and this may depend on the execution order of the tests. The rarer but still possible scenario is if a component being tested attaches an item to the DOM which it fails to clean up when unmounted, or delays removing the element due to an animation (e.g. a dialog or toast notification). Again whether this causes a failure would often depend on the execution order, making it appear "flakey").

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants