testing-library / react-testing-library

🐐 Simple and complete React DOM testing utilities that encourage good testing practices.
https://testing-library.com/react
MIT License
19.04k stars 1.11k forks source link

support `document` containers in cleanup #1330

Open quisido opened 6 months ago

quisido commented 6 months ago

What:

When container is set to document, the cleanup step fails:

TypeError: Cannot read properties of null (reading 'removeChild')

This is because the code assumes document.body exists:

    if (container.parentNode === document.body) {
      document.body.removeChild(container);
    }

In reality, both container.parentNode and document.body are null.

Why:

The tests erroneously fail.

How:

The attempt to remove the container from the body now only occurs when the body exists.

Checklist:

resolves #1329

codesandbox-ci[bot] commented 6 months ago

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit df68ced71826d3084faa95cab5f148f296860682:

Sandbox Source
react-testing-library-examples Configuration
eps1lon commented 6 months ago

Please add a test.

I don't understand why document.body would be null? Isn't that always defined?

quisido commented 6 months ago

I don't know the setup, but is null when container is document. Locally, I fixed this by setting it to document.createElement('body').

I unfortunately do not have bandwidth to write a test for this, but the reproduction steps should be simple enough.

I ran into this specifically when writing a test for a NextJS root layout, which attempts to mount an HTML and body element. The docs say to use container: document when rendering your own HTML element, which fixed the previous error during test but introduced the OP error during cleanup.

MatanBobi commented 6 months ago

@quisido thanks for taking the time to add this but we are not merging changes without proper coverage for them.

quisido commented 6 months ago

@quisido thanks for taking the time to add this but we are not merging changes without proper coverage for them.

Understandable. The PR is welcome to be updated. I opened an issue for it for tracking as well. I can try updating tests as time permits, but I wanted the problem and solution to both be documented.