Cypress 12.0.0 compatibility

BlueWinds commented 1 year ago

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. https://github.com/cypress-io/cypress-documentation/pull/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.

cypress-testing-library could do a breaking change. 9.x for Cypress 12+, and leave 8.x for Cypress <=11.
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.
[x] 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.
[x] 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 commented 1 year ago

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 commented 1 year ago

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

NicholasBoll commented 1 year ago

I'm looking into this now

NicholasBoll commented 1 year ago

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.

cypress-testing-library could do a breaking change. 9.x for Cypress 12+, and leave 8.x for Cypress <=11.

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 commented 1 year ago

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:

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.
Cypress begins executing the next command (findByTestId), and gets a specific DOM element. We wait for a promise to resolve.
Vue re-renders the list.
The element we had from findByTestId is now stale. We can't click it.
.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:

.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 commented 1 year ago

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 commented 1 year ago

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

the-ult commented 1 year ago

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 commented 1 year ago

That was fixed in #233

the-ult commented 1 year ago

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

NicholasBoll commented 1 year ago

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

BlueWinds commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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?

BlueWinds commented 1 year ago

@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 commented 1 year ago

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

NicholasBoll commented 1 year ago

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 commented 1 year ago

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.

KatieMFritz commented 1 year ago

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[bot] commented 1 year ago

:tada: This PR is included in version 9.0.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

BlueWinds commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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.

apolonskiy commented 1 year ago

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

olegg-gocheetah commented 1 year ago

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 commented 1 year ago

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

zunjooo commented 1 year ago

@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 commented 1 year ago

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

farhanhelmy-sr commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

rickythefox commented 11 months ago

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'

testing-library / cypress-testing-library

Cypress 12.0.0 compatibility #238