-
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
fix: disable automatic request retries #19161
Conversation
Thanks for taking the time to open a PR!
|
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 |
packages/server/lib/request.js
Outdated
|
||
if (retryIntervals.length === 0) { | ||
// normally, this request would not be retried, but we need to retry in order to support TLSv1 | ||
return onNext(0, 1) |
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.
what does onNext(0, 1)
do? Is there a way we could make the code more descriptive?
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.
cypress/packages/server/lib/request.js
Lines 284 to 288 in 9e4c13a
const retry = function (delay, attempt) { | |
retryStream.emit('retry', { attempt, delay }) | |
return setTimeout(tryStartStream, delay) | |
} |
So this runs a single retry
instantly.
Is there a way we could make the code more descriptive?
So many ways. 😄 I started refactoring this file and it became a much bigger task, so I decided to keep this PR to the bare minimum changes. For now I pushed a commit to clean up onNext
/onElse
specifically: d618df3
@@ -28,6 +28,7 @@ const REQUEST_DEFAULTS = { | |||
timeout: null, | |||
followRedirect: true, | |||
failOnStatusCode: true, | |||
retryIntervals: [0, 100, 200, 200], |
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.
Why are we adding retry intervals here and above?
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.
Previously this would inherit from the defaults in server/lib/request
, but now it has to set its own.
Automatic retries are still used for cy.visit
's resolve:url
and cy.request
, since those are not retriable by the browser. Fully removing automatic retries is a breaking change b/c we expose retryOnNetworkFailure and retryOnStatusCodeFailure as opts on cy.request, cy.visit
5df0b86
to
2901f91
Compare
2901f91
to
d618df3
Compare
@@ -281,7 +286,7 @@ const createRetryingRequestStream = function (opts = {}) { | |||
const reqStream = r(opts) | |||
let didReceiveResponse = false | |||
|
|||
const retry = function (delay, attempt) { | |||
const retry = function ({ delay, attempt }) { |
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.
👍 to destructuring refactor
* develop: chore(deps): update dependency ssri to 6.0.2 [security] (#19351) chore: Fix server unit tests running on mac by using actual tmp dir (#19350) fix: Add more precise types to Cypress.Commands (#19003) fix: Do not screenshot or trigger the failed event when tests are skipped (#19331) fix (#19262) fix: throw when writing to 'read only' properties of `config` (#18896) fix: close chrome when closing electron (#19322) fix: disable automatic request retries (#19161) chore: refactor cy funcs (#19080) chore(deps): update dependency @ffmpeg-installer/ffmpeg to v1.1.0 🌟 (#19300)
…cycle * 10.0-release: build: remove syncRemoteGraphQL from codegen chore: fix incorrect type from merge build: allow work with local dashboard (#19376) chore: Test example recipes against chrome (#19362) test(unify): Settings e2e tests (#19324) chore(deps): update dependency ssri to 6.0.2 [security] (#19351) fix: spec from story generation, add deps for install (#19352) chore: Fix server unit tests running on mac by using actual tmp dir (#19350) fix: Add more precise types to Cypress.Commands (#19003) fix: Do not screenshot or trigger the failed event when tests are skipped (#19331) fix (#19262) fix: throw when writing to 'read only' properties of `config` (#18896) fix: close chrome when closing electron (#19322) fix: disable automatic request retries (#19161) chore: refactor cy funcs (#19080) chore(deps): update dependency @ffmpeg-installer/ffmpeg to v1.1.0 🌟 (#19300)
User facing changelog
fetch
andxhr
requests could be associated with the wrong request.Additional details
cy.visit
'sresolve:url
andcy.request
, since those are not retriable by the browser.How has the user experience changed?
PR Tasks
cypress-documentation
?type definitions
?cypress.schema.json
?