feat: add more helpful debugging information to queries #108

NicholasBoll · 2020-01-30T18:31:48Z

Why:

I'm a big fan of the React Testing Library and the concept behind it. I wanted those concepts to come to @testing-library/cypress as well, but the commands didn't do what I expected. It wasn't even until recently that #100 allowed a previous subject to be used! These commands are just too useful, but just lacked the last mile of finesse. Most of these issues are outlined in #103

This PR closes some of the UX/DX gaps that built-in Cypress commands have.

My original intent was to break features up into separate PRs, but the features required changes to the same lines of code and could not be done independently. If it is a big concern, features can be introduced in separate PRs serially due to the interdependency of the same lines of code.

What:

Add element selector information for debugging (outlines element when you click on command) (fixes Unhelpful error messages for query* commands #103)
Screenshot shows the element outline highlighted in the application view. Also showing which element is yielded, like other Cypress commands that return elements.
Add @testing-library/dom errors (from get* queries) to failure messages - these are more helpful than the generic find*('input') does not exist messages (fixes Unhelpful error messages for query* commands #103)
Add retryability to findBy* when multiple elements are found (fixes findBy* retry-ability when multiple elements found #83)
Add option to disable logging of all commands
query* and find* have a consistent code path and error messaging (fixes Unhelpful error messages for query* commands #103)
Remove usage of Cypress commands in queries (fixes Unhelpful error messages for query* commands #103)
Add ability to use a previous subject in a more idiomatic way without introducing breaking changes caused by Support running commands against the previous yielded subject #100 (fixes TypeError: container.querySelectorAll is not a function #109 and Support running commands against the previous yielded subject #110)
Add parent/child log type detection for more accurate Cypress command logging ("Parent" aligns left while "child" is indented and preceded by a - like in the screenshot)

How:

I tried to make test cases for all changes. They should all fail without these changes. I'm happy to add more if I've missed any. I've tested changes against a local branch of https://github.com/Workday/canvas-kit using @testing-library/cypress and things are working.

Breaking Changes?:

query* will now fail if no element was found. Before the command would pass an empty object which could cause the next command to fail (e.g. .click()). If a query* failed to return an element, a .should('exist') would not fail because the assertion was given a non-null value. With this change the test should pass/fail as expected, but not as previously implemented.

Checklist:

Documentation
Tests
Ready to be merged

I don't expect this PR to be merged right away and I expect some conversation.

Some open questions:

Should get* queries be allowed instead of throwing errors? This library is now using @testing-library/dom get* APIs for error messages. If that's the case, get*, query*, and find* all would use the same underlying implementation.
Does the difference between *By* and *AllBy* make sense in the context of Cypress? Cypress DOM APIs return a jQuery NodeList with one or more elements. Most of the time this is completely fine and expected, but certain commands expect only one element. For example, .click will throw an error if handed multiple elements. .first() can easily be used to return only the first one or the previous query can be updated to return only one. If a user expects a certain number (including only one), the command can be followed by .should('have.length', 1) if the test is meant to ensure only a single element exists in the DOM and fail otherwise - so single vs multiple is opt-in for other elements.

* Add element selector information for debugging (outlines element when you click on command) (fixes testing-library#103) * Add @testing-library/dom errors (from `get*` queries) to failure messages - these are more helpful than the generic `find*('input') does not exist` messages (fixes testing-library#103) * Add retryability to `findBy*` when multiple elements are found (fixes testing-library#83) * Add option to disable logging of all commands * `query*` and `find*` have a consistent code path and error messaging (fixes testing-library#103) * Remove usage of Cypress commands in queries (fixes testing-library#103)

NicholasBoll · 2020-01-30T18:34:27Z

README.md

+built-in retryability using error messages from `get*` APIs to forward as error
+messages if a query fails. `query*` also uses `get*` APIs, but disables retryability.
+
+`findBy*` is less useful in Cypress compared to `findAllBy*`. If you intend to limit


Question:

Does the difference between *By* and *AllBy* make sense in the context of Cypress? Cypress DOM APIs return a jQuery NodeList with one or more elements. Most of the time this is completely fine and expected, but certain commands expect only one element. For example, .click will throw an error if handed multiple elements. .first() can easily be used to return only the first one or the previous query can be updated to return only one. If a user expects a certain number (including only one), the command can be followed by .should('have.length', 1) if the test is meant to ensure only a single element exists in the DOM and fail otherwise - so single vs multiple is opt-in for other elements.

I'm not sure why I would use findAllBy* when I expect there to only be one in the first place. Is the appeal just that you can use findAllBy* 100% of the time and not worry about how many elements are returned?

I like that findBy* communicates (through code) the intent of the test writer (that there should only be one) without having to make an explicit assertion for that.

I think the question is around idiomatic Cypress code vs idiomatic @testing-library/* code.

All Cypress commands that return elements (actually jQuery-wrapped NodeLists) return 1 or more elements or fail. The only exception to this is if the command is followed by as .should('not.exist'). This is because every command that uses cy.verifyUpcomingAssertions implicitly adds .should('exist') if no assertions are found. jQuery doesn't fail if it can't find an element by a selector, it just returns an empty node list. Cypress ensures there is at least one element. This combined with the implicit retry mechanism ensures you just tell Cypress what you want and it will retry in the background regardless of application timing. This is what makes Cypress so nice to work with.

Most of the time this is exactly what you expect to happen. It avoids a larger API footprint of ensuring only 1 element. Most of the time there is no issue.

For example:

cy.contains('button', 'Submit').click()

cy.contains can return more than 1 element. By default, cy.click needs one element and will throw an error saying it expected only one element. If you get an error, you can choose to narrow down the selector. I think Cypress did this so that developers wouldn't be forced to choose between wanting one selector and wanting many - also developers don't have to choose to always do single queries vs many queries and switch when there is a specific case. The Cypress API and errors guide the user into a single API with optional narrowing only when needed.

I'm perfectly happy to remove any documentation suggesting single vs many queries. I just wanted to pose a question - should the API reflect something Cypress users would most likely expect or something non-Cypress users would most expect?

The thing I personally find most useful about this library is helpers for common things I need to do. Like selecting an input element by its label.

Perhaps other people value the API to be more strictly adhering to the @testing-library/dom API. We've already determined it doesn't make sense in the Cypress context to do that exactly (the get* queries are already disabled for example)

Cypress does have a mechanism for asserting there should be only one element if necessary:

cy.get('some-selector').should('have.length', 1)

Sometimes the intent is to make sure there is only one

I think I'll make the language more positive.

But I do think we should keep findBy* around for the foreseeable future

Do you mean the current functionality that findBy* throws an error if more than one 1 element is found?

Do you mean the current functionality that findBy* throws an error if more than one 1 element is found?

Yes

I think the question is around idiomatic Cypress code vs idiomatic @testing-library/* code.

IMHO idiomatic @testing-library/* code not only provides useful common helpers (generally for selecting elements) but also encourages best practices. So, it may deviate from idiomatic code in the library / framework it augments because that library doesn't support/encourage best practice.

Consider the example:

cy.findAllBy*(...).first().click() (More common to Cypress)

cy.findBy*(...).click() (testing library recommended).

I would always prefer use the second version, because when my code changes so that selector matches a second element, I expect a test failure on selecting an element, not clicking the incorrect element.

It's possible to avoid this case using:
cy.findAllBy*('some-selector').should('have.length', 1)
But doing that after every findAllBy* is tedious, and likely to eventually be forgotten / missed - so in this case I think findBy* encourages better testing practices than findAllBy* and I don't think @testing-library/* should shy away from recommending different, better practices than Cypress or any other library it integrates with.

Is the update to the readme okay? https://github.com/testing-library/cypress-testing-library/#differences-from-dom-testing-library

The update to the readme definitely documents this change 👍

I'm not sure I agree it should be the recommendation of @testing-library/* 👐

README.md

NicholasBoll · 2020-01-30T18:35:47Z

README.md

 })
+cy.get('form').findAllByText('Button Text').should('exist')


I felt this example was missing. It was one of the reasons I overlooked this library and the I found out #100 was merged just recently. This is more idiomatic for Cypress.

NicholasBoll · 2020-01-30T18:37:48Z

README.md

-cy.queryByText('Button Text').should('exist')
-cy.queryByText('Non-existing Button Text').should('not.exist')
-cy.queryByLabelText('Label text', {timeout: 7000}).should('exist')
+cy.queryAllByText('Button Text').should('exist')


I'm not sure I have a good reason for changing these. It is part of the questions of the pull request. I think *By and *AllBy make sense for @test-library/dom, @testing-library/react, and company, but I'm not sure it does for Cypress.

Personally, I recommend always using the *ByAll since that's what native Cypress commands do now, but I'm welcoming other opinions

NicholasBoll · 2020-01-30T18:38:27Z

cypress/integration/find.spec.js

@@ -1,3 +1,5 @@
+/* eslint-disable max-lines-per-function */


I'm not sure this rule even makes sense in the context of a test file. Is the intent to limit how many tests there are in a file?

Yeah, I don't use describe blocks much in my regular tests, but in Cypress it's kinda necessary. We can add an .eslintrc to the cypress/ directory and disable it in there. Should probably disable the jest promise one too:

{ "rules": { "max-lines-per-function": "off", "jest/valid-expect-in-promise": "off" } }

That worked, thank you

NicholasBoll · 2020-01-30T18:40:12Z

cypress/integration/find.spec.js

+      expect(err.message).to.contain(errorMessage)
+    })
+
+    cy.findByText(regex, {timeout: 100}) // Every find query is implicitly a `.should('exist')


I think we should explicitly use .should('exist')

cy.verifyUpcomingAssertions has a default of existence if no other assertion is found. That's why it isn't explicitly needed. I like to err on the side of being more explicit, but I left it as is from previous code. I'm happy to change it if others agree

I agree with you.

In the jest world, I recommend people wrap get* queries with assertions of existence even though they throw:

expect(getByText(/success/i)).toBeInTheDocument()

NicholasBoll · 2020-01-30T19:08:22Z

src/index.js

        )
        return queryImpl(container, ...args)
      }
      const commandImpl = doc => baseCommandImpl(doc)

      const inputArr = args.filter(filterInputs)

+      const getSelector = () => `${queryName}(${queryArgument(args)})`
+
+      const win = cy.state('window')


This line removes the need for:

cy.window() .then((thenArgs) => {

kentcdodds · 2020-01-30T19:09:34Z

Thanks for this! I'll review it as soon as I'm able. I don't think we can move forward on this until #109 is dealt with first however. When you get a chance, could you weigh in there?

NicholasBoll · 2020-01-30T19:09:58Z

src/index.js

+        Selector: getSelector(),
+        'Applied To': getContainer(


These lines are found on other Cypress DOM traversal commands:

https://github.com/cypress-io/cypress/blob/13ebb9779d21238cae4da4b63cf6230da4a5341d/packages/driver/src/cy/commands/traversals.coffee#L26-L27

NicholasBoll · 2020-01-30T19:11:00Z

src/index.js

+        if (result.length === 1) {
+          consoleProps.yielded = result.toArray()[0];
+        } else if (result.length > 0) {
+          consoleProps.yielded = result.toArray();
+        }


This is how other Cypress commands show Yielded

https://github.com/cypress-io/cypress/blob/13ebb9779d21238cae4da4b63cf6230da4a5341d/packages/driver/src/cy/commands/traversals.coffee#L38

NicholasBoll · 2020-01-30T19:12:03Z

src/index.js

+        if (result.length > 1 && !/all/i.test(queryName)) {
+          // Is this useful?
+          throw Error(`Found multiple elements with the text: ${queryArgument(args)}`)
+        }


This line is part of the question asking if we should throw errors for the *By queries. It is what the other @testing-library libraries do

Let's change it to case-sensitive (just in case): !/All/.test(queryName).

I think that we should be consistent with the other libraries 👍

NicholasBoll · 2020-01-30T19:14:08Z

src/index.js

+      if (queryRegex.test(queryName)) {
+        // make the timeout extremely short to ensure `query*` commands pass or fail instantly
+        options.timeout = 0
+      }


Question:

Should we do this? The docs mention query is synchronous. Setting a timeout of 0 is the closest Cypress can get to being synchronous while still operating like Cypress does.

Previously the code didn't do a cy.verifyUpcomingAssertions with a retry loop so query* had an inconsistent experience and assertions like .should('not.exist') didn't work

Yes, I think we should do this, though I think that we should not override options.timeout if they're specified.

Also, I'm thinking that in a future breaking change we can remove query* and only support find*.

I agree with removing query* in the future. The functionality can be done using a low timeout. Most of the time, this is not what users want, but they can accomplish the same thing with built-in Cypress commands:

cy.findByText('My Button', { timeout: 0 }) // won't retry because the timeout is 0, but it is not asynchronous cy.contains('My Button', { timeout: 0 }) // consistent functionality to the above line in terms of timing

I'll remove the timeout override. For query* it will default to 0, but allow user override.

NicholasBoll · 2020-01-30T19:26:05Z

src/index.js

+        const result = Cypress.$()
+        result.selector = getSelector()
+        return result
+      }


This is a clever hack for both useful errors and tie into the Cypress assertion system. Cypress DOM assertions expect a jQuery NodeList and have special handling for .should('exist') and others. Even if @testing-library/dom throws an error, we should still return an empty jQuery NodeList so that failures in this library are consistent with other Cypress DOM-based commands.

src/index.js

…ibrary into pr/add-helpful-debugging-info # Conflicts: # src/index.js

* Fixes testing-library#109 without breaking change caused by testing-library#100

cypress/integration/find.spec.js

NicholasBoll · 2020-01-31T05:47:07Z

cypress/integration/find.spec.js

+  it('findByText should not break existing code', () => {
+    cy.window()
+      .findByText('Button Text 1')
+      .should('exist')
+  })


This test verifies non-breaking changes for #110.

NicholasBoll · 2020-01-31T05:48:06Z

cypress/integration/find.spec.js

+  it('findByText should show as a parent command if it starts a chain', () => {
+    const assertLog = (attrs, log) => {
+      if(log.get('name') === 'findByText') {
+        expect(log.get('type')).to.equal('parent')
+        cy.off('log:added', assertLog)
+      }
+    }
+    cy.on('log:added', assertLog)
+    cy.findByText('Button Text 1')
+  })
+
+  it('findByText should show as a child command if it continues a chain', () => {
+    const assertLog = (attrs, log) => {
+      if(log.get('name') === 'findByText') {
+        expect(log.get('type')).to.equal('child')
+        cy.off('log:added', assertLog)
+      }
+    }
+    cy.on('log:added', assertLog)
+    cy.get('body').findByText('Button Text 1')


package.json

NicholasBoll · 2020-01-31T05:50:49Z

src/index.js

@@ -4,6 +4,8 @@ import {getContainer} from './utils'
 const getDefaultCommandOptions = () => {
  return {
    timeout: Cypress.config().defaultCommandTimeout,
+    fallbackToPreviousFunctionality: true,


Thoughts? I figured this could be default behavior to avoid any backwards-breaking changes. This feature isn't currently available through Typescript definitions and maybe shouldn't be. This config can disable the backwards compatibility mode required for #110

I like it. Trying to think of a better name for it though. How about: enableChainContinuation?

Another thing to consider is maybe people would like to configure this globally rather than on a per-command basis. Should we expose a mechanism for changing the default config? Similar to how DOM Testing Library works?

I like it. Trying to think of a better name for it though. How about: enableChainContinuation?

Hmm. I agree fallbackToPreviousFunctionality isn't a good name because it is unclear what it does. enableChainContinuation isn't very descriptive and I interpret it as not what is actually happening.

By default all queries are trying to use previous subject (like Cypress DOM commands do except cy.get) and if an error is encountered, like selector.querySelectorAll is not a function, it will try again without using the previous subject. Setting this property to false will disable that fallback. What about retryWithoutPreviousSubject or fallbackRetryWithoutPreviousSubject?

Another thing to consider is maybe people would like to configure this globally rather than on a per-command basis. Should we expose a mechanism for changing the default config? Similar to how DOM Testing Library works?

I was thinking the same thing. Wrapping the config from DOM Testing Library and extracting this preference. I'll work to add that. 👍

I like retryWithoutPreviousSubject more. Thanks!

I realized that configure is untested and I'm not sure if other people have used it, but if you import the configure function from @testing-library/cypress, you will actually get a different reference that what is used inside query commands. This happens because Cypress is actually running outside your application.

We would have to expose a configure command instead:

cy.configureCypressTestingLibrary(config)

This command would then have access to the same reference... I'm guessing this is either an issue or people haven't configured @testing-library/dom yet

That's fine with me 👍

I doubt people are configuring anything today.

I'm currently validating if my assumptions are correct. What I said is definitely true of importing code from the application into the test, but may not be true in this case. I'll add tests to verify.

I have verified...

That's from a single console.log in the index.js file. Once is called from the import from support.js (where the commands are registered) and the other imported from the test file. They are not the same references, I'll have to add a command to allow for configuring

src/index.js

NicholasBoll · 2020-01-31T05:56:54Z

src/index.js

+          if (failedNewFunctionality && !failedOldFunctionality) {
+            options._log.error(Error(`@testing-library/cypress will soon only use previous subjects when queries are added to a chain of commands. We've detected an instance where the this functionality failed, but the old functionality passed. Please use cy.${queryName}(${queryArgument(args)}) instead of continuing from a previous chain.`))


Cypress doesn't have good deprecation or warning message primitives. This code sets the command in "error" mode and attaches the error on the log, but doesn't show any warning message in the command log. What do people think of this? This triggers when backwards compatibility mode usage was detected prompting an upgrade. Is the message enough?

Wish it didn't say "Error" and instead said "Warning" but that's ok. Could we change the message slightly?

@testing-library/cypress will eventually only use previous subjects when queries are added to a chain of commands. We've detected an instance where the this functionality failed, but the old functionality passed (so your test may break in a future version). Please use cy.${queryName}(${queryArgument(args)}) instead of continuing from a previous chain.

Wish it didn't say "Error" and instead said "Warning"

Agreed. Perhaps that could be a feature request for Cypress to allow for us to trigger warnings

kentcdodds

@NicholasBoll, I can't thank you enough for this. It's fantastic.

I have a few notes, but I'm just so grateful to you for dedicating some time to this.

kentcdodds · 2020-01-31T16:13:58Z

README.md

+built-in retryability using error messages from `get*` APIs to forward as error
+messages if a query fails. `query*` also uses `get*` APIs, but disables retryability.
+
+`findBy*` is less useful in Cypress compared to `findAllBy*`. If you intend to limit


I'm not sure why I would use findAllBy* when I expect there to only be one in the first place. Is the appeal just that you can use findAllBy* 100% of the time and not worry about how many elements are returned?

I like that findBy* communicates (through code) the intent of the test writer (that there should only be one) without having to make an explicit assertion for that.

cypress/fixtures/test-app/index.html

kentcdodds · 2020-01-31T16:16:59Z

src/index.js

@@ -4,6 +4,8 @@ import {getContainer} from './utils'
 const getDefaultCommandOptions = () => {
  return {
    timeout: Cypress.config().defaultCommandTimeout,
+    fallbackToPreviousFunctionality: true,


I like it. Trying to think of a better name for it though. How about: enableChainContinuation?

Another thing to consider is maybe people would like to configure this globally rather than on a per-command basis. Should we expose a mechanism for changing the default config? Similar to how DOM Testing Library works?

kentcdodds · 2020-01-31T16:18:00Z

cypress/integration/find.spec.js

@@ -1,3 +1,5 @@
+/* eslint-disable max-lines-per-function */


Yeah, I don't use describe blocks much in my regular tests, but in Cypress it's kinda necessary. We can add an .eslintrc to the cypress/ directory and disable it in there. Should probably disable the jest promise one too:

{ "rules": { "max-lines-per-function": "off", "jest/valid-expect-in-promise": "off" } }

kentcdodds · 2020-01-31T16:19:31Z

cypress/integration/find.spec.js

+    // This test is a little strange since snapshots show what element
+    // is selected, but snapshots themselves don't give access to those
+    // elements. I had to make the implementation specific so that the `$el`
+    // is the `subject` when the log is added and the `$el` is the `value`
+    // when the log is changed. It would be better to extract the `$el` from
+    // each snapshot


kentcdodds · 2020-01-31T16:24:12Z

src/index.js

-            result.selector = `${queryName}(${queryArgument(args)})`
+        const result = Cypress.$(value)
+        if (value && options._log) {
+          options._log.set('$el', result)


Oh, nice 👍

kentcdodds · 2020-01-31T16:26:06Z

src/index.js

+        if (result.length > 1 && !/all/i.test(queryName)) {
+          // Is this useful?
+          throw Error(`Found multiple elements with the text: ${queryArgument(args)}`)
+        }


Let's change it to case-sensitive (just in case): !/All/.test(queryName).

I think that we should be consistent with the other libraries 👍

kentcdodds · 2020-01-31T16:27:27Z

src/index.js

+      if (queryRegex.test(queryName)) {
+        // make the timeout extremely short to ensure `query*` commands pass or fail instantly
+        options.timeout = 0
+      }


Yes, I think we should do this, though I think that we should not override options.timeout if they're specified.

Also, I'm thinking that in a future breaking change we can remove query* and only support find*.

src/index.js

kentcdodds · 2020-01-31T16:34:54Z

src/index.js

+          if (failedNewFunctionality && !failedOldFunctionality) {
+            options._log.error(Error(`@testing-library/cypress will soon only use previous subjects when queries are added to a chain of commands. We've detected an instance where the this functionality failed, but the old functionality passed. Please use cy.${queryName}(${queryArgument(args)}) instead of continuing from a previous chain.`))


Wish it didn't say "Error" and instead said "Warning" but that's ok. Could we change the message slightly?

@testing-library/cypress will eventually only use previous subjects when queries are added to a chain of commands. We've detected an instance where the this functionality failed, but the old functionality passed (so your test may break in a future version). Please use cy.${queryName}(${queryArgument(args)}) instead of continuing from a previous chain.

kentcdodds · 2020-01-31T16:35:56Z

I approved the PR, but should've just "commented." A few changes are needed and we need to resolve conflicts in the README.md

Thanks again.

NicholasBoll · 2020-01-31T17:40:32Z

Thanks for taking time to review. I wanted this PR to involve dialog, so thanks for that. I'll be going over feedback and improving!

NicholasBoll · 2020-02-01T00:19:55Z

README.md

+`get*` queries are disabled. `find*` queries do not use the Promise API of
+`DOM Testing Library`, but instead forward to the `get*` queries and use Cypress'
+built-in retryability using error messages from `get*` APIs to forward as error
+messages if a query fails. `query*` also uses `get*` APIs, but disables retryability.
+
+`findAll*` can select more than one element and is closer in functionality to how
+Cypress built-in commands work. `findAll*` is preferred to `find*` queries.
+`find*` commands will fail if more than one element is found that matches the criteria
+which is not how built-in Cypress commands work, but is provided for closer compatibility
+to other Testing Libraries.
+
+Cypress handles actions when there is only one element found. For example, the following
+will work without having to limit to only 1 returned element. The `cy.click` will
+automatically fail if more than 1 element is returned by the `findAllByText`:
+
+```javascript
+cy.findAllByText('Some Text').click()
+```
+
+If you intend to enforce only 1 element is returned by a selector, the following
+examples will both fail if more than one element is found.
+
+```javascript
+cy.findAllByText('Some Text').should('have.length', 1)
+cy.findByText('Some Text').should('exist')


Here's the updated documentation about find* queries vs findAll* queries.

NicholasBoll · 2020-02-01T00:24:21Z

NicholasBoll dismissed kentcdodds’s stale review via baf066d 1 hour ago

Wow. That sounds so harsh. All I did was push updated code!

Alright. I think comments have been addressed. I resolved comments for feedback and open questions. I left comments describing PR changes for posterity.

weyert · 2020-02-02T02:21:57Z

Great work @NicholasBoll! It's going to make using Cypress Testing Library even more delightful!

kentcdodds

I just had two small bits of feedback so I did them myself in the github editor. Once the build passes, I'm merging this! THANK YOU SO MUCH!!!

kentcdodds · 2020-02-02T03:46:52Z

Thanks so much for your help @NicholasBoll! I've added you as a collaborator on the project. Please make sure that you review the other/MAINTAINING.md and CONTRIBUTING.md files (specifically the bit about the commit messages and the git hooks) and familiarize yourself with the code of conduct (we're using the contributor covenant). You might also want to watch the repo to be notified when someone files an issue/PR. Please continue to make PRs as you feel the need (you can make your branches directly on the repo rather than your fork if you want). Thanks! And welcome to the team :)

kentcdodds · 2020-02-02T03:48:51Z

🎉 This PR is included in version 5.2.0 🎉

The release is available on:

npm package (@latest dist-tag)
GitHub release

Your semantic-release bot 📦🚀

andycarrell · 2020-02-03T18:21:28Z

Hi 👋 I'm really not sure where the best place to bring this up is, so please direct me if there is a more appropriate channel.

Breaking Changes?:
query* will now fail if no element was found. Before the command would pass an empty object which could cause the next command to fail (e.g. .click()). If a query* failed to return an element, a .should('exist') would not fail because the assertion was given a non-null value. With this change the test should pass/fail as expected, but not as previously implemented.

This is a breaking change, and broke our tests yesterday - which I would not expect from a minor version change. Is there any context to why this was released on a minor version?

NicholasBoll · 2020-02-03T19:01:43Z

@andycarrell Thank you for brining this to our attention.

The original functionality wasn't intentional or specified and didn't seem to be the expected output. I called it out as a potential breaking change, but had no way of knowing if it would effect anyone. A test doing cy.queryByText('Non-existent').click() would still fail, but not in the way you'd think. click would have thrown an error about not having an element rather than query* throwing an error not finding the element.

My change broke a few things that weren't intended later fixed in patches. Unfortunately previous functionality wasn't actually documented or specified and we found out there was a few use-cases that were unexpected based on unspecified behavior.

I'm interested in your use-case. If you have time, can you file an issue on this repo with any relevant information as to what happened, what you expected to happen and why?

andycarrell · 2020-02-03T19:55:21Z

The original functionality wasn't intentional or specified

This doesn't seem to be inline with the documentation for cypress testing library:

I can understand that queryBy* isn't strictly necessary with Cypress, but changing that would be a larger update since it seems to be a departure from testing-library/* patterns.

I agree that cy.queryByText('Non-existent').click() failing "not in the way you'd think" is unideal, and that this change probably wouldn't affect that test failing - it would fail either way. The tests that would now unexpectedly fail are the ones that depend on queryBy*'s current functionality to assert something isn't in the dom.

Specifically we have a helper - that now no longer works with queryBy* and this update :(. If you think it's worth opening an issue for, then I can:

Cypress.Commands.add(
	"retryUntilDoesNotExist",
	{ prevSubject: true },
	subject => {
		cy.wrap(subject).should("not.exist");
	},
);

@NicholasBoll

NicholasBoll · 2020-02-03T21:00:11Z

I think I have a bit more insight. This functionality was actually originally broken by #100. That PR broke other things and was reverted. I verified your custom command worked before, but not afterward. Later this PR updated in a similar way to #100 still breaking your use-case.

You're right that the current code does not reflect this line:

The reason the query* variants are supported is to allow you to assert that elements do not appear on the screen.

This isn't necessary with how Cypress works and how the find* queries work. The following should work without a custom command:

// your current code
cy.queryByText('Eventually non-existent').retryUntilDoesNotExist()

// more idiomatic to other Cypress commands
cy.findByText('Eventually non-existent').should('not.exist')

@kentcdodds what do you think here? We could roll back all query* selectors to before #100 functionality (leaving find* queries alone - I've verified find* queries did not have this issue).

We've talked about deprecating all query* commands entirely in favor of just find* queries with standard Cypress functionality in an official breaking change and warning people not to use query* in documentation and in the command. Personally I'm fine with leaving the query* series alone if some people are dependent on how it works. The find* series should work as expected before and after with only improvements.

andycarrell · 2020-02-03T21:28:06Z

Thanks for confirming @NicholasBoll - I've applied the code change as written here and it works well 🙌

I guess my point here would be that if testing-library/cypress is going to deprecate queryBy* functionality it should introduce a warning and do so in a major version - iirc that's how getBy* was handled?

Incidentally, I'm don't understand why "more idiomatic Cypress" is necessarily a goal for testing-library - I've elaborated more on that thinking here: #108 (comment)

Recently testing-library/cypress-testing-library#108 added a way to take the previous subject of a previous command to scope the query. Also `find*` queries handle `.should('not.exist')` and should be preferred over `query*` which require additional logic for eventual non-existence queries.

kentcdodds · 2020-02-04T16:11:33Z

Sorry to break your code @andycarrell. It was definitely not intentional.

I think the best path forward here is to eventually remove the query* queries and only support find*. With that in mind, if we could restore query* to behave exactly as it had before these changes, but also add a deprecation warning (that's not too spammy/noisy) then we can remove them in the next major version.

I just opened a new issue about this here: #117.

andycarrell · 2020-02-04T18:55:35Z

@kentcdodds Thanks for the response, sounds like a good approach 👐

NicholasBoll · 2020-02-04T19:04:43Z

@andycarrell What version were you on when this broke?

andycarrell · 2020-02-04T19:23:07Z

@NicholasBoll - bumped @testing-library/cypress from 5.1.1 to 5.2.0.

* docs: Update Cypress Testing Library scoping example Recently testing-library/cypress-testing-library#108 added a way to take the previous subject of a previous command to scope the query. Also `find*` queries handle `.should('not.exist')` and should be preferred over `query*` which require additional logic for eventual non-existence queries. * docs: update note about other query types

NicholasBoll commented Jan 30, 2020

View reviewed changes

README.md Show resolved Hide resolved

NicholasBoll commented Jan 30, 2020

View reviewed changes

src/index.js Show resolved Hide resolved

NicholasBoll mentioned this pull request Jan 31, 2020

Support running commands against the previous yielded subject #110

Closed

NicholasBoll added 3 commits January 30, 2020 17:45

Merge branch 'master' of github.com:testing-library/cypress-testing-l…

c9a25b6

…ibrary into pr/add-helpful-debugging-info # Conflicts: # src/index.js

feat: add ability for queries to inherit previous subject

b58f747

* Fixes testing-library#109 without breaking change caused by testing-library#100

feat: add parent/child log type detection

1f14ce1

NicholasBoll commented Jan 31, 2020

View reviewed changes

cypress/integration/find.spec.js Show resolved Hide resolved

NicholasBoll commented Jan 31, 2020

View reviewed changes

package.json Show resolved Hide resolved

NicholasBoll commented Jan 31, 2020

View reviewed changes

src/index.js Show resolved Hide resolved

NicholasBoll commented Jan 31, 2020

View reviewed changes

kentcdodds previously approved these changes Jan 31, 2020

View reviewed changes

chore: implement feedback

217a289

docs: update readme to complete my thought process

aa76c50

NicholasBoll commented Feb 1, 2020

View reviewed changes

kentcdodds added 2 commits February 1, 2020 20:40

slightly simplify config fn

a5bfc6f

Update README.md

d85d918

kentcdodds approved these changes Feb 2, 2020

View reviewed changes

kentcdodds merged commit 547389d into testing-library:master Feb 2, 2020

kentcdodds added the released label Feb 2, 2020

NicholasBoll deleted the pr/add-helpful-debugging-info branch February 2, 2020 03:54

NicholasBoll mentioned this pull request Feb 3, 2020

"Your callback function returned a promise which never resolved" error with cy.queryByText() #37

Closed

NicholasBoll mentioned this pull request Feb 3, 2020

docs: Update Cypress Testing Library scoping example testing-library/testing-library-docs#373

Merged

kentcdodds mentioned this pull request Feb 4, 2020

Revert changes to query*s and add deprecation warning #117

Closed

NicholasBoll mentioned this pull request Feb 5, 2020

test: Add @testing-library/cypress to the project Workday/canvas-kit#442

Merged

6 tasks

This was referenced Mar 5, 2020

chore: add deprecation warning to query* #125

Merged

Release Beta #122

Merged

		})
		cy.get('form').findAllByText('Button Text').should('exist')

		if (failedNewFunctionality && !failedOldFunctionality) {
		options._log.error(Error(`@testing-library/cypress will soon only use previous subjects when queries are added to a chain of commands. We've detected an instance where the this functionality failed, but the old functionality passed. Please use cy.${queryName}(${queryArgument(args)}) instead of continuing from a previous chain.`))

feat: add more helpful debugging information to queries #108

feat: add more helpful debugging information to queries #108

Conversation

NicholasBoll commented Jan 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kentcdodds commented Jan 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kentcdodds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kentcdodds commented Jan 31, 2020

NicholasBoll commented Jan 31, 2020

Choose a reason for hiding this comment

NicholasBoll commented Feb 1, 2020

weyert commented Feb 2, 2020

kentcdodds left a comment

Choose a reason for hiding this comment

kentcdodds commented Feb 2, 2020

kentcdodds commented Feb 2, 2020

andycarrell commented Feb 3, 2020

NicholasBoll commented Feb 3, 2020

andycarrell commented Feb 3, 2020

NicholasBoll commented Feb 3, 2020

andycarrell commented Feb 3, 2020

kentcdodds commented Feb 4, 2020

andycarrell commented Feb 4, 2020

NicholasBoll commented Feb 4, 2020

andycarrell commented Feb 4, 2020

NicholasBoll commented Jan 30, 2020 •

edited

Loading