fix: ignore getBy inside of within for prefer-presence-queries on abs…

[nathanmmiller]

…ence queries

Checks

• I have read the contributing guidelines.

Changes

• Ignore getBy* nodes inside of within() for prefer-presence-queries when checking absence queries
• Added additional test case based on comments on fix: ignore getBy inside of within for prefer-presence-queries on absence queries #716

Context

Fixes #518 [Well, partially fixes it.]

[Belco90]

Hi again @nathanmmiller! Thanks for adding the extra test. Let's add tests to cover all combinations:

Checking absence:

• with(getBy).getBy() --> invalid (added already)
• with(getBy).queryBy() --> valid (added already)
• with(queryBy).getBy() --> invalid
• with(queryBy).queryBy() --> valid

Checking presence:

• with(getBy).getBy() --> valid
• with(getBy).queryBy() --> invalid
• with(queryBy).getBy() --> valid
• with(queryBy).queryBy() --> invalid

[nathanmmiller]

Hi again @nathanmmiller! Thanks for adding the extra test. Let's add tests to cover all combinations:

Checking absence:

• with(getBy).getBy() --> invalid (added already)
• with(getBy).queryBy() --> valid (added already)
• with(queryBy).getBy() --> invalid
• with(queryBy).queryBy() --> valid

Checking presence:

• with(getBy).getBy() --> valid
• with(getBy).queryBy() --> invalid
• with(queryBy).getBy() --> valid
• with(queryBy).queryBy() --> invalid

@Belco90 Hm now that I look at this list, I think I disagree with the implementation (which might mean more work for me) but I don't think "within(queryBy)..." is valid in either case. Surely "within" should be treated much the same way as a presence query?

Thoughts? I'm happy to add the 6 tests above - which will pass - but in seeing you write them out, I think that it's wrong that they should pass and I should only ignore within() if the inside is getBy.

[Belco90]

@nathanmmiller I think those examples and reporting the latest query chained from the within util makes sense.

Maybe it's easier to see with this example:

const modal = screen.getByRole('dialog')
const { queryByRole } = within(modal)
expect(queryByRole('button', { name: 'submit' })).toBeInTheDocument()

We need to report that query* query to be replaced by a get* query. Does it make more sense this way?

[nathanmmiller]

@nathanmmiller I think those examples and reporting the latest query chained from the within util makes sense.

Maybe it's easier to see with this example:

const modal = screen.getByRole('dialog')
const { queryByRole } = within(modal)
expect(queryByRole('button', { name: 'submit' })).toBeInTheDocument()

We need to report that query* query to be replaced by a get* query. Does it make more sense this way?

So this makes perfect sense to me and I completely agree - I am not worried about that use case, I believe it's already covered by my original test (in the form of expect(within(blah).queryBy*).toBeIn - being wrong).

What I'm saying is that I think
expect(within(queryBy*).getBy).toBeIn...
is wrong. Not because of the getBy-toBeIn match (this is correct), but because of the within(queryBy) - that is to say, if a "queryBy" is wrapped inside a within, that's wrong - because within implies presence - it's wrong in the same way that expect(queryBy).toBeIn is wrong.

So I believe my initial PR is wrong and if you agree, I will add more comprehensive unit tests to cover this as well, and modify it so that it's reported correctly.

In essence, the unit tests I want to have in the end are:
Checking absence:

• within(getBy).getBy() --> invalid (added already)
• within(getBy).queryBy() --> valid (added already)
• within(queryBy).getBy() --> 2x invalid because within(queryBy) is wrong AND because getBy is wrong
• within(queryBy).queryBy() --> invalid because within(queryBy) is wrong NOT because queryBy is wrong (it's right)

Checking presence:

• within(getBy).getBy() --> valid
• within(getBy).queryBy() --> invalid
• within(queryBy).getBy() --> invalid because within(queryBy) is wrong NOT because getBy is wrong (it's right)
• within(queryBy).queryBy() --> 2x invalid because within(queryBy) is wrong AND because queryBy is wrong

[Belco90]

@nathanmmiller ooh I get it now! You are totally right, let's go for the behaviour described in your previous comment then.

[nathanmmiller]

I have reopened this PR at #740 because of a complete mess when rebasing.