Skip to content

toBeEmptyElement isn't working as expected #141

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

Closed
Drew-Gerber opened this issue Jan 6, 2023 · 2 comments
Closed

toBeEmptyElement isn't working as expected #141

Drew-Gerber opened this issue Jan 6, 2023 · 2 comments

Comments

@Drew-Gerber
Copy link

  • react-native or expo: vanilla RN
  • @testing-library/react-native version: 11.5.0
  • @testing-library/jest-native version: 5.3.1
  • jest-preset: 'react-native'
  • react-native version: 70.6
  • node version: v16.15.1

Relevant code or config:

The relevant code appears to be here: https://github.com/testing-library/jest-native/blob/main/src/to-be-empty-element.ts#L9

Here's a minimal repro:

import { render } from '@testing-library/react-native';
import { Text } from 'react-native';

function Test() {
  return <Text>hello</Text>
}

test('renders correctly', () => {
  const { container } = render(<Test />);
  expect(container).toBeEmptyElement()  // this should fail, but passes
});

What you did:

First, I tried to use toBeEmptyElement in a test to assert that null was returned from a component. Noticed it passed (yay!), then I tried a negative test to make sure that the test failed if non-null was returned. The test still passed, when a failure was expected 🤔.

What happened:

The referenced test/repro code is passing, when a failure is expected.

Reproduction:

import { render } from '@testing-library/react-native';
import { Text } from 'react-native';

function Test() {
  return <Text>hello</Text>
}

test('renders correctly', () => {
  const { container } = render(<Test />);
  expect(container).toBeEmptyElement() // this should fail, but passes
});

Problem description:

The matcher toBeEmptyElement doesn't appear to work as suggested in the docs/as expected.

Suggested solution:

I think the pass condition in toBeEmptyElement should be element?.children instead of element?.props?.children?
Relevant code: https://github.com/testing-library/jest-native/blob/main/src/to-be-empty-element.ts#L9

Can you help us fix this issue by submitting a pull request?

I believe so - I can try to find some time.

@mdjastrzebski
Copy link
Collaborator

mdjastrzebski commented Jan 7, 2023

I think you've found a valid bug. However, the case might be more complex than the solution you suggested.

TLDR: do not invoke any assertions on container because nearly always it's a composite component. And assertions should only be run on host components. Use some query to find interesting element (even if it appears to be the root element) and use assertions on this element.

I'll try to explain it briefly:

  • Element tree under React Test Renderer/RNTL tests is a mix of host and composite elements. Your assertion should work fine when invoked on host elements but will can incorrectly report failure when invoked on composite element.
  • Most of the queries return host elements (getByTestId, getByRole, getByLabelText, ....)
  • Following queries return composite elements: getByText, getByPlaceholderText, getByDisplayValue, in such cases the returned composite element will always hold reference to a single host child (Text or TextInput in case of these queries).
  • When using container it will also always return composite element.

So when invoking the toBeEmptyElement on host component then both assertions will work fine. However, when invoking it on composite element you will get results that can be unintuitive:

  • for element?.children check: composite <Text /> will report having a single host <Text /> child despite the conceptual View not having any children.
  • for element?.props?.children check: composite <Text /> works fine, but the case you present will give the error you described.

Both can give wrong results in certain situations.

The actual solution would consist of following changes:

The last point is actually something actionable. But would need to include a compatibility hack to work correctly with composite Text and TextInput instances returned by current version of RNTL from following queries: getByText, getByPlaceholderText, getByDisplayValue.

@mdjastrzebski
Copy link
Collaborator

This should no longer happen with RNTL v12+ as all queries now return host elements, as well as container has been replaced by host root.

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