-
Notifications
You must be signed in to change notification settings - Fork 153
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Revert changes to query*s and add deprecation warning #117
Comments
I don't mind taking this on since I broke it. I can finish that today. |
Thanks @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 According to the list of releases:
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):
From what your issue was, I'm guessing the 2nd is causing your issue. If the 3rd was causing issues, using 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 |
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? |
I couldn't find an issue reproduction repository. You could fork this one. I do have the custom command you provided (added to the 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. |
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: 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. |
@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 |
Perfect, thanks @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). |
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 I'm not sure where the root cause of this being a breaking change, some part of the interaction between 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. |
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... :( |
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. |
I know why this is happening now. I changed // always worked
cy.queryByText('Never Exists').should('not.exist')
// Broke on the `queryByText` command since using `verifyUpcomingAssertions`
cy.queryByText('Never Exists').then(val => {
cy.wrap(val).should('not.exist')
}) The reason the I'm very sorry for the inconvenience. I'm glad the Cypress Testing Library I'll leave this issue open a bit more. Perhaps it is still safer to revert the |
* fix: Revert to previous query* API Closes #117
🎉 This issue has been resolved in version 5.3.1 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
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 :)The text was updated successfully, but these errors were encountered: