Add retry-ability to the cypress adapters for dom-testing-library #78

Jmaharman · 2019-08-27T15:42:37Z

What:
Replaced the waitForElement functionality with the built in cypress retry-ability helper to:

Why:

Give feedback in the UI that the command is being re-run, at present you can't see any spinners
Cover a scenario where loading a new page stopped the text from being found

How:
I've had to update src/index.js pretty rapidly to make this possible and while it doesn't break the API it is drastic.

Checklist:

Documentation N/A
Tests
Ready to be merged

While I feel that the code is complete, as well as having more tests now, it is drastically different from what it was previously and so I think a key contributor / project owner will need to look over it closely.

It seems to be that if you load a new page, the retry functionality that's baked in at the moment doesn't work.

There is a built in, but undocumented, promise loop that can be run so that functions can continue to be executed time and time ago, with this enabled the previously failing test now passes. To get it to work I have to trap the getBy exceptions when an item was not found. This means the previously useful error thrown is no longer as easily visible. I've made it accessible by clicking on the command. All getBy, getAllBy, queryBy, queryAllBy, etc, behaviours should all still work as expected. Another side effect of this was that Chai assertion error messages are not as helpful. I've hijacked the jQuery object selector because Cypress uses that in the error message for jQuery object messages. In the selector I've placed a description of the underlying function call, which gives pretty good feedback in the UI window. I had to adjust other tests due to message changes that came with retry-ability.

kentcdodds · 2019-08-27T16:56:05Z

Super appreciate this PR @Jmaharman. It sounds like a great idea, and I'll try to pull it down and play around with it soon. I'd love it if other people watching this repo could do the same and provide feedback as well. Thanks!

kentcdodds · 2019-08-27T16:59:09Z

Looks like the build is failing due to linting issues. If you run npm run validate before pushing that should run everything that the travis build runs so you can make sure you've cleared everything up. Could you do that please?

weyert · 2019-08-27T22:28:09Z

I will see if I can get this PR working locally at my work notebook. This looks exciting. If I understand it correctly this would make it easier to detect appearing or disappearing elements without the need of wait() left and right?

alexkrolick · 2019-08-27T23:23:57Z

This seems like the right direction to close #23, #33, #30, #31

Leaving a few comments on the code...

alexkrolick · 2019-08-27T23:24:54Z

src/index.js

+            }
+            catch (err) {
+              // Catch exceptions where it can't find the element, so we can retry
+              if (/Unable to find an element with the text/.test(err.message))


Should this regex capture all of the errors thrown by DOM Testing Library queries? There are other ones like "Unable to find an element with the label text" etc.

I was originally catching any exception to getBy working as expected, when testing other functionality I realised that the retry-ability stuff would try everything even when the failure is correct. For example getBy, findBy and queryBy all throw an error when matching multiple results, and that scenario is less likely something the user wants to retry, once it fails that is probably the correct result.

If there are other error messages for getByLabelText, getByDisplayValue, getByTestId, getByRole etc, then yes we definitely need to add those or create a specific error type in dom-testing-library and catch those.

I'll defer the decision to someone with greater knowledge of dom-testing-library for that one.

I've since seen that getByLabelText doesn't retry, due to the error message not being as you mention. We can add the relevant messages for each of those queries, but I wonder if there is a way to catch all of those in a more succinct and deliberate way?

@kentcdodds can we create a specific type of error message for get* exceptions in dom-testing-library? That way we can test on something more concrete than the error message text, which could change over time.

What if we add a property to the error object like error.isDTLError = true? Would that work?

Something along those lines would be fine yes. What does DTL stand for?

We could also create an error class in DTL, and use instanceof

The main thing we need to know is how many elements have been found. If it’s 0 then we know we need to re-run the function, if it’s more than that then we let it throw.

Either adding a property to that affect on said errors would work, whether you want to use a new class of error is up to you.

(DTL is DOM Testing Library)

alexkrolick · 2019-08-27T23:25:39Z

src/index.js

+
+            // Overriding the selector of the jquery object because it's displayed in the long message of .should('exist') failure message
+            // Hopefully it makes it clearer, because I find the normal response of "Expected to find element '', but never found it" confusing
+            result.selector = queryName + '(' + findTextOrRegex(args) + ')';


nice improvement

alexkrolick · 2019-08-27T23:28:00Z

src/index.js

+            })
+          }
+
+          if (/queryBy|queryAllBy/.test(queryName)) {


Is this what makes getBy different than queryBy?

I kind of think we should just make findBy and findAllBy the default names in Cypress since they are async even in DTL, and just not implement the other styles.

Yes that's the only difference at the moment. We can't retry the something where it's valid to return a null or empty array, simply because the retry-ability will keep going until it finds something.

I simply run the function to get the dom element and return early, no retryability for these. The only other option would be to keep retrying until just before the original timeout, but to me that would slow down tests unnecessarily, the developer should check the page is ready through some other query before using query*

I kind of think we should just make findBy and findAllBy the default names in Cypress since they are async even in DTL, and just not implement the other styles.

I'm kinda thinking this is the right way to go. What do other's think?

Sounds like a reasonable change. I don't think we need to support all the same queries for Cypress if they don't make sense with the Cypress ecosystem

There were no tests for find* so out of curiosity I've converted all of the get* tests to find* locally to see what happens, of which they all fail.

I've only ever used the get* and query* queries, and hadn't realised the implications of find*.

I've quickly tried to fix the find* queries, however I've found that when it can't find an element it doesn't show the spinner because it's still waiting for a response from the find* queries.

The try-ability code works on the premise that it will find the value, check to see if the next assertion passes and if not try again, until the timeout expires. Due to the fact it only tries once, we don't see the spinner.

I don't mind get* or find* are removed, but either way they both need to be implemented using the get* underlying functions.

To confirm, are you both saying you are happy for CTL to have the 4 queries?

queryBy*

queryAllBy*

findBy*

findAllBy*

Yes, and if we could implement getBy* and getAllBy* to throw an error with a descriptive message that would be good. For example:

You used getByAllText which has been removed from Cypress Testing Library because it does not make sense in this context. Please use findByAllText instead.

I think that would be great :)

By the way, thank you so much for working on this so tirelessly! I'm excited for this to be released and to have a library that works much better with Cypress than ever before.

Not at all, I'm doing very little in the grand scheme of things. We started using this library at work, and I saw the issue and realised I could do something about it.

I need to get something else done, but should be able to do the work over the weekend / early next week.

Super excited to get this merged! I will see if I can use your repo at my work's project to improve the tests and let you know how it goes. Currently, I have a bunch of wait() to give the page time to render some elements.

My understanding is that with this PR I could avoid these changes. I also it sounds like it should use findByText etc instead of getByText.

How can I help getting this merged?

Sorry for the delay guys, things have been hectic at work and at home. I will get onto it soon.

src/index.js

find* now used query* implementations, but hooked up via the Cypress retry-ability so that it remains async as per the main DTL API. get* queries are now redundant, because users expect the retry-ability from Cypress by default, we now throw an error asking the user to use find* instead. query* queries continue to work as they did before. I've created tests for all functions, and then the main behaviors associated to each type of query.

Jmaharman · 2019-09-03T22:45:37Z

Here you go all, get that down and give it a test in your own projects.

weyert · 2019-09-03T23:03:30Z

Thanks, I will give it a try tomorrow morning at work. I hope I can remove a bunch of wait() in my tests now 💃

kentcdodds · 2019-09-04T04:48:24Z

I resolved some merge conflicts and published this as 5.0.0-alpha.0 (you can install it via npm install @testing-library/cypress@alpha). I've tried it on my projects and upgrading was as simple as find/replace .get with .find in the cypress/ directory. 🎉 Great work here! I say this is ready to go!

kentcdodds · 2019-09-04T04:49:23Z

Oh, and this is great 👍

kentcdodds

This is good to go IMO.

kentcdodds · 2019-09-04T04:58:08Z

Just FYI to whoever merges this (can be me or another maintainer), make sure to use Squash and merge for this one and follow the instructions. This one will definitely need BREAKING CHANGE: ... at the end of the commit message so we can get a major version bump.

alexkrolick · 2019-09-04T05:38:08Z

LGTM, tested with a few smoke tests and just needed to update the get queries to find.

kentcdodds · 2019-09-04T05:50:09Z

🎉 This PR is included in version 5.0.0 🎉

The release is available on:

npm package (@latest dist-tag)
GitHub release

Your semantic-release bot 📦🚀

Jmaharman · 2019-09-04T06:37:27Z

I've realised this morning that I'm not retrying when multiple matches are found, it fails early. I'll open another PR for it.

kathar1223 · 2019-10-13T19:57:17Z

@Jmaharman i installed the latest version of this library. but unfortunately i don't see the command is not waiting for element to exist.
eg : cy.getByTestId('test-id') used to implicitly wait based on the defaultcommandtimeout given in cypress.json.

but after changing to cy.findByTestId('') doesn't wait and also doesn't fail even after passing wrong element to it.

I am i missing something here ?

kathar1223 · 2019-10-13T20:15:02Z

@Jmaharman @kentcdodds can you please respond to the above comment

Jmaharman · 2019-10-13T21:50:53Z

Currently on holiday, I will be back online next week.

…

On 14 Oct 2019, at 05:15, Kamran Athar ***@***.***> wrote: @Jmaharman @kentcdodds can you please respond to the above comment — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

kentcdodds · 2019-10-23T01:26:05Z

Ping

Jmaharman · 2019-10-23T06:49:21Z

@kathar1223 I've tried to replicate this issue in the Cypress testing suite for this project and found no issue. It waits for the cypress default of 4 seconds and then returns the below error:

I assume there is something else occurring here for the issue to arise for you. Can you explain your scenario in full? Are any of the preceeding interactions modifying the page in someway, e.g. previous command clicks a new link, which loads a new page

A reproducible example would be appreciated.

Jmaharman added 2 commits August 27, 2019 16:27

Creating a test that fails due to no retry-ability

9a6a02c

It seems to be that if you load a new page, the retry functionality that's baked in at the moment doesn't work.

Jmaharman mentioned this pull request Aug 27, 2019

getAllBy* commands chained by should don't retry correctly #23

Closed

alexkrolick reviewed Aug 27, 2019

View reviewed changes

Jmaharman added 3 commits August 28, 2019 08:02

Changes made to variable scope to pass linting rules

0c30eb1

Renamed findTextOrRegex to queryArgument so it's clearer what it is for

72436cb

Merge branch 'master' into master

4ebd0e6

Update index.js

4518d6f

kentcdodds approved these changes Sep 4, 2019

View reviewed changes

alexkrolick merged commit ab5263d into testing-library:master Sep 4, 2019

kentcdodds added the released label Sep 4, 2019

alexkrolick mentioned this pull request Sep 19, 2019

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

Closed

NicholasBoll mentioned this pull request Feb 5, 2020

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

Merged

DanielPe05 mentioned this pull request Sep 14, 2020

Timed out retrying: cy.click() failed because this element is detached from the DOM #153

Closed

Add retry-ability to the cypress adapters for dom-testing-library #78

Add retry-ability to the cypress adapters for dom-testing-library #78

Conversation

Jmaharman commented Aug 27, 2019

kentcdodds commented Aug 27, 2019

kentcdodds commented Aug 27, 2019

weyert commented Aug 27, 2019

alexkrolick commented Aug 27, 2019 • 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

Choose a reason for hiding this comment

Jmaharman commented Sep 3, 2019

weyert commented Sep 3, 2019

kentcdodds commented Sep 4, 2019

kentcdodds commented Sep 4, 2019

kentcdodds left a comment

Choose a reason for hiding this comment

kentcdodds commented Sep 4, 2019

alexkrolick commented Sep 4, 2019

kentcdodds commented Sep 4, 2019

Jmaharman commented Sep 4, 2019

kathar1223 commented Oct 13, 2019

kathar1223 commented Oct 13, 2019

Jmaharman commented Oct 13, 2019 via email

kentcdodds commented Oct 23, 2019

Jmaharman commented Oct 23, 2019

alexkrolick commented Aug 27, 2019 •

edited

Loading