-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
feat(intercept): add { log: false }
to StaticResponse
#25547
Conversation
{ log: false }
to StaticResponse
and req
{ log: false }
to StaticResponse
30 flaky tests on run #44715 ↗︎
Details:
create-from-component.cy.ts • 1 flaky test • app-e2e
specs_list_latest_runs.cy.ts • 1 flaky test • app-e2e
cypress-in-cypress-component.cy.ts • 1 flaky test • app-e2e
commands/net_stubbing.cy.ts • 1 flaky test • 5x-driver-firefox
cypress-origin-communicator.cy.ts • 1 flaky test • app-e2e
The first 5 flaky specs are shown, see all 18 specs in Cypress Cloud. This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. |
72733c8
to
47c787f
Compare
logs.push(log) | ||
}) | ||
|
||
cy.intercept('**/cypress.png?*', { log: true }).as('log-me') |
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.
Could shorten this test slightly by combining the two image fetches into one .then()
, and wait on both requests with the same .wait()
.
https://docs.cypress.io/api/commands/wait#Usage
I only recently learned you can cy.wait(['@alias1', '@alias2'])
on an array of aliases, so spreading that learning around a bit. :)
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.
Good tip, I didn't know that... somehow 🤦 I think I'll keep this as-is though, because it's simpler in my mind to first cleanly assert that dont-log-me
hasn't logged and then separately assert that log-me
is logged.
if (eventName === 'before:request') { | ||
if (immediateStaticResponse) { | ||
if (eventName === 'before:request' && immediateStaticResponse) { | ||
// Since StaticResponse is conflated with InterceptOptions, only send an immediate response if there are keys other than `log`. |
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.
This comment solidified some of what I've felt reading the code here - static response and log are conflated in the code.
It makes sense that they overlap when .intercept()
is called - it's the cost of how we've overloaded the arguments.
But I wonder if things would be cleaner if "log" were pulled out at that one point - so you'd have a static response / no static response, and then log / don't log to be passed around separately, rather than passing one object encompassing both, pulled apart into the components as-needed across multiple areas.
Thoughts?
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.
Agreed, it's pretty messy this way. I'll update the to break out the options from the StaticResponse
. How do you feel about the public API combining log
with StaticResponse
?
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.
Hm. This method signature is heavily overloaded - is it possible to fold it into routeMatcher
rather than staticResponse
? That might make more sense at a glance, but I haven't worked through the implications; I'll believe you if you say that's complicated / we can't do it. 🤣
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.
Minor note for when you work on docs - we have some broken links on the .intercept()
page. https://docs.cypress.io/api/commands/intercept#Syntax- all the links in the Syntax section don't connect properly.
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.
One nice thing about having it in the 2nd argument is that it maintains this sort of signature:
cy.intercept(which requests you're targeting, what you want to do to those requests)
But the 2nd argument is greatly overloaded as you say, so putting it in the first could be nice. However, when we go to add req.log
and allow dynamically toggling logs in the future, the user could set it in two places:
cy.intercept({ log: false }, (req) => req.log = false)
...which starts to feel a little weird to me.
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.
I worked on ^^ a bit and didn't feel that it was really making the code much clearer. I think this would be worth re-evaluating if we had more InterceptOptions
, but with just log
it's a marginal improvement for now.
I had trouble parsing out what exactly was the final decision for this feature from the click up and I think i may have missed a couple of meetings. Could you elaborate further on the pr as to what the change is? |
@mjhenkes good call-out, updated. See |
// eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
const debug = null |
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.
We could removed this right?
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.
This is here so that developers get an error if they attempt to invoke debug
in this file - since this file contains only proxy
middleware, any debug
calls should be to this.debug
so they get the advantage of the extra contextual information (method, URL, stage) about the HTTP request:
cypress/packages/proxy/lib/http/index.ts
Lines 298 to 302 in 2a67033
debug: (formatter, ...args) => { | |
if (!debugVerbose.enabled) return | |
debugVerbose(`${colorFn!(`%s %s`)} %s ${formatter}`, req.method, debugUrl, chalk.grey(ctx.stage), ...args) | |
}, |
The same pattern is followed in proxy
:
cypress/packages/proxy/lib/http/request-middleware.ts
Lines 9 to 12 in 2a67033
// do not use a debug namespace in this file - use the per-request `this.debug` instead | |
// available as cypress-verbose:proxy:http | |
// eslint-disable-next-line @typescript-eslint/no-unused-vars | |
const debug = null |
I guess you could get by with a lint rule for this, too, but I think I did this pattern in proxy
before I knew about blocking variable names with eslint.
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. We'll need a corresponding cypress docs update too since we're adding a new option to intercept right?
@mjhenkes Just put up the docs PR: cypress-io/cypress-documentation#5106 |
Additional details
{ log: false }
in theStaticResponse
field. For example:cy.intercept
can selectively disable these.req.log
support: Support dynamically disabling request logs viacy.intercept()
#26069Steps to test
Pass
{ log: false }
as the 2nd argument to cy.intercept to hide logs.How has the user experience changed?
PR Tasks
cypress-documentation
? docs: add{ log: false }
toStaticResponse
cypress-documentation#5106type definitions
?