TypeError: Cannot read property textContent of null when you expect a property on a missing element queried by data-testid

[sompylasar]

• react-testing-library version: 1.0.1
• node version: n/a
• npm (or yarn) version: n/a

Relevant code or config

expect(queryByTestId('greeting-text').textContent).toBe('hello there')

What you did:

Did not render the element with 'greeting-text' identifier.

What happened:

TypeError: Cannot read property textContent of null

Reproduction repository:

https://codesandbox.io/s/4q3zol71y9

Problem description:

The error does not describe what has gone wrong.

Suggested solution:

Throw a user-friendly error.

[kentcdodds]

Thanks for filing this @sompylasar!

For more context, here's what the error looks like when I make a typo in a similar test:

TypeError: Cannot read property 'textContent' of null

  18 | test('calling render with the same component on the same container does not remount', () => {
  19 |   const {container, queryByTestId} = render(<NumberDisplay number={1} />)
> 20 |   expect(queryByTestId('number-displays').textContent).toBe('1')
  21 | 
  22 |   // re-render the same component with different props
  23 |   // but pass the same container in the options argument.
  
  at Object.<anonymous>.test (src/__tests__/number-display.js:20:10)

The fact that Jest will call out the line number where the error happened makes the problem less concerning for me. I think that people will understand what went wrong.

And just to be clear for everyone, we're not throwing any errors, this is a regular JavaScript error (trying to read textContent on null). I think the error output makes it clear where that's happening. As to answering why that's happening, I think that could be improved.

For the benefit of everyone else, I talked with @sompylasar about this already. He asked if we could throw an error in queryByTestId when the element wasn't found and I explained that would break this use case:

expect(queryByTestId('foo')).not.toBeTruthy()

Where you're trying to assert that something does not exist.

So one thing's for sure: We will not be changing the current behavior.

However, it would be nice to improve the error message. One suggestion @sompylasar made is we could create a new function called queryByTestIdRequired, but first off I doubt many people will use it so we lose the benefits there, and secondly I'd prefer to avoid expanding the API unless it's absolutely necessary.

Another idea I had is to create custom expect matchers that people can use in their apps that could throw more helpful error messages:

expect(queryByTestId('foo')).not.toBeInTheDOM()
expect(queryByTestId('number-display')).toHaveTextContent('1')

I don't think I'll work on these myself, but I would be happy to accept a contribution that adds this capability with a simple API :)

[antsmartian]

@kentcdodds: I give it a shot, the implementation looks like the following:

expect.extend({
    toBeInTheDOM(received) {
        if (received) {
            return {
                message: () =>
                    'expected the dom to be present',
                pass: true,
            };
        } else {
            return {
                message: () => 'expected the dom not to be present',
                pass: false,
            };
        }
    },
})

expect.extend({
    toHaveTextContent(htmlElement,checkWith) {
        console.log(htmlElement.textContent)
        let pass = htmlElement.textContent === checkWith;
        if (pass) {
            return {
                message: () =>
                    `expected ${htmlElement.textContent} not equals to ${checkWith}`,
                pass: true,
            };
        } else {
            return {
                message: () => `expected ${htmlElement.textContent} equals to ${checkWith}`,
                pass: false,
            };
        }
    },
})

And I used in fetch.js test cases like:

expect(queryByTestId('foo')).not.toBeInTheDOM()
expect(queryByTestId('greeting-text')).toBeInTheDOM()
expect(queryByTestId('greeting-text')).toHaveTextContent('hello there')

Everything was green! If you find this good enough, then I can export these functions as a simple API. By the way thanks for a neat small library. The flushPromises is simple & intelligent away to wait for fetch calls.

[sompylasar]

@antoaravinth

        console.log(htmlElement.textContent)
        let pass = htmlElement.textContent === checkWith;
        if (pass) {
            return {
                message: () =>
                    `expected ${htmlElement.textContent} not equals to ${checkWith}`,
                pass: true,
            };
        } else {
            return {
                message: () => `expected ${htmlElement.textContent} equals to ${checkWith}`,
                pass: false,
            };
        }

I'd suggest retrieving htmlElement.textContent once at the first line and storing in a variable. It's not syntactically guaranteed to not change by the time the message function is called.

[antsmartian]

@sompylasar Agreed and it was a quick implementation for you guys to review. Will take care when I raise a PR.

[sompylasar]

Another suggestion: a generic matcher

.toMatchDOM(el => el.textContent === 'hello')

where el is guaranteed to exist or it fails; for safety and clarity, the exceptions from within the matching function can also be caught and enhanced with "the DOM element did not match" hint.

[antsmartian]

@sompylasar I like this idea, as we are allowing the user to pass their own assert function to execute in the current assertion context.

[kentcdodds]

I'd rather it be called: .toSatisfyDOM

There's a toSatisfy in jest-extend, so it would make sense to follow the naming convention there.

[pbomb]

Another related issue with the queryByTestId is with the TypeScript definition. It's declared that it can return HTMLElement | null which is true, but prevents me from being able to write the kind of simple code documented for this library. Just like this issue mentions the runtime error if the queried-for element is null, with TypeScript projects, these lines all become compile-time errors because of this possible scenario.

The rejected proposal of throwing an error would resolve this issue as well since it would no longer return null. Alternately, changing the type declaration in an incorrect way to specify that it doesn't return null might also be a reasonable solution to this problem. I'm not worried about these runtime errors being thrown as this library should only be used in a testing environment where errors thrown are likely to result, correctly, in test failures.

What are your thoughts on this problem? I lean towards fudging the type declaration as the developer experience is really bad when null can be returned.

[kentcdodds]

What if you just write a small wrapper around the render method inside your test/project that throws an error if it's null?

[sompylasar]

What if you just write a small wrapper around the render method inside your test/project that throws an error if it's null?

For the same reason I would use a 29-lines react-testing-library instead of having the small wrappers it provides inside my test/project.

[pbomb]

Yeah, I don't love that idea as it creates a reasonable amount of friction for devs. I think it's very similar to how you imported and re-exported react-dom APIs in this library to ease the friction. Having teams learn and use this library, but getting them to not use the render function in favor of some other wrapper we have is probably a deal breaker in terms of making things easy for them.

Also, how do you feel about the code exhibited in this library not being possible for the many teams that are writing typed JavaScript, either via Flow or TypeScript? I realize those teams are probably still in the minority, but likely not an insignificant percentage. As such a consumer, I'd prefer to see the type definitions relaxed (made incorrect) to ease the development at the cost of type-safety in error scenarios for our tests.

One other option I will explore, that you might prefer, is to overwrite the type definitions for this library in my project. I still feel like this is a hack and that it's not better than changing the definitions in this library itself, but at least would offer up another alternative.

[kentcdodds]

Ok, here are a few ideas:

Add options to queryByTestId:

const queryByTestId = render(<MyThing />)
expect(queryByTestId('foo', {throwIfMissing: true})).toBeTruthy()

Add getByTestId:

const queryByTestId = render(<MyThing />)
expect(getByTestId('foo')).toBeTruthy()

I'm not huge on the idea of expanding the API for something like this, but I appreciate the usefulness to some teams. And honestly, if we did this, I'd probably recommend people use this. Most of the time you wont want the return value to be null. I just appreciate the minimal abstraction that queryByTestId is on top of querySelector.

Anyway, I think I'd accept a PR that:

1. Adds docs for getByTestId (make sure the difference between queryByTestId and getByTestId is very clear, and that the recommendation is to use getByTestId). Perhaps in the future we'll remove queryByTestId entirely, because querying for an element that doesn't exist is a pretty rare use case.
2. Updates existing examples and tests to getByTestId
3. Adds at least one test for queryByTestId that can assert expect(queryByTestId('foo')).toBeFalsy()
4. Implements getByTestId as a minimal abstraction over queryByTestId that throws a useful error message (tested with toThrowErrorMatchingSnapshot()) when the returned element is null.

I think this will make everyone happier :)

[pbomb]

Thanks @kentcdodds, that sounds reasonable to me. I'll start throwing together a PR.

[antsmartian]

@kentcdodds Hey, do you want to me to raise a PR for these methods:

toBeInTheDOM
toHaveTextContent
toSatisfyDOM

Where do you want me to place these extensions? index.js should be fine?

[kentcdodds]

How about you do this:

1. Create a jest-extensions.js file in src where you put the source for those. This should not make calls to expect.extend but instead just be the extension objects themselves.
2. Create an extend-expect.js file in the root of the repository. This file will not be transpiled and should use Node 8 compatible functions (basically that means just use CommonJS instead of ESM). That file should actually do the expect.extend calls. It should also require files from the dist directory (you'll probably need to disable some ESLint rules (import/*) in the file for this to work when the dist directory doesn't exist, like on a fresh clone).
3. Add ./extend-expect.js to the files list in the package.json so it's included when publishing.
4. Add some tests :) I think I'd prefer just regular tests against actual rendered DOM nodes. You could probably update some existing tests.

What do you think?

[antsmartian]

If I understand things correctly, it should be the following approach:

1. Only the extension objects, like:

{
    toBeInTheDOM(received) {
        if (received) {
            return {
                message: () =>
                    'expected the dom to be present',
                pass: true,
            };
        } else {
            return {
                message: () => 'expected the dom not to be present',
                pass: false,
            };
        }
    },
}

2. Call expect.extend from jest-extensions.js to actually extend the jest stuff. require the jest-extensions.js (commonjs system) from dist to make this happen.

3 & 4 seems to be fine. I'm wondering why there is a split up on logic in these two files? Why can't be in a single shot and have a test cases for the same? Any benefit we get with the split up?

[kentcdodds]

The reason to split it up is so people can just do this in their test files:

import 'react-testing-library/extend-expect'

It's explicit, but it's really easy. And it means that if someone wanted to use them differently, or only use one of them, they could:

import {toBeInTheDOM} from 'react-testing-library/dist/jest-extensions'

expect.extend({toBeInTheDOM})

Gives a bit more flexibility without sacrificing usability.

[antsmartian]

@kentcdodds Ahh, that make sense to me. Will do the same and raise a PR. Thanks!

[kentcdodds]

Ok, @pbomb's PR was merged. A release should be out soon and you'll be able to use getByTestId 👍

[sompylasar]

For the record, the link to PR: #10

[kentcdodds]

I think we've dealt with this 👍

[kentcdodds]

@sompylasar, if you'd like to make a pr to add yourself to the contributor table you're more than welcome. You can follow the instructions in the CONTRIBUTING.md 👍

[sompylasar]

@kentcdodds Thanks!

[sompylasar]

👉 #29