Skip to content

feat: Create public Cypress.ensure API for use with custom queries #24697

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

Merged
merged 14 commits into from
Nov 29, 2022

Conversation

BlueWinds
Copy link
Contributor

@BlueWinds BlueWinds commented Nov 15, 2022

User facing changelog

Creates and adds types to Cypress.ensure API, which can be useful for creating custom queries. This new API is based off the previously undocumented cy.ensure* methods, which are now removed.

In addition, adds public types to cy.now(), which is again useful for custom queries.

Additional details

All the methods on Cypress.ensure are pre-existing, and were already on cy. This PR updates the arguments slightly for consistency, moves many of them to Cypress, and and adds publicly-facing types. Those that didn't end up on Cypress.ensure are split up around the codebase near where they're used.

These are referenced in the Custom Query documentation, which is "why this, why now." cypress-io/cypress-documentation#4835

Steps to test

Type tests have been added. All these commands are also used internally.

How has the user experience changed?

No change to the user experience for the vast majority of users. Plugin authors will have to do less TS juggling to get things to work.

PR Tasks

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Nov 15, 2022

Thanks for taking the time to open a PR!

@@ -1163,6 +1163,15 @@ declare namespace Cypress {
*/
end(): Chainable<null>

ensureElement(subject: any, commandName: string): void
Copy link
Contributor Author

Choose a reason for hiding this comment

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

any => void is correct here, even for ensureAttached, which really only makes sense with DOM elements: you can pass in any subject, and these functions either do nothing and return nothing, or they throw an error.

*
* @see https://on.cypress.io/custom-queries
*/
now(name: string, ...args: any[]): Promise<any> | ((subject: any) => any)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to be more specific, with name, args and the return type mapping to ChainableMethods, but typescript ended up caught in a recursion, since now is one of those ChainableMethods.

What we really need to do is go through and separate out "things on cy (like now, off, on, once)" from "commands that can be chained (like click, get, scrollTo)", but that would involve a larger refactor of the types than I want to undertake in this PR.

@@ -14,15 +14,15 @@ export default (Commands, Cypress, cy, state) => {
Commands.add('end', () => null)
Commands.add('noop', (arg) => arg)

Commands.addQuery('log', (msg, ...args) => {
Commands.add('log', (msg, ...args) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As per discussion in the docs PR, .log() should be a command and not a query.

Generally speaking, things that yield null don't make sense as queries, I just got overzealous and converted a few too many commands. :P

Copy link
Member

Choose a reason for hiding this comment

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

I wasn't even looking for this change here. 👍🏻

const validateType = (subject, type, cmd = state('current')) => {
const name = cmd.get('name')

const validateType = (subject, type, name) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed all of these to accept name, so that the signatures of the various "ensures" functions match with each other. Name is the only property we read off of command anyway. For a while I thought I needed the full command, but that ended up refactored away at some point, and just never got around to reverting the signatures.

The name / command arguments are new in Cy12, so we can be completely certain no one outside the monorepo is using them.

@@ -853,11 +853,11 @@ export default {
},
invalid_new_query: {
message: '`Cypress.Commands.addQuery()` is used to create new queries, but `{{name}}` is an existing Cypress command or query, or is reserved internally by Cypress.\n\n If you want to override an existing command or query, use `Cypress.Commands.overrideQuery()` instead.',
docsUrl: 'https://on.cypress.io/custom-commands',
docsUrl: 'https://on.cypress.io/custom-queries',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a new on link, for the new page I'm adding in the docs PR. I'll open a services PR to add it shortly.

@cypress
Copy link

cypress bot commented Nov 15, 2022



Test summary

21635 0 1716 0Flakiness 3


Run details

Project cypress
Status Passed
Commit c31e4a4
Started Nov 28, 2022 4:27 PM
Ended Nov 28, 2022 4:45 PM
Duration 17:31 💡
OS Linux Debian -
Browser Multiple

View run in Cypress Dashboard ➡️


Flakiness

specs_list_latest_runs.cy.ts Flakiness
1 App/Cloud Integration - Latest runs and Average duration > when no runs are recorded > shows placeholders for all visible specs
project-setup.cy.ts Flakiness
1 ... > can setup e2e testing for a project selecting JS
cypress-origin-communicator.cy.ts Flakiness
1 Cypress In Cypress Origin Communicator > cy.origin passivity with app interactions > passes upon test reload mid test execution

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@BlueWinds BlueWinds changed the title fix: Improve TypeScript support for custom queries feat: Add public TypeScript support for custom queries Nov 21, 2022
@@ -323,24 +323,27 @@ describe('src/cy/commands/actions/check', () => {
})

it('can set options.waitForAnimations', () => {
cy.stub(cy, 'ensureElementIsNotAnimating').throws(new Error('animating!'))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I moved ensureElementIsNotAnimating to $actionability, which is not public-facing, it's no longer trivial to stub. I've moved these tests to use an actual jQuery animation, and watch the count of command retries - this treats Cypress more as a 'black box', rather than peering inside at exactly how animation is determined.

@@ -3339,21 +3303,6 @@ describe('src/cy/commands/actions/click', () => {
})
})

// TODO: remove this after 4.0 when {multiple:true} is no longer default
Copy link
Contributor Author

@BlueWinds BlueWinds Nov 21, 2022

Choose a reason for hiding this comment

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

TODO from Cypress 4.0, referencing a long-closed issue. TODO done.

Basically, {multiple: true} used to be the default, so it was hidden in the console logs; in Cy >=4.0.0, this is no longer the case, so there's no default value to hide.

@@ -121,6 +120,14 @@ const setTopOnError = function (Cypress, cy: $Cy) {
top.__alreadySetErrorHandlers__ = true
}

const ensureRunnable = (cy, cmd) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ensureRunnable is only used in this one file, so I moved it here to be close to where it's used.

@@ -53,9 +54,127 @@ const getPositionFromArguments = function (positionOrX, y, options) {
return { options, position, x, y }
}

const ensureElDoesNotHaveCSS = ($el, cssProperty, cssValue, name: string, onFail) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Several methods from ensures.ts are only used here, and don't really make sense to expose for people to use in queries. I've moved them into this file.

@BlueWinds BlueWinds force-pushed the issue-7306-ts-improvements branch from 0d674d0 to 17fe50a Compare November 22, 2022 16:27
@BlueWinds BlueWinds changed the title feat: Add public TypeScript support for custom queries feat: Create public Cypress.ensure API for use with custom queries Nov 22, 2022
@BlueWinds BlueWinds marked this pull request as ready for review November 22, 2022 18:19
}

const isNotDisabled = (subject, name: string, onFail) => {
if (subject.prop('disabled')) {
Copy link
Member

Choose a reason for hiding this comment

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

with exposing these publicly, any risk someone will pass a non-jquery element to isNotDisabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm. Interesting thing to think about. The intended usage would be something like

Cypress.ensure.isElement(subject, 'myQuery')
Cypress.ensure.isNotDisabled(subject, 'myQuery')

If they don't ensure that it is an element first, and it's not an element, as it stands it would probably throw a pretty non-helpful error like "can't read property prop of null" or something.

If we added an implicit isElement() check to the top of each of these functions, that would provide more helpful error messages - but then we'd be running duplicate code for pretty much every cypress command.

I'm inclined to leave it as-is, but open to other thoughts.

Copy link
Member

Choose a reason for hiding this comment

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

maybe we be a bit more guarded in our check? if the original conditions are not met, ignore it?

if (subject.prop && _.isFunction(subject.prop) && subject.prop('disabled') {

Copy link
Member

Choose a reason for hiding this comment

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

though ignoring it sort of assumes it's true when it might be invalid? but oh well

@@ -375,7 +375,7 @@ export default (Commands, Cypress, cy, state) => {
})

return (subject) => {
cy.ensureSubjectByType(subject, 'element', this)
Cypress.ensure.isType(subject, 'element', 'shadow', cy)
Copy link
Member

Choose a reason for hiding this comment

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

honestly love how these read these updates! Nice job.

Co-authored-by: Emily Rohrbough <emilyrohrbough@users.noreply.github.com>
@chrisbreiding chrisbreiding self-requested a review November 28, 2022 19:41
Copy link
Contributor

@chrisbreiding chrisbreiding left a comment

Choose a reason for hiding this comment

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

In regards to documentation, will Cypress.ensure get its own page like Cypress.dom or just the mentions on the custom-queries page?

@BlueWinds
Copy link
Contributor Author

In regards to documentation, will Cypress.ensure get its own page like Cypress.dom or just the mentions on the custom-queries page?

I was planning to leave it with just the mentions on custom-queries for now. Adding a page documenting it more explicitly can be a post-release task.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants