Revert changes to query*s and add deprecation warning

[kentcdodds]

As noted in #108, we accidentally broke the query* queries. We should restore those to the original behavior and add a deprecation warning. Let's try to make it subtle enough to not be really annoying, but present enough to be noticed :)

[NicholasBoll]

I don't mind taking this on since I broke it. I can finish that today.

[kentcdodds]

Thanks @NicholasBoll!

[NicholasBoll]

@andycarrell Could you update #117 with some reproduction steps? I've been trying to reproduce the problem and haven't been able to. I'm positive I was able to reproduce before, but I must be missing something. I've tried the following with many versions of this library:

    cy.queryByText('Eventually not exists').then($el => {
      cy.wrap($el).should('not.exist')
    })

The button with "Eventually not exists" is in the index.html of this document and gets removed by a timer after 500ms. I've also tried with your custom command: retryUntilDoesNotExist().

According to the list of releases:

• 5.1.0: Support running commands against the previous yielded subject #100 adding support for previous subjects
• 5.1.1: fix: remove extra command log message for get* queries #106 removing an extra log in get* queries since forcibly failing results in a log
• 5.1.2: reverted Support running commands against the previous yielded subject #100
• 5.2.0: feat: add more helpful debugging information to queries #108 add more useful debugging and polish. Closes a bunch of previous issues. Includes a fallback on previous functionality before Support running commands against the previous yielded subject #100 (allowing previous subject queries, falling back to no previous subject for code relying on the functionality that all queries start from document)
• 5.2.1: Fix chain commands with previous subject different of document, element or window #115 removes previous subject requirement used in Support running commands against the previous yielded subject #100 and feat: add more helpful debugging information to queries #108. This prevents Cypress from throwing errors if the subject returned from a previous chain wasn't a document, window or element type. This was preventing the fallback from working in certain cases

You mentioned you bumped from 5.1.1 to 5.2.0.

From what I can tell there are 3 differences between #100 and #108 (main differences between 5.1.1 and 5.2.0):

• query* queries that fail @testing-library/dom get errors attached and fail vs returning {} when an element is not found (caused .click() to throw "needs an element" error vs query* erroring)
• query* runs verifyUpcomingAssertions rather than just returning a value (normalizes logic flow between query* and find*)
• feat: add more helpful debugging information to queries #108 runs a fallback for pre-Support running commands against the previous yielded subject #100 behavior

From what your issue was, I'm guessing the 2nd is causing your issue. If the 3rd was causing issues, using find* + .should('not.exist') would also fail. I'm not sure how the first could cause an issue. Technically #100 uses query* queries from @testing-library/dom and #108 uses get* queries from @testing-library/dom to get errors. Perhaps there is a subtle difference there? get* queries are used to get more useful error messages and run a retry loop until an error is no longer thrown (eventually finds the element)

I guess there is another subtle difference. #108 allows timeout overrides for retry loops where #100 did not. Perhaps there's a issue there?

It would really help to narrow down what previous functionality is required. I could simply revert any changes to the query* queries, but I'm not sure what tests to run so the original functionality isn't changed again.

[andycarrell]

Ah that's subtly different to what we have:

Cypress.Commands.add(
	"retryUntilDoesNotExist",
	{ prevSubject: true },
	subject => {
		cy.wrap(subject).should("not.exist");
	},
);

// in the test:
cy.queryByText('Eventually not exists').retryUntilDoesNotExist();

Can you try that?

I can try reproduce this if I get some time in the next couple of days, is there an example repo with Cypress && testing library already setup?

[NicholasBoll]

I couldn't find an issue reproduction repository. You could fork this one. query* specs are in the cypress/integration/query_spec.js file. The page that is run against is in the cypress/fixtures/index.html file. The "Eventual not exists" button is in there with a scrip tag that removes it from the DOM after 500ms. I think that's a really good starting point.

I do have the custom command you provided (added to the cypress/support/index.js file, but it seems to behave the same way and it seems to work. I must be missing something.

I'd like to fix the issue, but I'd also like to have a test that proves it works/doesn't work. When I said the behavior was unspecified, I meant by a test.

[ktsosno]

Hi - possibly related here. It looks like the getBy* commands were removed from the type definitions but are still supported calls. We swapped to queryBy* in the interim.

Docs:
https://testing-library.com/docs/dom-testing-library/api-queries

Types:
https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/testing-library__cypress/index.d.ts

I know it's possible someone else mismatched these types given the nature of DefinitelyTyped, but figured you'd like to know if there aren't plans to deprecate getBy calls.

[NicholasBoll]

@ktsosno AFAIK, they are only removed from the Cypress version, not others. There are adapters for DOM, React, Vue, etc.

The docs for Cypress Testing Library are here: https://testing-library.com/docs/cypress-testing-library/intro

There is a note about the lack of get* queries. At runtime, they throw an error warning to use a different query type.

[ktsosno]

Perfect, thanks @NicholasBoll!

[NicholasBoll]

@andycarrell Are you able to give a example repo to reproduce? I'm unsure of what functionality to revert back to: 5.0.1 or 5.1.0 (5.1.2 should be the same as 5.0.1).

[andycarrell]

Hi @NicholasBoll - sorry for the delay, I haven't had time to look further into this until now.

I've managed to recreate the pass/fail scenario, (passes on v5.1.2, fails on master). More specifically, it was the case of using retryUntilDoesNotExist on an element that already doesn't exist. I think this is a legitimate use case, since the test is specifying that the element is eventually gone, not whether it is gone at the moment the assertion is called.

I'm not sure where the root cause of this being a breaking change, some part of the interaction between queryByText and cy.wrap ?

Here's the test :)

  it('queryByText - reproduction', () => {
    const text = 'Supercalifragilistic'

    cy.queryByText('Eventually not exists').then($el => {
      cy.wrap($el).should('not.exist')
    })
    cy.queryByText(`Currently does not exist - ${text}`).then($el => {
      cy.wrap($el).should('not.exist')
    })
  })

Additionally, I'm not even sure if it's worth reverting this change, if it just going to be slated for the next major version - I'm not even sure of the wider impact currently, and reverting it would just delay the pain to the next major version update.
I think the case presented is a legitimate and useful case for queryBy* still returning null and it's a bit punishing that the original change (v5.2) removes this 👐

[NicholasBoll]

That must be what I was missing. It seems to be working with eventually not existing, but failing on does not exist now. I can reproduce again! Thank you!

There is a risk that reverting the change will break those relying on how 5.2 works now... :(

[andycarrell]

There is a risk that reverting the change will break those relying on how 5.2 works now... :(

Yeh for sure @NicholasBoll - and any later versions :(

Personally, we've updated our failing tests, and are now on v5.3.0, so the outcome of this shouldn't affect our usage.