False negative for testing-library/prefer-screen-queries

[julienw]

Hey!

When handling the new failures in our projects due to adding testing-library/prefer-screen-queries, I noticed some patterns weren't noticed by the rule. Strangely other similar patterns were noticed.

This pattern is when returning the result of render, and then destructuring in the caller, ie some variations of:

function setup() {
  return render(<Foo />);
}

// Then in a test
const { getByText } = setup();
getByText("foo");

I couldn't find what difference these cases, so I'll rather pinpoint existing code in our project.

This was noticed by the rule:
https://github.com/firefox-devtools/profiler/blob/1c5dfed055f864413b3a62e43ca2d27b074d6108/src/test/components/TrackContextMenu.test.js#L64-L73
And everything in this file
https://github.com/firefox-devtools/profiler/blob/1c5dfed055f864413b3a62e43ca2d27b074d6108/src/test/components/ZipFileTree.test.js#L56-L70
And everything in this file

This wasn't noticed by the rule:
https://github.com/firefox-devtools/profiler/blob/1c5dfed055f864413b3a62e43ca2d27b074d6108/src/test/components/ButtonWithPanel.test.js#L80-L82
(and below in the same file)
https://github.com/firefox-devtools/profiler/blob/1c5dfed055f864413b3a62e43ca2d27b074d6108/src/test/components/CallNodeContextMenu.test.js#L107-L112

There are other examples, please tell me if you want me to give you all cases.

Initially I thought this was because of the somewhat complex setup, but then I was surprised to see that some cases were actually caught by the rule...

Thanks!

[Belco90]

Hi again! Could you confirm which version of the plugin you are using please? If using v4, could you share the config for the plugin too?

[julienw]

Yes sure, this is with version 4.1.2.

And this is the config relevant to this plugin:

module.exports = {
  env: {
    jest: true,
  },
  plugins: ['testing-library'],
  extends: [
    'plugin:testing-library/react',
  ],
  rules: {
    // Adding more errors now
    'testing-library/no-manual-cleanup': 'error',
    'testing-library/no-wait-for-snapshot': 'error',
    'testing-library/prefer-explicit-assert': 'error',
    'testing-library/prefer-presence-queries': 'error',
    'testing-library/prefer-wait-for': 'error',

    // Disable some rules that are in the "recommended" part.
    // This is a purely stylistic rule
    'testing-library/render-result-naming-convention': 'off',
    // This disallows using `container`, but this is still useful for us sometimes
    'testing-library/no-container': 'off',
    // This disallows using direct Node properties (eg: firstChild), but we have
    // legitimate uses:
    'testing-library/no-node-access': 'off',
    // Disable until https://github.com/testing-library/eslint-plugin-testing-library/issues/359
    // is fixed.
    'testing-library/await-async-query': 'off',
  },
};

I believe testing-library/prefer-screen-queries is enabled automatically when extending 'plugin:testing-library/react', right?

[Belco90]

At first sight, this looks like a bug. However, I can't figure out what could be the cause of the issue, so this needs further investigation.

[julienw]

So I tried to find smaller testcases. In our project we use a custom module, and we make a heavy use of "setup" functions returning getByXXX functions. But as far I understand this plugin should agressively find them because we didn't disable the agressive behavior.
I think that all cases where the plugin wouldn't find an incorrect usage in our code comes from this pattern. All usages where we use the direct result of render have been found as far as I know.

So, here are the weird behaviors.

First weird behavior:

import * as React from 'react';
import { render } from 'firefox-profiler/test/fixtures/testing-library';

function setup() {
  return render(<div />);
}

it('detected', async () => {
  const { getByText } = await setup();

  expect(getByText('foo')).toBeInTheDocument(); // detected
});

it('undetected', () => {
  const { getByText } = setup();

  expect(getByText('foo')).toBeInTheDocument(); // undetected
});

As you see, the only difference is the await in the first test block. In this case, the second getByText isn't detected at all, even when the first block is removed.

However, here is a weirder case. Here both cases are undetected. The first block is the same as the one above, but the second block is a new one, where we don't destructure the assignment in the same line as the call to setup.

import * as React from 'react';
import { render } from 'firefox-profiler/test/fixtures/testing-library';

function setup() {
  return render(<div />);
}

it('undetected 1', () => {
  const { getByText } = setup();

  expect(getByText('foo')).toBeInTheDocument(); // undetected
});

it('undetected 2', () => {
  const results = setup();
  const { getByText } = results;

  expect(getByText('foo')).toBe('foo'); // undetected
});

But when I remove the first block:

import * as React from 'react';
import { render } from 'firefox-profiler/test/fixtures/testing-library';

function setup() {
  return render(<div />);
}

it('detected', () => {
  const results = setup();
  const { getByText } = results;

  expect(getByText('foo')).toBe('foo'); // detected ?!
});

The block is the exact same as the second one in the previous example.

The same thing happens when I take the first example above, but switch the 2 blocks: then nothing is detected.

Note that when I use the 2 "detected" blocks both are properly detected.
So it looks like that the "undetected" block somehow shadows the following blocks that would otherwise be detected.

Could that be a problem around this tool?

eslint-plugin-testing-library/lib/rules/prefer-screen-queries.ts

Lines 66 to 78 in 632b492

	function saveSafeDestructuredQueries(node: TSESTree.VariableDeclarator) {
	if (isObjectPattern(node.id)) {
	for (const property of node.id.properties) {
	if (
	isProperty(property) &&
	ASTUtils.isIdentifier(property.key) &&
	helpers.isBuiltInQuery(property.key)
	) {
	safeDestructuredQueries.push(property.key.name);
	}
	}
	}
	}

In short, there are 2 problems here:

1. some patterns aren't detected when they should be.
2. when they aren't detected they seem to shadow and hide other cases.

I hope these shorter cases will help!

[Belco90]

Oh wow, amazing analysis! This is really helpful, thank you so much.

Interesting findings. It could definitely be the code bit you highlighted. I hope I can spend some time soon checking this.

[Belco90]

Ok, I think this is related to #372 and "just" reworking the rule internally will handle this properly. I'm working on this, I'll get back when I've refactored the rule to see if this is solved.

[leosuncin]

Another example of false positive, it fails when import queries from @testing-library/react

import { render, screen, queries } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import ListTodo from 'components/todo/list-todo';
import React from 'react';
import { randomArrayElement } from 'utils/factories';
import { todoBuild } from 'utils/factories';

describe('<ListTodo />', () => {
  it('should list todos', () => {
    const spyChangeTodo = jest.fn();
    const spyRemoveTodo = jest.fn();
    const todos = Array.from({ length: 10 }, () =>
      todoBuild({ traits: 'old' }),
    );
    const todo = randomArrayElement(todos);

    render(
      <ListTodo
        todos={todos}
        onChangeTodo={spyChangeTodo}
        onRemoveTodo={spyRemoveTodo}
      />,
    );
    userEvent.click(
      // I need to click on checkbox inside a listitem, so I pass the listitem as container to getByRole
      queries.getByRole(screen.getByRole('listitem', { name: RegExp(todo.text) }), 'checkbox'),
     // Avoid destructuring queries from `render` result, use `screen.getByRole` instead  testing-library/prefer-screen-queries
    );

    expect(spyChangeTodo).toHaveBeenCalledWith(todo.id, { done: !todo.done });

    userEvent.click(
      screen.getByRole('button', { name: `Delete todo: ${todo.text}` }),
    );

    expect(spyRemoveTodo).toHaveBeenCalledWith(todo, expect.any(Number));
  });
});

And another one, it fails when import getByRole from @testing-library/react

import {
  getByRole,
  render,
  screen,
  waitForElementToBeRemoved,
} from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import Todo from 'components/todo';
import React from 'react';
import { randomArrayElement } from 'utils/factories';
import server from 'utils/test-server';

describe('<Todo />', () => {
  beforeAll(() => server.listen());

  afterEach(() => server.resetHandlers());

  afterAll(() => server.close());

  it('should remove one todo', async () => {
    render(<Todo />);

    await waitForElementToBeRemoved(screen.getByTestId('loading-todos'));

    // Get random todo item
    const element = randomArrayElement(screen.getAllByRole('listitem'));
    // Click button inside listitem, so I pass the listitem as container to getByRole
    userEvent.click(getByRole(element, 'button', { name: /Delete todo/ }));

    expect(element).not.toBeInTheDocument();
  });
});

[leosuncin]

The version I'm using is 4.2.0 with the next settings:

{
  "plugins": ["jest", "jest-formatting", "testing-library"],
  "env": {
    "jest": true
  },
  "extends": [
    "plugin:jest/recommended",
    "plugin:jest/style",
    "plugin:jest-formatting/strict",
    "plugin:testing-library/react"
  ]
}

[Belco90]

@leosuncin thanks for your comment. Let's see those examples:

First example. Interesting, I didn't remember about queries exported from the Testing Library package so that one could be probably allowed. However, in your scenario, you should actually use within, which is allowed by the rule:

const listItem = screen.getByRole('listitem', { name: RegExp(todo.text) })
userEvent.click(within(listItem).getByRole('checkbox'))

Second example. As discussed in #366, queries directly imported from the Testing Library package are reported correctly (although the error message could be improved). However, in the same way as the first example, you should actually use within:

const element = randomArrayElement(screen.getAllByRole('listitem'));
userEvent.click(within(element).getByRole('button', { name: /Delete todo/ }));

If you would like to discuss more details about this, could you move the thread to a new issue since it's not directly related to the problem described on this one, please?

[leosuncin]

Oh, I didn't see #366