Feature request: convert no-get-by-for-checking-element-not-present to be prefer-appropriate-query-for-expect

[benmonro]

// reports:
expect(getByText("Foo")).not.toBeInTheDocument() // as it already does
expect(queryByText("Foo")).toBeInTheDocument()

// does not report:
expect(queryByText("Foo")).not.toBeInTheDocument()
expect(getByText("Foo")).toBeInTheDocument()

In addition as discussed below in the comments, the use of get* queries inside of waitForElementToBeRemoved will no longer report an error either in a callback or directly as a parameter.

  //does not report:
  await waitForElementToBeRemoved(screen.getByTitle("Loading..."));
  await waitForElementToBeRemoved(() => screen.getByTitle("Loading..."));

[Belco90]

I don't see any reference for this in the doc:

• waitForElementToBeRemoved doc has examples with queryBy* or callbacks.
• dom-testing-library v7 changelog has an example with an existing element inside, not an inline getBy*.

// This is on dom-testing-library v7 changelog example
const submitButton = screen.getByText(/submit/i)
// (at this point you know submitButton exists, otherwise getByText would throw an error)
await waitForElementToBeRemoved(submitButton)

// Which is not the same as
// (you don't know here if getBy* exists or not so it can throw within waitForElementToBeRemoved)
await waitForElementToBeRemoved(screen.getByText(/submit/i))

To be honest, even if the doc doesn't mention that, it could work fine but this is just an optional rule. It's not forcing anyone to follow this advice and it's more a best practices thing. I don't think this is an issue then.

[benmonro]

@Belco90 i like the rule actually when used in expects but this new approach is actually a very good practice and the rules should not discourage it.

Actually in your second example it is essentially the same. If it throws the test will fail which is fine and if it doesn't throw it will wait for the removal. So effectively it's the exact same code path.

I still maintain that

expect(queryByText("Foo")).toBeInTheDocument()

should report but

waitForElementToBeRemoved(getByText("Foo"))

should not.

Can we open this discussion up to the broader community before quickly closing the issue?

[Belco90]

Sure. But those 2 examples are not technically the same and it's explained why.

As mentioned, even if the getBy* works directly within waitForElementToBeRemoved this rules aims to avoid using it as a best practice, so I don't see the point of having a rule for not using getBy* which allows using getBy*. The only workaround I see is to make the rule configurable so you can choose if you want to disable the expect assert or the waitForElementToBeRemoved util.

[benmonro]

How is it a best practice to not use getBy with waitForElementToBeRemoved? Is there a scenario you had in mind when the rule was written? I'm of the opinion it actually is better to use getBy when something is supposed to exist because it gives you better erroring. In this case an element needs to exist before it can be removed so what am I missing?

[benmonro]

Also to be clear, the rule is for prefer get by for checking non existence. It's not a rule for not using get by as you commented.

[benmonro]

Also I was thinking the rule could be expanded to include using queryBy for checking existence.

Ie expect(queryByText("Foo")).toBeInTheDocument()

Should report, do you agree?

[Belco90]

The rule was originated by:

• disappearance recipe mentioning to use queryBy* rather than getBy*
• testing library examples not using getBy* (as per my first comment)
• improving a previous rule that wasn't working as expected. Original issue here

Also to be clear, the rule is for prefer get by for checking non existence. It's not a rule for not using get by as you commented.

I'm afraid not. By the rule name (no-get-by...) and rule details the idea of this rule is not to use getBy* for mentioned cases.

Also I was thinking the rule could be expanded to include using queryBy for checking existence.

Ie expect(queryByText("Foo")).toBeInTheDocument()

Should report, do you agree?

👆 Why? The rule is aiming just to test when checking element not present. In this example you are checking that an element is present. Additionally, there is no issue on asserting element existence with queryBy*.

Please take a look to all these docs and motivations for the rule and let me know.

[benmonro]

The issue on checking presence with queryBy is it does not give you good errors when it fails to find the element in the way that getBy or findBy does. Would you be open to it being in a septate rule?

With regards to the use of getBy in waitForElementToBeRemoved I'd be curious to hear from @kentcdodds on if there's any reasons to use queryBy over getBy. It seems to work just fine to me...

[Belco90]

I'm fine to get another issue for a new rule to prefer asserting with getBy* over queryBy*, and even better if you create a PR :)

Again, even if waitForElementToBeRemoved works within inline getBy (it used to work for me some versions ago but didn't try on this one) this is about a good practice to follow. It would be similar to use awaitForElement + getBy* queries on prev versions: it works, but it's a best practices to use findBy* directly.

[benmonro]

Yeah I'll file a new request for that. 👍

Using get by in a waitForElementToBeRemoved isn't the same as using findBy instead of waitFor + queryBy

Still not seeing how it's a best practice but maybe Kent will give some insight

[kentcdodds]

Actually, these are the same thing:

// this:
const submitButton = screen.getByText(/submit/i)
await waitForElementToBeRemoved(submitButton)

// is the same as this:
await waitForElementToBeRemoved(screen.getByText(/submit/i))

Both of those should work equally well.

The disappearance recipe works just as well with get* as it does with query* and I wouldn't have a rule about that either way.

I believe this should report:

// reports:
expect(queryByText("Foo")).toBeInTheDocument()

// does not report:
expect(queryByText("Foo")).not.toBeInTheDocument()
expect(getByText("Foo")).toBeInTheDocument()

[Belco90]

Both should work equally well is not the same as they are the same thing tho. In first example you are 100% sure nothing will throw within waitForElementToBeRemoved. In the second, it can throw inside waitForElementToBeRemoved. If waitForElementToBeRemoved handles the error, they work equally as you said, but they are not technically the same thing. This is what I meant.

If waitForElementToBeRemoved handles getBy* properly, I don't find proper reference in docs or release changelogs. I always understood by docs and examples that as a general rule is better to avoid queries that could throw inside other methods (and what this rule aimed). If this is not the case, I think this should be updated in the docs too.

I don't get why a rule called no-get-by-for-checking-element-not-present should report about no query by for checking element is present, which is completely the opposite. Can you elaborate on that?

[kentcdodds]

I think you're confused @Belco90. Look carefully:

// this:
const submitButton = screen.getByText(/submit/i)
await waitForElementToBeRemoved(submitButton)

// is the same as this:
await waitForElementToBeRemoved(screen.getByText(/submit/i))

Just like:

const foo = 'hi'
console.log(foo)

// is the same as this:
console.log('hi')

[Belco90]

@kentcdodds I don't think is comparable as we are talking about getBy* queries which can throw, not a simple assignment, but nevermind.

[benmonro]

I don't get why a rule called no-get-by-for-checking-element-not-present should report about no query by for checking element is present, which is completely the opposite. Can you elaborate on that?

Personally I would vote for changing that rule name to prefer-appropriate-query-for-expect or something to that effect, rather than having 2 separate rules as ultimately they are just inverses of the same thing.

[kentcdodds]

I'm sorry I'm not able to communicate this effectively to you @Belco90, but you're still misunderstanding. I think you're reading it like this:

// this:
const submitButton = screen.getByText(/submit/i)
await waitForElementToBeRemoved(submitButton)

// is NOT the same as this (I think this is what you're thinking we're doing, but it's not):
await waitForElementToBeRemoved(() => screen.getByText(/submit/i))

// but it IS the same as this:
await waitForElementToBeRemoved(screen.getByText(/submit/i))

[Belco90]

I don't get why a rule called no-get-by-for-checking-element-not-present should report about no query by for checking element is present, which is completely the opposite. Can you elaborate on that?

Personally I would vote for changing that rule name to prefer-appropriate-query-for-expect or something to that effect, rather than having 2 separate rules as ultimately they are just inverses of the same thing.

If we end up just leaving only the expect check, I agree on that (tho it would mean a breaking change).

@kentcdodds yeah I noticed it's not wrapped withing a function anymore. I feel I can't communicate properly what I want, but if the method works properly using getBy* queries directly and there is any kind of disadvantage on doing it, it's not worth it try to rephrase myself. What I want to make sure is: the getBy* query works fine within waitForElementToBeRemoved only after dom-testing-library v7 or has been fine always? If the former, then the rule would be still useful for prev versions I guess.

[benmonro]

@Belco90 I've personally seen it work just fine previous to v7. we have tests that do this:

waitForElementToBeRemoved(() => getByText("foo"))

[Belco90]

Then I'll close #91 in favour of of repurposing this one to convert the rule into a different one.

[benmonro]

cool thanks @Belco90 !

[benmonro]

I'll update the description to include that as well.

[benmonro]

@Belco90 updated the description, let me know if that sounds good to you?

[Belco90]

So just to sum up:

• we remove current no-get-by-for-checking-element-not-present as waitForElementToBeRemoved doesn't need to be tested within that rule.
• we add a new rule in replacement of previous mentioned one, called prefer-appropriate-query-for-expect which tests: 1) presence of elements is checked using getBy* query (because queryBy* error info is worse) and 2) absence of elements is checked using queryBy* (as current rule was checking before and for same reasons).

Is that right?

[Belco90]

I think we should improve the name, as this doesn't apply to all expects in general but only those checking presence/absence of elements.

[benmonro]

Yeah agreed just couldn't think of a better name.

Everything you summed up looks good to me

[Belco90]

Can't come up with something better than prefer-appropriate-expect-presence-query.

[Belco90]

This shouldn't be really complex to implement to be honest, but I'd like to wrap this together these other changes for v3:

• new rule prefer-wait-for feat(prefer-wait-for): new rule prefer-wait-for #88
• update rule await-async-utils feat(await-async-utils): reflect waitFor changes #89
• new rule no-wait-for-empty-cb. Still pending to be implemented, but would be a rule to complain about empty callbacks passed to waitFor.

[benmonro]

I'd say less is more just go with prefer-appropriate-presence-query but yours is fine too if you feel strongly about it.

[Belco90]

🎉 This issue has been resolved in version 3.0.0 🎉

The release is available on:

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

Your semantic-release bot 📦🚀