fix: Adding overload to screenshot to fix type errors when overwriting this command. #27130

0neMiss · 2023-06-24T03:19:22Z

Additional details

Why was this change necessary?

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.

What is affected by this change?
type definitions for the cy.screenshot method.

Steps to test

when copying this code snippet from the cypress docs

Cypress.Commands.overwrite(
  'screenshot',
  (originalFn, subject, fileName, options) => {
    // call another command, no need to return as it is managed
    cy.get('.app')
      .should('be.visible')

      // overwrite the default timeout, because screenshot does that internally
      // otherwise the `then` is limited to the default command timeout
      .then({ timeout: Cypress.config('responseTimeout') }, () => {
        // return the original function so that cypress waits for it
        return originalFn(subject, fileName, options)
      })
  }
)

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

Have tests been added/updated? I dont think this is needed but let me know if i'm wrong on that.
Has a PR for user-facing changes been opened in 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.
Have API changes been updated in the type definitions? No api changes, just type changes.

… each overflow

CLAassistant · 2023-06-24T03:19:28Z

All committers have signed the CLA.

cypress-app-bot · 2023-06-24T03:19:42Z

Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.

cypress · 2023-06-24T03:49:55Z

1 failed and 34 flaky tests on run #48534 ↗︎

1	27936	1349	0	34

Details:

code review changes
Project: cypress	Commit: `2d66993bde`
Status: Failed	Duration: 19:51 💡
Started: Jul 2, 2023 4:24 AM	Ended: Jul 2, 2023 4:44 AM

cypress/e2e/scaffold-project.cy.ts • 1 failed test • launchpad-e2e

View Output Video

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

View Output Video

Test		Artifacts
network stubbing > intercepting request > can delay and throttle a StaticResponse		`Output` `Video`

cypress/cypress.cy.js • 3 flaky tests • 5x-driver-electron

View Output Video

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

View Output Video

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

View Output Video

Test		Artifacts
cy.origin navigation > #consoleProps > .go()		`Output` `Video`

commands/waiting.cy.js • 1 flaky test • 5x-driver-chrome:beta

View Output Video

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.}

mike-plummer

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 under CypressScreenshotTests so we can validate that Typescript remains happy with this new definition? You can use the // $ExpectType hint to ensure proper subject handling

mike-plummer · 2023-06-27T19:31:20Z

cli/types/cypress.d.ts

+     * @example
+     *    cy.get(".post").screenshot("post-element")
+     */
+    screenshot(context: string, fileName?: string, options?: Partial<Loggable & Timeoutable & ScreenshotOptions>): Chainable<null>,


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>,

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

@mike-plummer Ready for another pass I think!

mike-plummer · 2023-07-05T14:41:06Z

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:

On the TS types for screenshot we need to specify the return type as Chainable<Subject> which lets TS infer the subject based on the chaining. This means there's no need for an actual subject parameter, it's implicitly added to the function signature based on how TS infers the command is being invoked.
When overwriting there isn't an implicit chain, so TS can't 100% tell if there's a subject or not. In this case we need to provide appropriate generics to the overwrite command to tell it the command we're overwriting is being overwritten in a way that handles a subject. This is the Cypress.Commands.overwrite<'screenshot', 'element'> bit I added

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

lmiller1990 · 2023-07-06T23:12:09Z

Looks good, did you want more changes or can we ✅ this one @mike-plummer ?

0neMiss · 2023-07-07T03:48:29Z

Thanks @mike-plummer for the feedback and the assistance!

mike-plummer · 2023-07-07T14:09:36Z

@0neMiss Merged! This should go out in our next release - we typically try to maintain ~2 week release cadence. Thanks again for the contribution!

cypress-bot · 2023-07-10T20:09:08Z

Released in 12.17.1.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v12.17.1, please open a new issue.

0neMiss added 2 commits June 23, 2023 21:10

fixing overload match for case when screenshot is called while chaining.

8eaf27e

updating jsdocs example to correctly reflect the overflow matches for…

60d3313

… each overflow

0neMiss changed the title ~~Adding overload to screenshot to fix type errors when overwriting this command.~~ fix - Adding overload to screenshot to fix type errors when overwriting this command. Jun 24, 2023

0neMiss mentioned this pull request Jun 24, 2023

Update overwrite screenshot section cypress-io/cypress-documentation#5323

Merged

0neMiss changed the title ~~fix - Adding overload to screenshot to fix type errors when overwriting this command.~~ fix: Adding overload to screenshot to fix type errors when overwriting this command. Jun 24, 2023

changelog

7ae610b

0neMiss marked this pull request as draft June 25, 2023 03:50

mike-plummer self-assigned this Jun 26, 2023

mike-plummer requested changes Jun 27, 2023

View reviewed changes

0neMiss added 4 commits July 1, 2023 20:31

merge conflict

66429d9

code comments

067e86d

code review changes

2d66993

changelog again

0109396

0neMiss marked this pull request as ready for review July 2, 2023 04:13

0neMiss and others added 2 commits July 1, 2023 22:39

fixing test

2300a1d

Update types for implicit subject, add tests

09503d0

Update changelog

d470d9d

mike-plummer requested review from astone123 and lmiller1990 July 6, 2023 12:57

mike-plummer added 2 commits July 6, 2023 12:23

Merge branch 'develop' into pr/27130

a1887f6

Fix changelog

56deaa9

lmiller1990 approved these changes Jul 6, 2023

View reviewed changes

mike-plummer approved these changes Jul 7, 2023

View reviewed changes

Merge branch 'develop' into fix-screenshot-overload

4b11130

mike-plummer merged commit d56b628 into cypress-io:develop Jul 7, 2023
60 of 62 checks passed

cypress-bot bot locked as resolved and limited conversation to collaborators Jul 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

0neMiss commented Jun 24, 2023 •

edited

CLAassistant commented Jun 24, 2023 •

edited

cypress-app-bot commented Jun 24, 2023

cypress bot commented Jun 24, 2023 •

edited

mike-plummer left a comment

mike-plummer Jun 27, 2023

0neMiss Jun 28, 2023 •

edited

0neMiss Jul 2, 2023 •

edited

mike-plummer commented Jul 5, 2023

lmiller1990 commented Jul 6, 2023

0neMiss commented Jul 7, 2023 •

edited

mike-plummer commented Jul 7, 2023

cypress-bot bot commented Jul 10, 2023

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

0neMiss commented Jun 24, 2023 • edited

Additional details

Steps to test

How has the user experience changed?

PR Tasks

CLAassistant commented Jun 24, 2023 • edited

cypress-app-bot commented Jun 24, 2023

cypress bot commented Jun 24, 2023 • edited

1 failed and 34 flaky tests on run #48534 ↗︎

mike-plummer left a comment

Choose a reason for hiding this comment

mike-plummer Jun 27, 2023

Choose a reason for hiding this comment

0neMiss Jun 28, 2023 • edited

Choose a reason for hiding this comment

0neMiss Jul 2, 2023 • edited

Choose a reason for hiding this comment

mike-plummer commented Jul 5, 2023

lmiller1990 commented Jul 6, 2023

0neMiss commented Jul 7, 2023 • edited

mike-plummer commented Jul 7, 2023

cypress-bot bot commented Jul 10, 2023

0neMiss commented Jun 24, 2023 •

edited

CLAassistant commented Jun 24, 2023 •

edited

cypress bot commented Jun 24, 2023 •

edited

0neMiss Jun 28, 2023 •

edited

0neMiss Jul 2, 2023 •

edited

0neMiss commented Jul 7, 2023 •

edited