Skip to content
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

Cypress 12.0.0 compatibility #238

Merged
merged 8 commits into from Dec 13, 2022
Merged

Conversation

BlueWinds
Copy link
Contributor

@BlueWinds BlueWinds commented Nov 14, 2022

Fixes #244

What:

Hello from the Cypress team! In Cypress 12.0.0, we'll be releasing a new API for custom commands: Cypress.Commands.addQuery. This is in order to resolve one of Cypress' oldest and most annoying issue, "Detached DOM".

Queries are a type of command, but simpler - Cypress' own command queue manages the retry loop, rather than relying on each implementation to call verifyUpcomingAssertions.

As the most popular plugin for Cypress, and by far the most popular plugin providing additional commands, I'm reaching out and see how you want to handle compatibility with Cypress 12.

Why:

Using Commands.addQuery instead of Commands.add has a couple of benefits:

  • Users of cypress-testing-library won't run into Detached DOM errors. This is the big one, any why I did the work!
  • Cypress handles retrying and assertions, simplifying your code.

If you want to learn more about them, check out the Cypress docs I also have in flight. cypress-io/cypress-documentation#4835. Here's a deployed version of them, with the new guide about queries I'm working on: https://deploy-preview-4835--cypress-docs.netlify.app/api/cypress-api/custom-queries

How:

Cypress.Commands.addQuery does not exist in Cypress 11, which means that as I've submitted it, this PR is breaking. I see a couple of options.

  1. cypress-testing-library could do a breaking change. 9.x for Cypress 12+, and leave 8.x for Cypress <=11.
  2. cypress-testing-library could do feature detection. if (Cypress.Commands.addQuery) { ...use code in this PR... } else { ...use old version... }.

The first option has the benefit of being small, without duplicated code or the burden of maintaining multiple versions of the plugin inside a single file. The second option has the benefit of allowing the same version of cy-test-lib to run with any version of Cypress.

If you prefer option 2, let me know and I'll update the PR in that direction.

Checklist:

  • [n/a] Documentation - N/A. This has no user-facing impact.
  • Tests - I've updated the existing mocks to reflect the new API. All the e2e tests still pass without changes, demonstrating that the change shouldn't affect end users.
  • Ready to be merged? No. I'm opening this to begin the conversation and show you the code changes - how you want to handle versioning is an open question. Also, Cypress 12.0.0 isn't out yet, so package.json in my fork points to a pre-release binary.

@BlueWinds
Copy link
Contributor Author

BlueWinds commented Nov 14, 2022

So there's currently an issue with our 12.0.0 pre-release (https://cdn.cypress.io/beta/npm/12.0.0/linux-x64/release/12.0.0-cc63b13085591b70826c909119784e7d358ea56e/cypress.tgz), referenced in the package.json - we're not properly including one of the babel dependencies in the binary, which means Cypress can't properly compile any specs. Whoops.

We're working on fixing this to get working binaries to test with, but I wanted to open the PR without waiting, so you have as much time as possible to look things over.

Issue resolved on the Cypress side, above no longer applies.

We're currently planning on putting out Cypress 12 on Dec 8th or thereabouts, so three weeks. Please let me know if there's anything I can do to help or you have any questions!

@BlueWinds
Copy link
Contributor Author

Updated to a new version of the Cy12 binary which resolves the previous issue - tests should now pass completely.

@NicholasBoll
Copy link
Contributor

I'm looking into this now

@NicholasBoll NicholasBoll self-assigned this Nov 15, 2022
@NicholasBoll
Copy link
Contributor

Cypress.Commands.addQuery does not exist in Cypress 11, which means that as I've submitted it, this PR is breaking. I see a couple of options.

  1. cypress-testing-library could do a breaking change. 9.x for Cypress 12+, and leave 8.x for Cypress <=11.
  2. cypress-testing-library could do feature detection. if (Cypress.Commands.addQuery) { ...use code in this PR... } else { ...use old version... }.

Option 1 would be a breaking change and would drop support for all versions of Cypress outside 12 and those peerDependencies would have to be removed from the package.json file. The testing library suite of projects don't allow supporting multiple versions simultaneously, so Cypress <= 11 support would be dropped entirely (no more big fixes without upgrading).

Option 2 would require multiple support paths which makes maintenance more costly and would require either multiple versions of Cypress to hit each code path, or a way that the tests could override.

Is Cypress 12 dropping support for the private functions used in this code? Like cy.verifyUpcomingAssertions?

@BlueWinds
Copy link
Contributor Author

BlueWinds commented Nov 15, 2022

Is Cypress 12 dropping support for the private functions used in this code? Like cy.verifyUpcomingAssertions?

We're not dropping support for it - custom commands will continue to work as they always have, including using verifyUpcomingAssertions. Most users won't see any problems if they simply continue using the existing version.

However, some will - including Cypress' own internal test suite. We're being a lot more rigorous about enforcing "elements must be attached to the DOM in order to be acted upon", so several previously passing tests involving scrolling around virtualized lists in our Vue components started to fail.

Something like

cy.findByTestId('list-item').scrollIntoView()
cy.findByTestId('list-item').click()

is more likely to fail:

  1. We find the list item, and start scrolling to it. scrollIntoView scrolls the page, and Vue begins to rerender the virtualized list, queueing it to be updated next tick.
  2. Cypress begins executing the next command (findByTestId), and gets a specific DOM element. We wait for a promise to resolve.
  3. Vue re-renders the list.
  4. The element we had from findByTestId is now stale. We can't click it.
  5. .click() retries on the stale element until it times out.

This is a completely classic "Detached DOM" error. We're just running into it a lot more frequently with Cypress 12 + cy-test-lib, because Cy12 checks for element actionability (including 'is attached to DOM') more frequently and reliably.

Queries prevent this by re-executing, replacing step 5:
5. .click() retries all the queries leading up to it to find a fresh subject - findByTestId returns the fresh element, and we can click it.

So that's the benefit - queries can fetch fresh elements from the page, updating the subject that future commands are using, while commands resolve once - when they eventually return a value, that value is "fixed", even if the DOM re-renders.

The cost is, as you've said, we place some plugins in a really awkward spot.

@NicholasBoll
Copy link
Contributor

I've been fixing bugs in version 8 in anticipation of a possible breaking change from this pull request. If we do decide to drop support for Cypress <=11, I might as well fix all outstanding issues first.

@BlueWinds
Copy link
Contributor Author

Let me know when you're done for the moment, I'll fix merge conflicts and include the changes you make in here. 👍

@the-ult
Copy link

the-ult commented Nov 17, 2022

This PR fixes the warnings for: cy.state('subject') as well. Correct?

Cypress Warning: `cy.state('subject')` has been deprecated and will be removed in a future release. Consider migrating to `cy.currentSubject()` instead.

@SimenB
Copy link
Contributor

SimenB commented Nov 17, 2022

That was fixed in #233

@the-ult
Copy link

the-ult commented Nov 17, 2022

I was still on 8.0.3 -> 8.0.7 has it fixed indeed. Thank you

@NicholasBoll
Copy link
Contributor

I'm done for the moment. I was looking at a debug limit option for larger pages, but it will take more work.

…ress 12+.

Set Cypress peerDependency to ^12.0.0
@BlueWinds
Copy link
Contributor Author

BlueWinds commented Nov 21, 2022

I'm done for the moment. I was looking at a debug limit option for larger pages, but it will take more work.

Updated to fix merge conflicts, and make sure everything you fixed is merged in cleanly.

I also used the commit message

BREAKING CHANGE: Use addQuery interface, which is only present in Cypress 12+.
Set Cypress peerDependency to ^12.0.0

which I believe should be the correct thing for semantic-release to trigger a breaking change. With that done, I think this is ready, save updating the devDependency to Cy12 once it's released.

Let me know if there's anything else I can help with / needs to be done.

@BlueWinds
Copy link
Contributor Author

@NicholasBoll - Cypress 12.0.0 is going out today, and has been published to NPM. It's not marked as latest yet, we're still rolling that out, but it's available and ready for use. I've bumped the dependency in package.json to use it instead of the pre-release binary.

Should be ready to merge/release this at your convenience.

@JasonFairchild
Copy link

I would like to upgrade to cypress 12, but I am getting these NPM errors from the cypress-testing-library after installing it. Is merging this going to cause problems for users of this library that are also on versions before cypress 12?
image

@BlueWinds
Copy link
Contributor Author

@JasonFairchild - Yes, this is a breaking change. Users of Cypress 11 and earlier will need to stay on cy-test-lib 8.x, while users on Cy 12+ will need to use cy-test-lib 9+.

@Belco90
Copy link
Member

Belco90 commented Dec 9, 2022

Tempted to click the merge button, but I'll wait for @NicholasBoll review 😅

@NicholasBoll
Copy link
Contributor

I'm evaluating today to make sure the error messages, logs, etc work as expected. Possibly add specifications for such messages if they do not exist.

@NicholasBoll
Copy link
Contributor

I like how the new addQuery cleans up the logic and the API abstraction seems to align well to make the code much more understandable. The only critique I have of the addQuery API is the forced use of the this keyword instead of addQuery giving access to a command API instead:

Cypress.Commands.addQuery('focused', (api, options = {}) => {
  api.setTimeout(...)
})

But otherwise it looks good. I checked Command log output and Cypress functionality when I increased timeouts to make sure everything looks good. I'm assuming since addQuery was used internally and @BlueWinds was the one to convert internal commands, everything was thought about. I didn't catch from the first commit that the error stack trace was fixed. This fix was back-ported.

Thank you for your fine work and I apologize for the delay.

I'll add the necessary steps to release a breaking change.

@NicholasBoll NicholasBoll added the BREAKING CHANGE This change will require a major version bump label Dec 13, 2022
Copy link
Contributor

@NicholasBoll NicholasBoll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution and your patience!

@NicholasBoll NicholasBoll merged commit 8c93575 into testing-library:main Dec 13, 2022
@KatieMFritz
Copy link

Thank you for all your work! I'm installing Cypress 12 for a new project today, and want to add the testing library. When do you anticipate these latest changes being available to install via npm? Should I link to a specific branch for now?

@github-actions
Copy link

🎉 This PR is included in version 9.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@BlueWinds
Copy link
Contributor Author

I like how the new addQuery cleans up the logic and the API abstraction seems to align well to make the code much more understandable. The only critique I have of the addQuery API is the forced use of the this keyword instead of addQuery giving access to a command API instead:

The use of this was an attempt to keep the arguments matching exactly to what the user provides - calling cy.get(a, b, c) results in the call of (a, b, c) - no extra magic parameters.

In retrospect I think it would have been better to break the symmetry and pass in the $Command instance as an argument rather than using it as this as you say, but hopefully the above also makes some sense.

Thank you for your fine work and I apologize for the delay.

I'll add the necessary steps to release a breaking change.

Thank you for being responsive and helpful! :)

@tracy-ash
Copy link

Not sure if I missed something that I need to do wrt this breaking change, but I'm now getting the error below when trying to run a spec.

Any help appreciated! Thanks

running versions:
@testing-library/cypress: "^9.0.0"
cypress: "^12.1.0"

CypressError
The following error originated from your test code, not from Cypress.

> Cypress.Commands.addQuery() is used to create new queries, but findAllByLabelText is an existing Cypress command or query, or is reserved internally by Cypress.

If you want to override an existing command or query, use Cypress.Commands.overrideQuery() instead.

When Cypress detects uncaught errors originating from your test code it will automatically fail the current test.

Cypress could not associate this error to any specific test.

We dynamically generated a new test to display this failure.[Learn more](https://on.cypress.io/api/custom-queries)
[node_modules/@testing-library/cypress/dist/add-commands.js:8:1](http://localhost:62349/__/#)
   6 |   command
   7 | }) => {
>  8 |   Cypress.Commands.addQuery(name, command);
     | ^
   9 | });
  10 | Cypress.Commands.add('configureCypressTestingLibrary', config => {
  11 |   (0, _.configure)(config);

@alexandrzavalii
Copy link

alexandrzavalii commented Dec 23, 2022

@tracy-ash same here, had to use the fixed cypress version 12.1.0

@testing-library/cypress: "^9.0.0"
cypress: "12.1.0"

@MarkLeMerise
Copy link

Not sure if I missed something that I need to do wrt this breaking change, but I'm now getting the error below when trying to run a spec.

Any help appreciated! Thanks

running versions: @testing-library/cypress: "^9.0.0" cypress: "^12.1.0"

CypressError
The following error originated from your test code, not from Cypress.

> Cypress.Commands.addQuery() is used to create new queries, but findAllByLabelText is an existing Cypress command or query, or is reserved internally by Cypress.

If you want to override an existing command or query, use Cypress.Commands.overrideQuery() instead.

When Cypress detects uncaught errors originating from your test code it will automatically fail the current test.

Cypress could not associate this error to any specific test.

We dynamically generated a new test to display this failure.[Learn more](https://on.cypress.io/api/custom-queries)
[node_modules/@testing-library/cypress/dist/add-commands.js:8:1](http://localhost:62349/__/#)
   6 |   command
   7 | }) => {
>  8 |   Cypress.Commands.addQuery(name, command);
     | ^
   9 | });
  10 | Cypress.Commands.add('configureCypressTestingLibrary', config => {
  11 |   (0, _.configure)(config);

For @tracy-ash and anyone else running into this, be sure that the side-effect import "@testing-library/cypress/add-commands is only ever called once. I was unintentionally calling it multiple times by importing other helper functions I had put in the commands.ts module, so every import caused this side-effect import to be called again. Once I ensured it was only called once, everything began working again.

On a side note, this is with Cypress 12.2.0 and @testing-library/cypress 9.0.0.

@zunjooo
Copy link

zunjooo commented Jan 19, 2023

Had a same issue "> Cypress.Commands.addQuery() is used to create new queries, etc." and by incrementing cypress version to this one, all works smoothly again.

image

@apolonskiy
Copy link

Still having issue above, with latest Cypress or lower versions (like 12.1.0). Could someone advice?

@olegg-gocheetah
Copy link

olegg-gocheetah commented Feb 15, 2023

For me it happens when I run all specs, when running one by one it seems to be working fine.

"@testing-library/cypress": "^9.0.0",
"cypress": "^12.5.1",

@tadaslinge
Copy link

@olegg-gocheetah @apolonskiy @zunjooo have you found any solution for issue "> Cypress.Commands.addQuery() is used to create new queries, etc."

@zunjooo
Copy link

zunjooo commented Mar 24, 2023

@tadaslinge I did not have this problem since the version upgrade, as I mentioned above - now I am on v12.8.1 and still everything regarding this issue is okay.

@farhanhelmy-sr
Copy link

I'm on "cypress": "^12.10.0", this problem still exist

@farhanhelmy-sr
Copy link

Not sure if I missed something that I need to do wrt this breaking change, but I'm now getting the error below when trying to run a spec.
Any help appreciated! Thanks
running versions: @testing-library/cypress: "^9.0.0" cypress: "^12.1.0"

CypressError
The following error originated from your test code, not from Cypress.

> Cypress.Commands.addQuery() is used to create new queries, but findAllByLabelText is an existing Cypress command or query, or is reserved internally by Cypress.

If you want to override an existing command or query, use Cypress.Commands.overrideQuery() instead.

When Cypress detects uncaught errors originating from your test code it will automatically fail the current test.

Cypress could not associate this error to any specific test.

We dynamically generated a new test to display this failure.[Learn more](https://on.cypress.io/api/custom-queries)
[node_modules/@testing-library/cypress/dist/add-commands.js:8:1](http://localhost:62349/__/#)
   6 |   command
   7 | }) => {
>  8 |   Cypress.Commands.addQuery(name, command);
     | ^
   9 | });
  10 | Cypress.Commands.add('configureCypressTestingLibrary', config => {
  11 |   (0, _.configure)(config);

For @tracy-ash and anyone else running into this, be sure that the side-effect import "@testing-library/cypress/add-commands is only ever called once. I was unintentionally calling it multiple times by importing other helper functions I had put in the commands.ts module, so every import caused this side-effect import to be called again. Once I ensured it was only called once, everything began working again.

On a side note, this is with Cypress 12.2.0 and @testing-library/cypress 9.0.0.

How to fix this?

@CarleneCannon-Conner
Copy link

CarleneCannon-Conner commented Jul 4, 2023

Hiya

I'm on cypress: 2.16.0 & @testing-library/cypress 9.0.0, looks as though this problem still exists

CypressError
The following error originated from your test code, not from Cypress.
  > Cypress.Commands.addQuery() is used to create new queries, but findAllByLabelText is an existing Cypress command or query, or is reserved internally by Cypress.
 If you want to override an existing command or query, use Cypress.Commands.overrideQuery() instead.
When Cypress detects uncaught errors originating from your test code it will automatically fail the current test.
Cypress could not associate this error to any specific test.
We dynamically generated a new test to display this failure.


[node_modules/@testing-library/cypress/dist/add-commands.js:8:1](http://localhost:9095/__/#)
   6 |   command
   7 | }) => {
>  8 |   Cypress.Commands.addQuery(name, command);
     | ^
   9 | });
  10 | Cypress.Commands.add('configureCypressTestingLibrary', config => {
  11 |   (0, _.configure)(config);

@CarleneCannon-Conner
Copy link

Apologies I misunderstood the error I reported above. I was able to fix my issue. Prior to the migration efforts from v10.11.0 to v12.16.0 we had import '@testing-library/cypress/add-commands'; in each of the test files where tests used those commands. I understand now that in v12.16.0 we need only import '@testing-library/cypress/add-commands' in our /cypress/support/commands.js file. See https://testing-library.com/docs/cypress-testing-library/intro/#usage for more info. Hopes this helps anyone else who has similar issues.

@AnkaNik
Copy link

AnkaNik commented Jul 24, 2023

Hi. I have same issue when try to use synpress as a cypress plugin.
version cypress 12.7.1

@rickythefox
Copy link

Fixed this by monkey-patching the function before importing testing library:

Cypress.Commands.addQueryOriginal = Cypress.Commands.addQuery;
Cypress.Commands.addQuery = (name, command) => {
  if (cy[name]) {
    Cypress.Commands.overwriteQuery(name, command)
  } else {
    Cypress.Commands.addQueryOriginal(name, command)
  }
}
import '@testing-library/cypress/add-commands'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BREAKING CHANGE This change will require a major version bump released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Cypress 12 as an acceptable peer dependency