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: addQuery Remaining Queries #24203

Merged
merged 34 commits into from
Oct 26, 2022
Merged

Conversation

BlueWinds
Copy link
Contributor

@BlueWinds BlueWinds commented Oct 11, 2022

User facing changelog

  • If cy.getCookie() fails to find a cookie, you can no longer traverse the non-existent cookie's properties (eg, with .its('value)). If you expect a cookie not to exist, assert that directly, eg: cy.getCookie('foo1').should('not.exist')
  • .invoke() no longer waits for promises to resolve. If you wish to await a promise, use cy.then() instead.

This PR updates the following commands to be queries:

.debug()
.document()
.end()
.focused()
.hash()
.its()
.invoke()
.location()
.log()
.pause()
.url()
.title()
.window()

All traversals are also updated (traversals are grouped together because they share code):

.children()
.closest()
.eq()
.filter()
.find()
.first()
.last()
.next()
.nextAll()
.nextUntil()
.not()
.parent()
.parents()
.parentsUntil()
.prev()
.prevAll()
.prevUntil()
.siblings()

Error messages when selectors fail, and when commands receive invalid subjects, have also been improved; see below for screenshots.

Additional details

Steps to test

it('passes', () => {
  cy.$$('body').prepend('<div id="foo"><button>foo</button></div>')

  setTimeout(() => {
    cy.$$('#foo').replaceWith('<span id="foo"><a class="active">bar</a></span>')
  }, 1000)

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

This test passes on this branch, and fails on develop or issue-7306.

How has the user experience changed?

Traversal failure

When queries initially succeed but later fail (because the DOM has updated), the error message now shows the whole query that led up to the failure, rather than just the element it failed on.

Old:

Timed out retrying after 500ms: Expected to find element: children, but never found it. Queried from element: <span#foo>

New:

Timed out retrying after 500ms: Expected to find element: children, but never found it. Queried from:

cy.get(body).find(#foo)

image

Subject validation

In addition to fixing some Detached DOM errors (but not all of them!), several error messages have been updated or added.

When a command receives an invalid subject, it now lists all valid subject types, rather than only the first plus the mysterious phrase "all X subject validations failed".

Old:

cy.scrollTo() failed because it requires a DOM element.
...
All 2 subject validations failed on this subject.

New:

cy.scrollTo() failed because it requires a DOM element or window.
...

image

Accessing properties

When using .its() or .invoke() to access nested properties, error messages now contain the full path the user requested. This more closely matches the error message to what the user wrote in their test.

Old:

Timed out retrying after 100ms: cy.invoke() errored because the property: bar returned a string value instead of a function. cy.invoke() can only be used on properties that return callable functions.

cy.invoke() waited for the specified property bar to return a function, but it never did.

New:

Timed out retrying after 100ms: cy.invoke() errored because the property: foo.bar returned a string value instead of a function. cy.invoke() can only be used on properties that return callable functions.

cy.invoke() waited for the specified property foo.bar to return a function, but it never did.

image

Failed assertions following traversals

When a failed assertion follows a traversal, we now use "subject" as a placeholder to indicate that we never actually reached the assertion.

Old:

[find] #wrapper
[contains] Newer
[assert] expected :cy-contains('Newer'), [type='submit'][value~='Newer'] to have class updated

Timed out retrying after 4000ms: Expected to find content: 'Newer' within the element: <div#wrapper> but never did.

New:

[find] #wrapper
[contains] Newer
[assert] expected subject to have class updated

Timed out retrying after 4000ms: Expected to find content: 'Newer' within the element: <div#wrapper> but never did.

AssertionError
Timed out retrying after 4000ms: Expected to find content: 'Newer' within the element: <div#wrapper> but never did.
image

PR Tasks

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Oct 11, 2022

Thanks for taking the time to open a PR!

@cypress
Copy link

cypress bot commented Oct 11, 2022



Test summary

4051 0 1094 0Flakiness 1


Run details

Project cypress
Status Passed
Commit 7cb9377
Started Oct 17, 2022 6:03 PM
Ended Oct 17, 2022 6:18 PM
Duration 15:30 💡
OS Linux Debian - 11.4
Browser WebKit 16

View run in Cypress Dashboard ➡️


Flakiness

cypress/e2e/commands/xhr.cy.js Flakiness
1 src/cy/commands/xhr > abort > aborts xhrs currently in flight

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 Issue 7306 add query traversal feat: addQuery Remaining Queries Oct 11, 2022
@@ -134,21 +134,6 @@ describe('src/cy/commands/assertions', () => {
cy.noop(obj).its('requestJSON').should('have.property', 'teamIds').should('deep.eq', [2])
})

// TODO: make cy.then retry
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 the intervening years since this TODO was left, we've refined and better defined exactly what retries and what doesn't.

.then() will never retry, and should not, so removing the commented out tests.

})

it('throws when first parameter is neither of type object nor of type string nor of type number', function (done) {
it('throws when we can\'t determine both a valid options and path', function (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.

.invoke is fundamentally too flexible. Consider the following invoke commands, all valid.

// Valid
.invoke('method', 'arg1')
.invoke({log: false}, 'method', 'arg1')

// Invalid
.invoke(false, 'method', 'arg1')
.invoke(false, 'arg1')

The invalid cases both have the signature [bool, string]. Is this an invalid options argument, or an invalid path argument? The function signature of .invoke() is too versatile / vague to know for sure.

The new implementation throws a slightly different error, because it's simpler / has fewer conditionals and switch statements than the original. I think it's just as valid of an error.

@@ -903,30 +902,19 @@ describe('src/cy/commands/connectors', () => {
Command: 'invoke',
Function: '.bar()',
Subject: this.obj,
'With Arguments': [],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

.invoke() now always includes With Arguments, even if it's an empty array. I strongly prefer consistency in my values over special cases (eg, omit when it's empty).

cy.noop(this.obj).invoke('math.sum', 1, 2, 3).then(function () {
expect(this.lastLog.invoke('consoleProps')).to.deep.eq({
Command: 'invoke',
Function: '.math.sum(1, 2, 3)',
'With Arguments': [1, 2, 3],
Subject: this.obj['math'],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Behavior change: .invoke() now always puts the actual subject as the Subject in its console logs. The subject was this.obj, the property retrieved is .math.sum. IMO this is more consistent and intuitive.

@@ -1058,7 +1047,7 @@ describe('src/cy/commands/connectors', () => {
cy.on('fail', (err) => {
const { lastLog } = this

expect(err.message).to.include('Timed out retrying after 100ms: `cy.invoke()` errored because the property: `baz` does not exist on your subject.')
expect(err.message).to.include('Timed out retrying after 100ms: `cy.invoke()` errored because the property: `foo.bar.baz.fizz` does not exist on your 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.

Behavior change: .invoke() now logs the full path when we tried to fetch it and failed. This makes it easier to debug and more consistent with the rest of Cypress' API design.

I think the fact it didn't used to work this way is more a quirk of implementation than any sort of thought-out-feature.

args: { cmd: name },
})
}
const log = this.get('_log') || (options.log !== false && Cypress.log({
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 one of the ways .invoke / .its are intertwined: .invoke() creates a log with a different message, then calls .its(). .its() then pulls out that log message and keeps it up to date.

Ugly.

return (subject) => {
cy.ensureSubjectByType(subject, ['optional', 'window', 'document', 'element'], this)
cy.ensureSubjectByType(subject, ['optional', 'element', 'window', 'document'], 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.

Moved 'element' to the front, so that the new error messages say "expects a DOM element, window or document" - putting the most common / likely value first.


const getWindow = () => {
const window = state('window')
Commands._addQuery('title', function title (options: Partial<Cypress.Loggable & Cypress.Timeoutable> = {}) {
Copy link
Contributor Author

@BlueWinds BlueWinds Oct 12, 2022

Choose a reason for hiding this comment

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

Nice example of how a query can have the same functionality, but be a lot shorter because we don't have to re-write the retry logic inside each one. Was very pleased with how the changes to this file panned out.

// and start the reconnection process
cy.wrap(Cypress)
// @ts-ignore
.invoke('automation', 'remote:debugger:protocol', {
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 test is a victim of the breaking change to .invoke(): It no longer accepts asynchronous functions (like Cypress.automation).

@@ -12,6 +12,8 @@ let server
// this is a transparent TCP proxy for Chrome's debugging port
// it can kill all existing connections or shut the port down independently of Chrome or Cypress
const startTcpProxy = () => {
console.error('starting tcp proxy ', { realPort, fakePort })
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just added an extra logging line that was helpful to me understanding how this system test worked.

@BlueWinds BlueWinds marked this pull request as ready for review October 12, 2022 20:26
Copy link
Contributor

@flotwig flotwig left a comment

Choose a reason for hiding this comment

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

The "behavior changes" you've mentioned make sense to me 👍

@@ -1659,19 +1569,19 @@ describe('src/cy/commands/connectors', () => {
cy.wrap(obj).its('foo.bar.baz.fizz')
});

[null, undefined].forEach((val) => {
[null/*, undefined*/].forEach((val) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is intentionally commented out, there should be a comment explaining why.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not intentional; Fixed in latest commit.

@@ -305,7 +305,7 @@ describe('Cookie Behavior with experimentalSessionAndOrigin=true', () => {
// firefox actually sets the cookie correctly
cy.getCookie('foo1').its('value').should('equal', 'bar1')
} else {
cy.getCookie('foo1').its('value').should('equal', null)
cy.getCookie('foo1').should('equal', null)
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 .its bug recorded anywhere? Even if there's no issue for it, I think it's worth mentioning in the changelog. We're probably not the only folks with tests relying on this quirk.

@@ -95,7 +95,7 @@ describe('src/cy/commands/querying', () => {
cy.get('body').focused().then(function () {
const { lastLog } = this

expect(lastLog.get('type')).to.eq('parent')
expect(lastLog.get('type')).not.to.eq('child')
Copy link
Contributor

Choose a reason for hiding this comment

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

In that case, do you think it'd be cleaner to change type to a boolean flag? log.isParent?

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 great! It's awesome all these commands will now be resilient to detached dom elements. I really like the error message improvements too.

Should this have an accompanying docs PR for the invoke changes?

@BlueWinds
Copy link
Contributor Author

Added a new test string change, and matching test: Failed assertions following traversals. See that section of the PR description for an image of the change.

Going through PR feedback now.

@BlueWinds
Copy link
Contributor Author

Looks great! It's awesome all these commands will now be resilient to detached dom elements. I really like the error message improvements too.

Should this have an accompanying docs PR for the invoke changes?

There definitely should be an accompanying docs PR; I've just been putting updating the docs off for a bit until we finish going through the product review phase for queries, so I can go through the docs page by page and update everything that needs updating all at once.

With that said, the invoke changes specifically are straightforward, so I'll start a detached DOM docs branch today.

@BlueWinds
Copy link
Contributor Author

Matching docs PR: cypress-io/cypress-documentation#4785

@BlueWinds BlueWinds requested review from flotwig and removed request for lmiller1990 October 17, 2022 19:50
@@ -528,9 +528,9 @@ export default function (Commands, Cypress, cy, state, config) {
// we are not limited to "within" subject
// https://github.com/cypress-io/cypress/issues/14253
if (userOptions.capture !== 'runner') {
const withinSubject = state('withinSubject')
const withinSubject = cy.getSubjectFromChain(cy.state('withinSubjectChain') || [])
Copy link
Member

Choose a reason for hiding this comment

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

I'd be nice if getSubjectFromChain would handle checking for undefined.

Copy link
Contributor Author

@BlueWinds BlueWinds Oct 24, 2022

Choose a reason for hiding this comment

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

Yeah, I've been thinking the same thing.

I'm going to do it in the next PR rather than here - I already have some changes queued to the subject API, so rolling it in there makes sense.

@BlueWinds BlueWinds merged commit a052842 into issue-7306 Oct 26, 2022
@BlueWinds BlueWinds deleted the issue-7306-addQuery-traversal branch October 26, 2022 17:53
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