feat: refactor cy.intercept internals to generic event-based system #15255
Conversation
This reverts commit d662c5d. Had to revert this because it ends up creating an inconsistency in the APIs. res.delay() is a function, which is why res.delayMs needs to be the scalar.
|
Thanks for taking the time to open a PR!
|
|
Test summary
Run 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 |
| 'before:request': onBeforeRequest, | ||
| 'response': onResponse, | ||
| 'after:response': onAfterResponse, |
There was a problem hiding this comment.
the next step after this PR is to add request and before-response events here, and expose req.on to the user
| @@ -34,14 +36,16 @@ export const onRequestComplete: HandlerFn<NetEventFrames.HttpRequestComplete> = | |||
| if (isAwaitingResponse) { | |||
| // the user is implicitly expecting there to be a successful response from the server, so fail the test | |||
| // since a network error has occured | |||
| return failCurrentTest(err) | |||
| throw err | |||
There was a problem hiding this comment.
Might need to check how this interacts with the latest error handling and ensure the right error is displayed.
I'm of the opinion now that, whenever possible, we should avoid re-throwing errors when we have already caught them and instead wrap them with the context where they're caught and report them via other means (like how failCurrentTest calls into cy.fail). There's a lot of older code that doesn't follow this pattern, but for new or changed code, it would be nice to have it do so.
There was a problem hiding this comment.
I think it would be worthwhile to add a test for this error here: https://github.com/cypress-io/cypress/blob/develop/packages/runner/cypress/integration/reporter.errors.spec.js
There was a problem hiding this comment.
After our pairing session, I added the reporter tests here: 41e0e79 (#15255)
There's still the issue with double-wrapping cy.intercept errors, in addition to cy.fail propagating to top in the reporter tests.
| @@ -34,14 +36,16 @@ export const onRequestComplete: HandlerFn<NetEventFrames.HttpRequestComplete> = | |||
| if (isAwaitingResponse) { | |||
| // the user is implicitly expecting there to be a successful response from the server, so fail the test | |||
| // since a network error has occured | |||
| return failCurrentTest(err) | |||
| throw err | |||
There was a problem hiding this comment.
I think it would be worthwhile to add a test for this error here: https://github.com/cypress-io/cypress/blob/develop/packages/runner/cypress/integration/reporter.errors.spec.js
|
Released in This comment thread has been locked. If you are still experiencing this issue after upgrading to |
User facing changelog
Additional details
req.on....)http:request:received,http:response:received,http:response:completeevent implementations to a generic implementation that calls handlers for all 3How has the user experience changed?
PR Tasks
cypress-documentation? docs: revert "delayMs -> delay" cypress-documentation#3629type definitions?cypress.schema.json?