-
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
fix: Adding overload to screenshot to fix type errors when overwriting this command. #27130
fix: Adding overload to screenshot to fix type errors when overwriting this command. #27130
Conversation
|
1 failed and 34 flaky tests on run #48534 ↗︎Details:
|
Test | Artifacts | |
---|---|---|
scaffolding new projects > generates valid config file for pristine project without cypress installed |
Output
Screenshots
Video
|
commands/net_stubbing.cy.ts • 1 flaky test • 5x-driver-electron
Test | Artifacts | |
---|---|---|
network stubbing > intercepting request > can delay and throttle a StaticResponse |
Output
Video
|
cypress/cypress.cy.js • 3 flaky tests • 5x-driver-electron
Test | Artifacts | |
---|---|---|
... > correctly returns currentRetry |
Output
Video
|
|
... > correctly returns currentRetry |
Output
Video
|
|
... > correctly returns currentRetry |
Output
Video
|
commands/net_stubbing.cy.ts • 1 flaky test • 5x-driver-chrome:beta
Test | Artifacts | |
---|---|---|
network stubbing > intercepting request > can delay and throttle a StaticResponse |
Output
Video
|
e2e/origin/commands/navigation.cy.ts • 1 flaky test • 5x-driver-chrome:beta
Test | Artifacts | |
---|---|---|
cy.origin navigation > #consoleProps > .go() |
Output
Video
|
commands/waiting.cy.js • 1 flaky test • 5x-driver-chrome:beta
Test | Artifacts | |
---|---|---|
... > errors > throws when route is never resolved |
Output
Video
|
The first 5 flaky specs are shown, see all 21 specs in Cypress Cloud.
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.
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.
Hey @0neMiss , thanks for chasing this down. Couple things I'll point out:
- This PR is marked as a Draft - are you still working on it, or was it ready for review?
- Unfortunately there was a release yesterday so there's a conflict on the
CHANGELOG
file now that'll have to get resolved - Could you take a look at adding a case or two to
cli/types/tests/cypress-tests.ts
underCypressScreenshotTests
so we can validate that Typescript remains happy with this new definition? You can use the// $ExpectType
hint to ensure propersubject
handling
cli/types/cypress.d.ts
Outdated
* @example | ||
* cy.get(".post").screenshot("post-element") | ||
*/ | ||
screenshot(context: string, fileName?: string, options?: Partial<Loggable & Timeoutable & ScreenshotOptions>): Chainable<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.
I'm assuming the context
parameter is meant to represent the subject
from the previously-chained command? I think that should look more like this (note the Chainable<Subject>
instead of Chainable<null>
) based on examples elsewhere in this file
screenshot(fileName?: string, options?: Partial<Loggable & Timeoutable & ScreenshotOptions>): Chainable<Subject>,
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.
Hey sorry! Yea I'm having a hard time getting the yarn install to work. Something about python versions and some microsoft tools. That's why I marked it as a draft because I haven't actually been able to run and build this locally yet.
I also saw that this check was failing and I wasn't sure what steps to take to resolve that. Any help on either of those things would be appreciated!
I'll update to fix the Chainable<Subject>
and the changelog. I actually don't know why I called that variable context haha.
I'll add those test cases as well, granted that I figure out this issue with running locally. If you have some suggestions for me to try i'd be happy to. Otherwise this might have to wait till later this week when I'm a little more available.
Thanks for taking a look @mike-plummer
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.
@mike-plummer Ready for another pass I think!
Hey @0neMiss , thanks for the update. I pulled this down and made a few tweaks based on what I found (hope you don't mind 😬 ). I think there are really two bits of magic sauce we have to apply to get this working:
I also added a couple tests to validate the subject passing behavior. Hopefully CI agrees that things look good (tests run fine locally). LMK if you disagree or see a hole in my logic. I'm going to push one more minor tweak to the changelog just for formatting purposes - assuming you're good with my changes I can wrangle some reviewers and get this merged. We're running a release today so this probably won't make the cut, but we should be able to get it in for the next one |
Looks good, did you want more changes or can we ✅ this one @mike-plummer ? |
Thanks @mike-plummer for the feedback and the assistance! |
@0neMiss Merged! This should go out in our next release - we typically try to maintain ~2 week release cadence. Thanks again for the contribution! |
Released in This comment thread has been locked. If you are still experiencing this issue after upgrading to |
Additional details
When trying to overwrite this command copying what is mentioned in the docs will throw a type error. The example provided in the docs is correct however the type definitions need to have an overload case that accounts for when screenshot is chained from another cypress command. Currently there is no overload that matches this case.
type definitions for the
cy.screenshot
method.Steps to test
when copying this code snippet from the cypress docs
We should no longer see the type errors outlined in this post from the docs.
cypress-io/cypress-documentation#5323 (comment)
How has the user experience changed?
Should no longer see type errors when trying to overwrite the screenshot command. See the linked comment above for screenshots of the error messages.
PR Tasks
cypress-documentation
? Update overwrite screenshot section cypress-documentation#5323 (comment)I made a pr to update the docs for this, however the docs were mostly correct already, the type definitions are what's the issue here.
type definitions
? No api changes, just type changes.