-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
retry CRI.List when connecting to the browser #6133
Conversation
Thanks for the contribution! Below are some guidelines Cypress uses when doing PR reviews.
PR Review ChecklistIf any of the following requirements can't be met, leave a comment in the review selecting 'Request changes', otherwise 'Approve'. User Experience
Functionality
Maintainability
Quality
Internal
|
Test summaryRun details
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 |
Funny enough, I see empty targets right now in another recipe with Chrome
So the message could be in your case
But I seriously doubt it now. We could keep the current retry in this PR and add a message there https://github.com/sindresorhus/p-retry#onfailedattempterror but again I doubt it will give you much
|
Oh, one last thing. Monkey-patching 3.8.1 binary using new code in
👀 on the 🏀 |
@bahmutov I updated the way it retries to incorporate the delay messaging & added tests that it console.logs as expected. I tried using |
@flotwig yeah, now you just need to use extra variables and write custom code, I understand it might feel simple right now. But I don't want to argue about it any more. We have an example of failing CRI target in GH action! https://github.com/cypress-io/cypress-example-kitchensink/runs/383684300 |
Looks like I'm a bit late to the party, but instead of polling one could listen to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, but I'm not sure how to test locally. @bahmutov it would be great to test this one last time with your super super slow Chrome setup and ensure that it still works.
I have tested this branch using
Before, it was failing, but now it is retrying correctly. |
@erezrokah Thanks! I've created an issue here to track that: #6153. Since this PR already has tests and has been manually confirmed, we'll merge it for now, but using the CDP events would be an improvement since it could cut down on the potential slowdown from retrying at an interval. |
Thanks @flotwig, I'm just glad to see it resolved :) |
When can we expect this fix to be released? |
CRI.List
if theabout:blank
page cannot be found, maybe the browser is startingUser facing changelog
The CI runs should stop failing due to Cypress not being able to find the tab to control.
Additional details
I did experiments with closing and opening Chrome Canary with lots of tabs and extensions and could get into situations where the initial
CRI.List
call returns no targets, but the next call returns full list of tabs. Adding retries seems like a solution we need to try.How has the user experience changed?
No changes, unless the user is running Cypress with debug logs
DEBUG=cypress:server:protocol
. In that case they should see several callsPR Tasks