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

feat: move .contains() and .shadow() to be queries; remove cy.ng() #23791

Merged
merged 18 commits into from
Oct 11, 2022

Conversation

BlueWinds
Copy link
Contributor

@BlueWinds BlueWinds commented Sep 12, 2022

User facing changelog

cy.ng() was an outdated, undocumented Cypress command; it has been removed.

.contains() and .shadow() have been heavily refactored, as part of the ongoing effort to resolve #7306. .contains() in particular has improved performance.

Additional details

Queries-that-use-other-queries and command-that-use-queries are now supported to a much improved extent; see the new tests in cy.cy.ts for some minimal examples, or the new version of .contains() which relies on .get() for a more practical use.

Contains() and Shadow() are now queries, using the new _addQuery API introduced in #23665. This means they append to the subject chain, rather than replace it.

Removing .ng() happened as part of this effort because it also relied on the old command-based version of .get() - rather than update it alongside the other commands, we decided to remove it instead. See https://cypressio.slack.com/archives/C01TQQCQLJY/p1663015490536119 for internal discussion around the subject.

As part of rewriting .contains(), I had to go into dom/elements/find.ts. The existing implementation involved overwriting jquery functionality, and creating and throwing away a selector function every time .contains() retried; The new version properly registers :cy-contains() with jquery, with no need to override the built-in :contains(). This should result in less memory-churn when retrying .contains() / .should('contain.text').

Steps to test

This PR brings us to the first example of a fixed detached DOM error:

it.only('test', () => {
  cy.$$('body').append('<button id="btn">foobar</button>')

  setTimeout(() => {
    cy.$$('#btn').remove()
    cy.$$('body').append('<button id="btn" class="active">foobaz</button>')
  }, 1000)

  cy.get('#btn').contains('foo').should('have.class', 'active')
})

In develop, this results in a Detached DOM error - .get() resolves to foobar, but then the timeout removes it and adds foobaz (with the same ID). .contains() then retries forever with foobar as its subject, eventually failing.

But in this branch, .contains() now uses the logic in retryQuery in command_queue.ts, which properly re-fetches the subject each attempt - no Detached DOM error!

Future PRs will expand on this, moving more commands into queries, and also updating commands so that they can receive the same benefits.

How has the user experience changed?

PR Tasks

  • Have tests been added/updated?
  • [n/a] Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
  • Has a PR for user-facing changes been opened in cypress-documentation?
  • Have API changes been updated in the type definitions?

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Sep 12, 2022

Thanks for taking the time to open a PR!

@BlueWinds BlueWinds changed the title Issue 7306 add query custom commands feat: move .contains() and .shadow() to be queries; remove cy.ng() Sep 27, 2022
Base automatically changed from issue-7306-addSelector-redux to develop September 28, 2022 16:01
@BlueWinds BlueWinds changed the base branch from develop to issue-7306 September 29, 2022 16:40
@cypress
Copy link

cypress bot commented Sep 29, 2022



Test summary

4056 0 349 0Flakiness 0


Run details

Project cypress
Status Passed
Commit 0eecaed
Started Oct 5, 2022 7:32 PM
Ended Oct 5, 2022 7:44 PM
Duration 12:09 💡
OS Linux Debian - 11.4
Browser Electron 106

View run in Cypress Dashboard ➡️


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

@@ -1293,6 +1291,7 @@ space

it('is case sensitive when matchCase is undefined', () => {
cy.get('#test-button').contains('Test')
cy.contains('test').should('not.exist')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding this assertion because I had problems show up in Launchpad component tests due to incorrect handling of case in .contains(). I thought I had the casing correct because we had tests covering it, but turns out they only asserted the positive case and not the negative.

packages/driver/src/cy/commands/index.ts Show resolved Hide resolved
@@ -298,7 +298,7 @@ export default {
docsUrl: 'https://on.cypress.io/contains',
},
length_option: {
message: `${cmd('contains')} cannot be passed a \`length\` option because it will only ever return 1 element.`,
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 error message was incorrect - you can in fact pass a length option. It just has to be 1. 🤷‍♀️ Not a change in .contains()s behavior - it's always been this way.

export default (Commands, Cypress, cy, state) => {
/*
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this PR, I've resolved the temporary and confusing state of having two implementations of .get(). It was ugly, but now it's gone!

matchCase?: boolean
} = {}) => {
const $expr = $.expr[':']
/*
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 hadn't intended to do anything here in this PR / initiative, but I kept running into bugs where the old logic would break with multiple .contains() statements + aliases in the same test. This was happening because I was only calling getContainsSelector once, rather than each time a retry occurred, and not cleaning it up properly; the method I thought was just returning a string was also modifying the global jquery Object!

The old getContainsSelector had side effects, making it hard to use safely. The new one is purely functional - it takes arguments and returns a string. It also performs better, since we register these three handlers once, and allow jquery to compile / cache them, rather than adding the :cy-contains() expression with internal state on each retry loop.

Copy link
Contributor

Choose a reason for hiding this comment

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

just returning a string was also modifying the global jquery Object!

😢

Glad to see we are moving away from mutating things (especially using functions that you wouldn't expect to mutate something).

@BlueWinds BlueWinds marked this pull request as ready for review September 29, 2022 22:59
expect(err.docsUrl).to.eq('https://on.cypress.io/contains')

done()
})

cy.contains('Nested Find').should('have.length', 2)
})

it('restores contains even when cy.get fails', (done) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The "restores contains" tests are no longer relevant, because we're not overriding jQuery's builtin :contains in the first place. Nothing to clean up! :)

@lmiller1990
Copy link
Contributor

Quick smoke test before review. It works as expected.

Cypress 10.9

image

issue-7306-addQuery-custom-commands

image

Copy link
Contributor

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

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

Few questions, but looks good - this was an easy one to review. Refreshing to see a PR with more deletions than additions... I feel like the a lot of technical debt is starting to get addressed.

packages/driver/src/cy/commands/index.ts Show resolved Hide resolved
matchCase?: boolean
} = {}) => {
const $expr = $.expr[':']
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

just returning a string was also modifying the global jquery Object!

😢

Glad to see we are moving away from mutating things (especially using functions that you wouldn't expect to mutate something).

}

// we set the `cy-contains` jquery selector which will only be used
// in the context of cy.contains(...) command and selector playground.
$expr['cy-contains'] = cyContainsSelector
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the mutation you were referring to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. In develop this is overridden each time .contains() retries, with cyContainsSelector holding internal state (rather than letting jquery parse the args and pass it in).

@@ -4,7 +4,7 @@ export default (Commands, Cypress, cy, state) => {
timeout: options.timeout,
})

cy.state('current').set('timeout', options.timeout)
this.set('timeout', options.timeout)
Copy link
Contributor

@lmiller1990 lmiller1990 Sep 30, 2022

Choose a reason for hiding this comment

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

Just to clarify, are there equivalent? My understanding is cy.state('current') is just the current Cypress instance. this.set points to the same thing - an instance of the class defined in driver/src/cypress.ts?

Copy link
Contributor

Choose a reason for hiding this comment

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

cy.state('current') is the currently running command, not the Cypress instance. Inside queries, looks like this is applied as a command created to wrap the query function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. This is a DX decision I made around queries - they have direct access to their own $Command instance (via this), and properties on the command instance are where you store any information that needs to 'make it out' of the query closure (such as timeout).


function isSubmit (elem: Element): elem is HTMLInputElement {
return elem.tagName === 'INPUT' && (elem as HTMLInputElement).type === 'submit'
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we have special logic for isSubmit?

Also won't any button in a form trigger a submit? eg

<form>
  <button>submit</button>
</form>

In this case the button would behave like a submit.

It is only used once:

// taken from jquery's normal contains method
  return function (elem) {
    if (isSubmit(elem)) {
      return regex.test(elem.value)
    }

It's not really clear why we do this here, do you happen to know why? Not sure you needed to dive into that logic while you were here, but I'm curious either way 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/submit

You can have <input type="submit" value="Text displayed to user" /> - and the value property shows up on the screen. So .contains() wants to look at it as if it were text content on the page.

@@ -776,8 +776,6 @@ describe('src/cy/commands/querying', () => {
cy.get('#missing-el').should('have.prop', 'foo')
})

it('throws when using an alias that does not exist')
Copy link
Member

Choose a reason for hiding this comment

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

Should there be a test case added for this instead of deleted this placeholder test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good thought. Was writing an assertion to cover this when it occurred to me to check on the error being thrown - dug through and found that this case is already covered here: https://github.com/cypress-io/cypress/blob/issue-7306-addQuery-custom-commands/packages/driver/cypress/e2e/commands/aliasing.cy.js#L485

It's a bit confusing that alias tests are split between aliasing.cy.js and querying.cy.js, but I don't think I want to move them as part of this PR. Another bit of cleanup to be done later.

@@ -919,6 +917,12 @@ describe('src/cy/commands/querying', () => {
})

context('#contains', () => {
it('should keep multiple contains() separate', () => {
Copy link
Member

Choose a reason for hiding this comment

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

What does this test mean?

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've changed the name of this test to does not interfere with other aliased .contains() and moved it further down in the file, but not sure how to succinctly capture what it's guarding against in the test title.

Added a longer explanation as a code comment; let me know if that's still not clear and I'll see what I can do.

@@ -896,6 +899,10 @@ export class $Cy extends EventEmitter2 implements ITimeouts, IStability, IAssert
}

now (name, ...args) {
if (this.queryFns[name]) {
return this.queryFns[name].apply(this.state('current'), args)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the first argument be this so the context is shared for query commands? And the second subject would be this.state('current')?

Copy link
Member

@emilyrohrbough emilyrohrbough left a comment

Choose a reason for hiding this comment

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

I have only gotten partially through the review, but look like the changes to .shadow() is unintentionally introducing new behaviors to the command.

const consoleProps: Record<string, any> = {
'Applied To': $dom.getElements(subject),
}
Commands._addQuery('shadow', function contains (userOptions: ShadowOptions = {}) {
Copy link
Member

Choose a reason for hiding this comment

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

The way _addQuery is currently implemented, this changes .shadow from being a child command which requires an element subject to a dual command which an optional previous subject. This is new behavior that I don't anticipate is intentinal.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like this is handled below by cy.ensureSubjectByType(subject, 'element') instead of being part of the _addQuery functionality. Is that right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aye. _addQuery does not have builtin functionality to assert on how queries can be used - each query that wants to has to explicitly run its own ensure()s to validate its position and incoming subject.

The reasons for this are a bit multifaceted, but it boils down to "parent / child / dual don't map cleanly to how Cypress implements them." You can execute parent commands after child commands, as long as the parent command returns null. And for commands we already have code to derive parent/child/dual from the prevSubject anyway. prevSubject is the "natural" concept, and parent/child/dual is a computed property.

So for Queries, I skipped that whole set of logic. The goal is to make it clearer when and how this code executes, to someone reading or writing queries. Each query states, explicitly with an ensures statement (rather than via an argument to _addQuery), exactly what subjects are allowed.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't it default to parent and none and the query commands that change this behavior override this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It defaults to dual/optional because I want to bypass the existing logic entirely. This goes pretty deep into the weeds of how prevSubject is implemented.

prevSubject is checked before invoking the command / query. But for queries, we instead want to throw an error inside the retry loop - the subject might (is expected to!) change between retries, and we want to run the validation each time.

So we use 'optional' to signal that Cypress shouldn't try running any assertions before invoking the command - any incoming subject is acceptable. The query will validate the incoming subject later, in the inner queryFn.

Copy link
Member

@emilyrohrbough emilyrohrbough Oct 3, 2022

Choose a reason for hiding this comment

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

Can you link through where this is? I'd like to understand where this bypasses..I have been pretty deep in this code recently, and my understanding of addCommand, is that we are actually wrapping the commands callback (which we aren't doing with _addQuery()) which enforces the previous subject check for us.

Copy link
Contributor Author

@BlueWinds BlueWinds Oct 3, 2022

Choose a reason for hiding this comment

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

Ok, went through the code and looks like my explanation is out of date - what I said is why I originally had optional / dual, but it no longer applies. Thank you for pressing on the matter!

optional and dual aren't needed at all, so I've removed them. Queries just plain old don't interact with the concept of prevSubject / type anymore - they follow their own codepath which bypasses them entirely, and my above explanation was badly out of date. Just had to tweak .contains() slightly to make sure it properly showed up as a child log message.

},
})
if (!subject || (!$dom.isElement(subject) && !$elements.isShadowRoot(subject[0]))) {
subject = cy.$$('body')
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this overwriting of subject is what's causing this test failure (this assertion in particular).

I guess I could go either way on whether the test should be updated or the Applied To should be changed back to undefined if there's no subject.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pinpointing a failure!

I'm leaning towards updating the assertion - I think it makes sense to highlight the body tag we queried against. Especially looking ahead a bit towards iframe support, when a command's context could be just part of the page, "this is the section of the DOM we looked at" could possibly be useful.

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.

This looks really good to me. Just some test failures that need to be addressed.

@@ -1550,19 +1550,13 @@ space
})
})

it('sets type to parent when subject isnt element', () => {
Copy link
Contributor Author

@BlueWinds BlueWinds Oct 3, 2022

Choose a reason for hiding this comment

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

This is a behavioral change: .contains() now logs as a child whenever it's used as a child command. Previously, it logged as a child if used as a child command unless the previous subject was body/window.

This exception is not a useful one. Simple, easy to understand rules are preferable. If the previous command returns a subject (not null), .contains() now shows as a child of that command.

@chrisbreiding
Copy link
Contributor

I guess this PR can close this issue since cy.ng() won't be around anymore.

@BlueWinds
Copy link
Contributor Author

Ok, took longer than I'd hoped but have all tests passing (except one which I commented out with a TODO to re-enable as part of a follow-up - since this is into a feature branch, I'd like to put that off for now and start work related to .within() in a fresh PR).

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.

Looks like there are a lot of unrelated commits and changes in the diff. Might be caused by merging develop directly into this branch instead of merging it into the issue-7306 branch and then merging that branch into this one.

As it is, it's difficult to review the changes.

@BlueWinds BlueWinds force-pushed the issue-7306-addQuery-custom-commands branch from b9373ee to 0eecaed Compare October 5, 2022 15:48
@BlueWinds
Copy link
Contributor Author

@chrisbreiding - Merged develop into issue-7306, then rebased this PR off that branch. I'll be more careful about that in the future; I thought that if I merged develop into both branches, it would work itself out, but apparently not!

@BlueWinds BlueWinds merged commit 2326f96 into issue-7306 Oct 11, 2022
@BlueWinds BlueWinds deleted the issue-7306-addQuery-custom-commands branch October 11, 2022 14:49
mjhenkes pushed a commit that referenced this pull request Nov 14, 2022
* feat: Commands.addSelector, and migrate .get() to be a selector

* Fix for failed tests

* Last test fix

* More test fixes

* Self review changes

* Remove the concept of prevSubject from selectors entirely

* Rename addSelector to addQuery

* Quick fix for last commit

* Fix TS

* Fix merge from develop

* Add types and other review updates

* Increase timeout to try fixing flakiness

* Rename addQuery to _addQuery

* Fix typo in previous commit

* Fix TS

* Include AUT assertion in cy.get()

* Fix for previous commit

* Review feedback

* Minor test improvement

* Swifter failure on sizzle syntax error

* Better solution for refetching current subject in verifyUpcomingAssertions

* Command IDs now include their chainerId

* Revert "chore: Revert "feat: _addQuery() (#23665)" (#24022)"

This reverts commit f399994.

* feat: move .contains() and .shadow() to be queries; remove cy.ng() (#23791)

* First stab at removing old .get() implementation

* Fix TS and a couple of tests

* Fix tests and TS

* Fix case-sensitivity for .contains()

* Stop TS complaining

* Rework cy-contains jquery expression

* Add comments, make ts happy

* Fix one test, review feedback

* Review updates

* Fix additional tests

* Fix accidental deletion of vital code

* One more try at getting logs right

* Fix race condition in cross-origin .contains

* Add commented out test to ensure .within() works properly with selectors

* Fix for sessions + query subject chaining

* Fix mixing .within() shadow DOM and .contains() in same chainer

* One more attempt at .within + .contains

* Fix rebase commits

* feat: addQuery Remaining Queries (#24203)

* First stab at removing old .get() implementation

* Fix TS and a couple of tests

* Fix tests and TS

* Fix case-sensitivity for .contains()

* Stop TS complaining

* Rework cy-contains jquery expression

* Add comments, make ts happy

* Fix one test, review feedback

* Review updates

* Fix additional tests

* Fix accidental deletion of vital code

* One more try at getting logs right

* Fix race condition in cross-origin .contains

* Add commented out test to ensure .within() works properly with selectors

* Fix for sessions + query subject chaining

* Fix mixing .within() shadow DOM and .contains() in same chainer

* One more attempt at .within + .contains

* Fix rebase commits

* Update many commands to be queries; improve log message around invalid subjects

* Update connectors, location, focused and window commands to queries

* Return noop to a command and not a query (to avoid implicit assertions)

* More test fixes

* Fix test failures

* Fix for weird-ass frontend-component test

* Error message improvements

* Fix for broken system test

* Update withinSubject to use subject chain

* Test clarifications

* Unbreak cypress-testing-library via withinState backwards compatibility

* Typo in last commit

* Improvement for assertion following failed traversal

* feat: Fix detached DOM errors for all Cypress commands (#24417)

* First stab at removing old .get() implementation

* Fix TS and a couple of tests

* Fix tests and TS

* Fix case-sensitivity for .contains()

* Stop TS complaining

* Rework cy-contains jquery expression

* Add comments, make ts happy

* Fix one test, review feedback

* Review updates

* Fix additional tests

* Fix accidental deletion of vital code

* One more try at getting logs right

* Fix race condition in cross-origin .contains

* Add commented out test to ensure .within() works properly with selectors

* Fix for sessions + query subject chaining

* Fix mixing .within() shadow DOM and .contains() in same chainer

* One more attempt at .within + .contains

* Fix rebase commits

* Update many commands to be queries; improve log message around invalid subjects

* Update connectors, location, focused and window commands to queries

* Return noop to a command and not a query (to avoid implicit assertions)

* More test fixes

* Fix test failures

* Fix for weird-ass frontend-component test

* Error message improvements

* Fix for broken system test

* Update withinSubject to use subject chain

* Test clarifications

* Unbreak cypress-testing-library via withinState backwards compatibility

* Typo in last commit

* Improvement for assertion following failed traversal

* WIP adding query support to

* More work on actionability + detached dom

* Fix TS, rename _addQuery to addQuery

* Another try to fix types

* Fix lint

* Fix for bad merge

* Fixes for a couple more tests

* Increase timeout 50ms -> 100ms on certain tests failing in CI

* Switch to new branch of cypress-testing-library

* Update lockfile

* Fix yarn.lock with latest version of forked testing-library

* More test fixes

* Fix TS again

* Increase test assertion timeout so it passes on slow browsers (webkit)

* Apply suggestions from code review

Co-authored-by: Emily Rohrbough <emilyrohrbough@users.noreply.github.com>
Co-authored-by: Zach Bloomquist <git@chary.us>

* More review changes

* Fix selectFile tests based on updated error message

* Improve types and type comments for Commands.add

* Undo change to Commands.add types

* Update yarn lockfiles again

* Remove overwriteQuery from Cy12; .focused() now respects passed in timeout

* Update cli/types/cypress.d.ts

Co-authored-by: Chris Breiding <chrisbreiding@users.noreply.github.com>

* Restore .uncheck() tests

Co-authored-by: Emily Rohrbough <emilyrohrbough@users.noreply.github.com>
Co-authored-by: Zach Bloomquist <git@chary.us>
Co-authored-by: Chris Breiding <chrisbreiding@users.noreply.github.com>

* Fix for hanging driver test after merge

* Fix for app component test

Co-authored-by: Emily Rohrbough <emilyrohrbough@users.noreply.github.com>
Co-authored-by: Zach Bloomquist <git@chary.us>
Co-authored-by: Chris Breiding <chrisbreiding@users.noreply.github.com>
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.

4 participants