chore: compare `cy.screenshot` images in percy #21598

marktnoonan · 2022-05-23T13:42:26Z

Closes https://cypress-io.atlassian.net/browse/UNIFY-1566

The main change here is a circle.yml update, to use Percy to diff the screenshot images generated using the cy.screenshot() command from screenshot.cy.tsx. We do this currently on develop, so this ticket was just catch up.

When I did get these uploads working, there were some differences to investigate. Most were are result of global styles and only required changes to test code, not to the app itself.

After those changes to the test code, images only contained some differences in the presence/absences of scrollbars, so this PR includes changes to the app around overflow behavior, to fix those differences.

In addition to these Percy snapshots, I used ImageMagick to compare images generated by the screenshot system tests on both branches, confirming that none of these changes have altered those images.

User facing changelog

Scrollbars in screenshots will now be present/absent in the same situations in 9.x and 10.0. There were some situations where this previously was not the case in 10.0.

Additional details

The rest of the PR is:

a change to the screenshot.cy.tsx test itself to remove global styles. It's a component test, so it mounts in the context of the global app styles. Since global styles are different between 9.x and 10.0, there was some noise in the initial diffs caused by font and line height differences at the global level. Removing the 10.0 global styles in this test makes it much easier to diff with 10.0-release.
Some changes to overflow styles to bring the presence/absence of scrollbars into line between these screenshots when taken from 10.0 and 9.x. These differences hadn't shown up before due to the images being the same, apart from scrollbars. These changes involve taking a similar approach to the current is-screenshotting body class to add an screenshot-scrolling class. This allows us to conditionally add/remove CSS rules related to overflow without adding/removing classes with JavaScript in the nested Vue components. When managing the conditional classes in the Vue components, the styles were modified too late for the screenshot.
There's a tweak to the overflow of the specs-list-container which fixes another edge case that only appeared when scrollbars are present.

Since the latest Percy diff will compare this branch against 10.0-release, I've captured some diffs here between these screenshots on this branch and develop. What can be seen in these screenshots is a 1px shift to the left of all the content, since on 10.0-release (in this PR) we have previously fixed a 1px border bug in component test screenshots.

That means the 1px shifts, shown here in the diff with develop, are expected:

How has the user experience changed?

No UX changes.

PR Tasks

Have tests been added/updated?
[na] Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
[na] Has a PR for user-facing changes been opened in cypress-documentation?
[na] Have API changes been updated in the type definitions?
[na] Have new configuration options been added to the cypress.schema.json?

cypress-bot · 2022-05-23T13:42:29Z

Thanks for taking the time to open a PR!

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.
Become familiar with the Code Review Checklist for guidelines on coding standards and what needs to be done before a PR can be merged.

cypress · 2022-05-23T14:23:35Z

Test summary

4874 • 0 • 59 • 0 • 2

Run details


Project	cypress
Status	Passed
Commit	`2dd755b`
Started	May 26, 2022 1:03 PM
Ended	May 26, 2022 1:22 PM
Duration	18:42 💡
OS	Linux Debian - 10.11
Browser	Electron 100

View run in Cypress Dashboard ➡️

Flakiness

	commands/net_stubbing.cy.ts	1
1	network stubbing > intercepting request > can delay and throttle a StaticResponse
	cypress/proxy-logging.cy.ts	1
1	Proxy Logging > request logging > xhr log has response body/status code when xhr response is logged second

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

marktnoonan · 2022-05-23T15:40:20Z

packages/app/src/pages/Specs/Runner.vue

+.is-screenshotting #resizable-panels-root {
+  overflow-x: visible;
+  overflow-y: visible;
+}


I'd actually like to consolidate more of our screenshot css rules into this structure, instead of conditionally managing tailwind classes with JS. But to keep this PR focused, I'm not doing that here, just moving what's needed.

marktnoonan · 2022-05-23T15:41:14Z

packages/app/src/runner/ResizablePanels.vue

    :class="{
      'select-none': panel1IsDragging || panel2IsDragging,
-      'overflow-x-auto': !isFirefox
+      'overflow-x-hidden': isFirefox


this lets the !important that comes with the utility class override the otherwise-preferred overflow.

marktnoonan · 2022-05-23T15:41:33Z

packages/app/src/runner/SpecRunnerOpenMode.vue

@@ -13,7 +13,7 @@
    />
    <ResizablePanels
      v-else
-      :style="{width: `calc(100vw - ${collapsedNavBarWidth}px)`}"
+      :style="{width: `calc(100vw - ${screenshotStore.isScreenshotting ? 0 : collapsedNavBarWidth}px)`}"


This one was just a loose end with open-mode screenshots.

marktnoonan · 2022-05-23T15:43:06Z

packages/app/src/runner/screenshot/screenshot.cy.tsx

+    }
+  })
+
+  return cy.get('style').should('have.length', 1)


This now means that only the existing CSS injected by the component will be applied. Other CSS targeting body, HTML, global reset CSS etc will not apply here.

We may want to change this later, but for now this gives us the clearest diff with 10.0-release.

This now means that only the existing CSS injected by the component will be applied.

This comment has great context!! What do you think about including it in the code?

marktnoonan · 2022-05-23T15:45:36Z

packages/app/src/specs/InlineSpecListTree.vue

@@ -1,7 +1,7 @@
 <template>
  <div
    v-bind="containerProps"
-    class="pt-8px specs-list-container overflow-y-auto overflow-x-hidden"
+    class="pt-8px specs-list-container overflow-hidden"


Since the scrolling in the specs list takes place within a virtual list, the overflow-x value of auto was causing a weird empty space at the bottom of the parent, to do with how the vh works in the specs-list-container height calculation below.

rachelruderman · 2022-05-23T17:02:46Z

packages/app/src/runner/screenshot/screenshot.cy.tsx

+      removeGlobalStyles().then(() => {
+        cy.screenshot(`percy/component_testing_takes_a_screenshot_viewport_${capture}`, { capture })
+      })


Suggested change

removeGlobalStyles().then(() => {

cy.screenshot(`percy/component_testing_takes_a_screenshot_viewport_${capture}`, { capture })

})

removeGlobalStyles()

cy.screenshot(`percy/component_testing_takes_a_screenshot_viewport_${capture}`, { capture })

AFAIK you don't need .then because Cypress runs promises sequentially and you're not using the return value from removeGlobalStyles within cy.screenshot

tbiethman · 2022-05-24T14:03:35Z

packages/app/src/runner/screenshot/screenshot.cy.tsx

-        cy.screenshot(`percy/large_component_hardcoded_size_viewport_${viewport[0]}_${viewport[1]}`, { capture: 'viewport' })
-        cy.screenshot(`percy/large_component_hardcoded_size_fullPage_${viewport[0]}_${viewport[1]}`, { capture: 'fullPage' })
+        removeGlobalStyles().then(() => {
+          cy.pause()


This pause is still hanging out here.

tbiethman

I compared the generated screenshots to what's in develop, everything lines up how we'd expect. Also validated that they're showing up in Percy.

Great work on this @marktnoonan. It's nice to have regression coverage here, puts us in a good spot to refactor/condense some of these styles going forward.

ZachJW34

Other than the pause looks good to me!

ZachJW34 · 2022-05-25T19:52:31Z

packages/driver/src/cy/commands/screenshot.ts

@@ -98,6 +98,10 @@ const scrollOverrides = (win, doc) => {
  // hide scrollbars
  doc.documentElement.style.overflow = 'hidden'

+  // this body class is used to set some other overflow-related CSS
+  // around the resizable panels in the Runner
+  document.querySelector('body')?.classList.add('screenshot-scrolling')


Not sure why TS is happy with this, if document.querySelector('body')? is ever null (which it shouldn't be) it would be safer to use document.querySelector('body')?.classList?.add('screenshot-scrolling')

I would just do document.body.classList.add('screenshot-scrolling')

I got warnings in vscode when I did plain document.body so not sure what's up.

ZachJW34 · 2022-05-25T19:53:01Z

packages/app/src/runner/screenshot/screenshot.cy.tsx

-        cy.screenshot(`percy/large_component_hardcoded_size_viewport_${viewport[0]}_${viewport[1]}`, { capture: 'viewport' })
-        cy.screenshot(`percy/large_component_hardcoded_size_fullPage_${viewport[0]}_${viewport[1]}`, { capture: 'fullPage' })
+        removeGlobalStyles().then(() => {
+          cy.pause()


…ess into tgriesser/fix/UNIFY-1408 * 'tgriesser/fix/UNIFY-1408' of github.com:cypress-io/cypress: chore: compare `cy.screenshot` images in percy (#21598)

…pack * develop: fix: UNIFY-1408, warnings should be nested appropriately & clearable (#21630) chore: fix unit-tests-release job (#21652) chore(deps): update dependency eventsource to v2 [security] (#21639) fix: Add hover states for test titles in reporter (#21635) docs(CONTRIBUTING): Fix link to "good first issue" for newcomers (#21614) chore: compare `cy.screenshot` images in percy (#21598) fix: switching from ct to e2e (non-configured) does not go through setup (#21607) fix: issue with compilation failures in component testing (#21599) test: fix flaky launchpad test (#21637) docs: remove gitter link in contributing guide. (#21592) fix: order projects by most recently opened (#21589) fix: prevent crash on runs visit when offline (#21618) fix: pass family parameter to connect method (#21545) chore: clean up `debug` statements in preparation for 10.0 release, add `debug` docs (#21621) chore: add regression test for ts detection (#21578)

marktnoonan added 5 commits May 22, 2022 17:21

upload cy.screenshots from CT if found

a065055

remove global styles in screenshot test

e1403d4

tweak #main-pane style during scrolling screenshots

bcdb0b9

ignore size of nav during open-mode screenshots

9b45ca1

remove internal overflow styles

b14d789

tweak overflow behavior

4520626

add comment for clarity

9f559c5

marktnoonan commented May 23, 2022

View reviewed changes

marktnoonan requested review from ZachJW34 and tbiethman May 23, 2022 15:45

marktnoonan marked this pull request as ready for review May 23, 2022 15:46

marktnoonan requested a review from a team as a code owner May 23, 2022 15:46

rachelruderman reviewed May 23, 2022

View reviewed changes

tbiethman reviewed May 24, 2022

View reviewed changes

tbiethman approved these changes May 24, 2022

View reviewed changes

Merge branch '10.0-release' into UNIFY-1566-percy-ci

2d8e4a5

marktnoonan changed the base branch from 10.0-release to develop May 25, 2022 14:03

Merge branch 'develop' into UNIFY-1566-percy-ci

e03377f

ZachJW34 reviewed May 25, 2022

View reviewed changes

ZachJW34 approved these changes May 25, 2022

View reviewed changes

marktnoonan added 4 commits May 25, 2022 16:08

Merge branch 'develop' into UNIFY-1566-percy-ci

d45b485

Merge branch 'develop' into UNIFY-1566-percy-ci

f2a9fc5

clean up test, add comment

b46d5bf

Merge branch 'develop' into UNIFY-1566-percy-ci

2dd755b

marktnoonan merged commit 08e1682 into develop May 26, 2022

marktnoonan deleted the UNIFY-1566-percy-ci branch May 26, 2022 14:08

marktnoonan mentioned this pull request Jun 1, 2022

fix: scroll of inline specs list #21994

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: compare `cy.screenshot` images in percy #21598

chore: compare `cy.screenshot` images in percy #21598

marktnoonan commented May 23, 2022 •

edited

Loading

cypress-bot bot commented May 23, 2022

cypress bot commented May 23, 2022 •

edited

Loading

marktnoonan May 23, 2022

marktnoonan May 23, 2022

marktnoonan May 23, 2022

marktnoonan May 23, 2022

rachelruderman May 23, 2022 •

edited

Loading

marktnoonan May 23, 2022

rachelruderman May 23, 2022

tbiethman May 24, 2022

ZachJW34 May 25, 2022

tbiethman left a comment

ZachJW34 left a comment

ZachJW34 May 25, 2022

marktnoonan May 25, 2022

ZachJW34 May 25, 2022

chore: compare cy.screenshot images in percy #21598

chore: compare cy.screenshot images in percy #21598

Conversation

marktnoonan commented May 23, 2022 • edited Loading

User facing changelog

Additional details

How has the user experience changed?

PR Tasks

cypress-bot bot commented May 23, 2022

cypress bot commented May 23, 2022 • edited Loading

Test summary

Run details

Flakiness

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rachelruderman May 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbiethman left a comment

Choose a reason for hiding this comment

ZachJW34 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chore: compare `cy.screenshot` images in percy #21598

chore: compare `cy.screenshot` images in percy #21598

marktnoonan commented May 23, 2022 •

edited

Loading

cypress bot commented May 23, 2022 •

edited

Loading

rachelruderman May 23, 2022 •

edited

Loading