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

[julienw]

import { render, getByText } from '@testing-library/react';
import React from 'react';

render(<div>some text</div>);

getByText(document.body, 'some text');

=> testing-library/prefer-screen-queries: Avoid destructuring queries from render result, use screen.getByTestId instead

In this case, we use the function imported from @testing-library/dom reexported from @testing-library/react that allows to search for an element in the tree of any other element (like within). This exact example is of course useless (we can use screen) but this is to show the problem with a simple testcase. I believe this is a valid use case and shouldn't be flagged by testing-library/prefer-screen-queries.

What do you think?

[Belco90]

Hey @julienw.

I believe this is pretty much the same idea behind allowing destructuring screen itself.

You can see in that issue we decided not to implement that functionality, since the purpose of using screen is to avoid destructuring the queries every time you need to use a new one or replace an existing one. So the same reason would apply here and it wouldn't be a valid case.

I agree the error doesn't fit really well for this particular case tho, but I'm not sure if it's worth it to report it differently for this scenario.

Does it make sense to you?

[julienw]

I'm a big fan of screen in general.

In this specific case, we wanted to use getByText on a specific subtree. For example:

const popup = screen.getByTestId("modal-dialog");
const element = getByText(popup, "label");
// Do something with element

This is an advanced use case, and probably we should really use within instead in this case, but I believe this is out of scope for prefer-screen-queries. Does that make sense?

The alternative is to just add an ignore comment for prefer-screen-queries for this specific line :-)

[MichaelDeBoey]

@julienw The scenario you're describing sounds indeed like a good use case for using within instead.

[gndelia]

Hi @julienw, thanks for opening the issue!. I agree that this sounds like a use case for within. However, there's something that I might be missing. You mentioned that you wanted to use getByText on a specific subtree, and that you use the re exported version from '@testing-library/react';.

Shouldn't something like this work too?

const element = screen.getByText(popup, "label");

If I understand correctly, the queries don't have any difference in the react package from the dom package, as they are just re-exported here

In that sense, your code would comply with the rule even if you don't use within (which I would recommend using, as this is its main purpose).

[julienw]

Hi @julienw, thanks for opening the issue!. I agree that this sounds like a use case for within. However, there's something that I might be missing. You mentioned that you wanted to use getByText on a specific subtree, and that you use the re exported version from '@testing-library/react';.

Exactly. Just like screen itself, and within too. They're all tools reexported from dom-testing-library.

Shouldn't something like this work too?

const element = screen.getByText(popup, "label");

This doesn't work, screen.getByText doesn't accept an Element as its first parameter, but the "global" query does.

BTW I noticed that renaming the query also avoids the rule:

import { getByText as globalGetByText } from '@testing-library/react';

globalGetByText(element, "text"); // Doesn't get noticed

[julienw]

@julienw The scenario you're describing sounds indeed like a good use case for using within instead.

Yes! But my point is that this rule prefer-screen-queries doesn't sound the right location to check this. Especially the error message doesn't look right in this case.

But TBH I'd be happy as well if you said "OK this works as expected" and I'd just have to use within or ignore the rule here. Maybe improving the error message for this case could be nice, and maybe mentioning this in the doc would be a good idea, in that case.

[Belco90]

I'd say this works as expected. The error message and the rule docs can be improved to clarify this and the destructuring screen cases. I'm leaving this issue open so we add corresponding doc at least.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[julienw]

I was thinking that another workaround is doing something like

import TestingLibrary, { render } from '@testing-library/react';
TestingLibrary.getByText(element, selector);

By avoiding the destructuring.