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?

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?

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.

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?

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!

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.

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.

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();
  });
});

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"
  ]
}

@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?

Oh, I didn't see #366

Hey @julienw! I made some progress and identified the problem, but will take me a while to solve it. I basically got the rule refactored so it's easier to spot the issue, but there are different things to improve to solve the problem described here.

🎉 This issue has been resolved in version 4.6.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

Thanks so much for the fix !