prefer-in-document: `.toHaveLength(1)` vs `.toBeInTheDocument()`

[nstepien]

• eslint-plugin-jest-dom version: 3.8.1
• node version: 16.1.0
• npm version: 7.11.1

Relevant code or config

expect(screen.queryAllByRole('option')).toHaveLength(2);
// some action here
expect(screen.queryAllByRole('option')).toHaveLength(1);

What you did: I want to ensure there is only one option.

What happened:

The rule wants this to be

expect(screen.queryByRole('option')).toBeInTheDocument();

Reproduction repository:

Problem description: It's not the same thing! I cannot assert that there is only one option now.

Suggested solution: prefer-in-document should allow .toHaveLength(1). Or there should be an option at least.

[astorije]

Actually expect(screen.queryAllByRole('option')).toHaveLength(1); and expect(screen.queryByRole('option')).toBeInTheDocument(); technically test the same thing, but semantically they do not represent the same thing, so it doesn't always make sense to use the toBeInTheDocument format.

For example, given the following test:

test('should only have 1 option', () => {
  render(<MySelectThatRenderOneOrMoreOptions />);

  // Retrieve all rendered options
  const allOptions = within(
    screen.getByRole('combobox'),
  ).getAllByRole('option');

  // Make sure there is only 1
  expect(allOptions).toHaveLength(1);
  expect(allOptions).toHaveValue('value');
});

Here the test semantically represents counting the number of rendered elements and making sure there is one and exactly one element.

After running the auto-fixer (and renaming variables accordingly):

test('should only have 1 option', () => {
  render(<MySelectThatRenderOneOrMoreOptions />);

  // Retrieve all rendered options
  const option = within(
    screen.getByRole('combobox'),
  ).getByRole('option');

  // Make sure there is only 1
  expect(option).toBeInTheDocument();
  expect(option).toHaveValue('value');
});

In this case, the test is structured in a way that makes it about the first element itself. Granted, it would fail if there were more than 1 element (and RTL would let us know we might want to use getAllByRole instead), but then the failure would be misleading.

I think prefer-in-document should not be suggesting .toBeInTheDocument when it encounters .toHaveLength(1).

[mdotwills]

What about the flip case here, when a findAll* or getAll* type query has been used with a matcher that expects a zero value, for example expect(screen.findAllByRole('button')).toHaveLength(0) - using @astorije's reasoning above, does it make sense to infer that toHaveLength(0) == not.toBeInTheDocument() or should this also be ignored by the prefer-in-document rule?

[nstepien]

I think toHaveLength(0) == not.toBeInTheDocument() is okay.

[mdotwills]

@nstepien yes, makes sense to me.

Just one last subtly I wanted to ask about, and that is how best to handle queries which are expected to return exactly one vs queries which are expected to return one or more matches - e.g. getBy* vs getAllBy*? See the Summary Table here.

As we have been discussing, this makes sense

// no violation here, no fix required
expect(screen.queryAllByRole('option')).toHaveLength(1);

but what about the case in which

// original
expect(screen.queryByRole('option')).toHaveLength(1);

In this second case, queryByRole will return one element only and error in other scenarios. Is the preferred rule here better to be .toBeInTheDocument() since we are testing the single element or to leave as .toHaveLength(1)? I am thinking .toBeInTheDocument() in this case, as the *By* queries are semantically different to *ByAll* queries.

[nstepien]

I think it depends,

• it could be a mistake and the user actually intended to use ByAll
• it's not a mistake, and in that case toBeInTheDocument() makes more sense

I'd say the rule should warn about it, and provide 2 suggestions, wdyt? https://eslint.org/docs/developer-guide/working-with-rules#providing-suggestions

[mdotwills]

I like your suggestion @nstepien 👍 I'll have a crack at implementing that

[mdotwills]

Based on the discussion in this thread, I've put together this truth table of selector type vs assertion style:

Selector type	.toBeInTheDocument()	.toHaveLength(1)	.toHaveLength(0)
(*By*)	✅ correct usage, no change	Did you mean to use (*AllBy*)?	🔧 replace with .not.toBeInTheDocument()
(*AllBy*)	🔧 replace with .toHaveLength(1)	✅ correct usage, no change	✅ correct usage, no change

The resolution of the (*By*) ∩ .toHaveLength(1) suggestion of did you mean to use (*AllBy*) could resolve as

• Yes, then replace (*By*) with (*AllBy*), leaving expect((*AllBy*)).toHaveLength(1),
• No, then replace .toHaveLength(1) with .toBeInTheDocument() , leaving expect((*By*)).toBeInTheDocument()

both of which are stable states.

[nstepien]

Got hit by this again today 😔

[SevenOutman]

Any update on this? Got positive on findAllByRole with .toHaveLength(1) when checking for "exactly one match".

[G-Rath]

@SevenOutman PRs are welcome; reading over the comments I think its just wrong for this rule to be reporting on toHaveLength(1) for the same reason we don't report on on that matcher for any other value aside from 0: they're semantically not the same, and there's no reliable way that we can say for sure which matcher is the right one to be using.

I'd say that should be our first focus, and then we can explore if it would make sense to expand the rule again to cover cases where we can know for sure that you should be using toBeInTheDocument, or maybe that we should actually have a new rule (i.e for these cases).

[SevenOutman]

@SevenOutman PRs are welcome

Sure. I've created a PR #273 for it. :)