Skip to content
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

chore: compare cy.screenshot images in percy #21598

Merged
merged 13 commits into from
May 26, 2022
Merged
10 changes: 10 additions & 0 deletions circle.yml
Original file line number Diff line number Diff line change
Expand Up @@ -526,6 +526,16 @@ commands:
PERCY_ENABLE=${PERCY_TOKEN:-0} \
PERCY_PARALLEL_TOTAL=-1 \
$cmd yarn workspace @packages/<<parameters.package>> cypress:run:<<parameters.type>> --browser <<parameters.browser>> --record --parallel --group <<parameters.package>>-<<parameters.type>>
- run:
command: |
if [[ <<parameters.package>> == 'app' && <<parameters.percy>> == 'true' && -d "packages/app/cypress/screenshots/runner/screenshot/screenshot.cy.tsx/percy" ]]; then
PERCY_PARALLEL_NONCE=$CIRCLE_SHA1 \
PERCY_ENABLE=${PERCY_TOKEN:-0} \
PERCY_PARALLEL_TOTAL=-1 \
yarn percy upload packages/app/cypress/screenshots/runner/screenshot/screenshot.cy.tsx/percy
else
echo "skipping percy screenshots uploading"
fi
- store_test_results:
path: /tmp/cypress
- store_artifacts:
Expand Down
18 changes: 18 additions & 0 deletions packages/app/src/pages/Specs/Runner.vue
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,24 @@ iframe.aut-iframe {
background: white;
}

.is-screenshotting #main-pane {
overflow: auto !important;
}

.is-screenshotting.screenshot-scrolling #main-pane {
overflow: visible !important;
}

#resizable-panels-root {
overflow-x: auto;
overflow-y: hidden;
}

.is-screenshotting #resizable-panels-root {
overflow-x: visible;
overflow-y: visible;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


iframe.spec-iframe {
border: none;
height: 0;
Expand Down
6 changes: 4 additions & 2 deletions packages/app/src/runner/ResizablePanels.vue
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
<template>
<div
class="flex overflow-y-hidden"
id="resizable-panels-root"
class="flex"
:class="{
'select-none': panel1IsDragging || panel2IsDragging,
'overflow-x-auto': !isFirefox
'overflow-x-hidden': isFirefox
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}"
@mouseup="handleMouseup"
@mousemove="handleMousemove"
Expand Down Expand Up @@ -209,4 +210,5 @@ watchEffect(() => {

// TODO: UNIFY-1704 - avoid special case for FF
const isFirefox = window.__CYPRESS_BROWSER__?.family === 'firefox'

</script>
2 changes: 1 addition & 1 deletion packages/app/src/runner/SpecRunnerOpenMode.vue
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
/>
<ResizablePanels
v-else
:style="{width: `calc(100vw - ${collapsedNavBarWidth}px)`}"
:style="{width: `calc(100vw - ${screenshotStore.isScreenshotting ? 0 : collapsedNavBarWidth}px)`}"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

:offset-left="collapsedNavBarWidth"
:max-total-width="windowWidth - collapsedNavBarWidth"
:initial-panel1-width="specsListWidthPreferences"
Expand Down
29 changes: 24 additions & 5 deletions packages/app/src/runner/screenshot/screenshot.cy.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,16 @@ const Layout: FunctionalComponent = () => {

const captureTypes = ['fullPage', 'viewport', 'runner'] as const

function removeGlobalStyles () {
cy.get('style').each((item) => {
if (item[0].dataset.cy !== 'injected-style-tag') {
item.remove()
}
})

return cy.get('style').should('have.length', 1)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@rachelruderman rachelruderman May 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}

describe('screenshot', () => {
captureTypes.forEach((capture) => {
it(`takes a standard screenshot with viewport: ${capture}`, () => {
Expand All @@ -45,7 +55,9 @@ describe('screenshot', () => {
styles,
})

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

})
})

Expand All @@ -55,7 +67,9 @@ describe('screenshot', () => {
styles,
})

cy.screenshot('percy/component_testing_screenshot_custom_viewport_screenshot')
removeGlobalStyles().then(() => {
cy.screenshot('percy/component_testing_screenshot_custom_viewport_screenshot')
})
})

it('screenshot with a really long viewport', () => {
Expand All @@ -64,7 +78,9 @@ describe('screenshot', () => {
styles,
})

cy.screenshot('percy/component_testing_screenshot_long_viewport')
removeGlobalStyles().then(() => {
cy.screenshot('percy/component_testing_screenshot_long_viewport')
})
})

const style = `
Expand Down Expand Up @@ -134,8 +150,11 @@ describe('screenshot', () => {
}

mount(() => <Comp />, { style }).then(() => {
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()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pause is still hanging out here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bump

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' })
})
})
})
})
Expand Down
4 changes: 2 additions & 2 deletions packages/app/src/specs/InlineSpecListTree.vue
Original file line number Diff line number Diff line change
@@ -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"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

>
<ul
v-bind="wrapperProps"
Expand Down Expand Up @@ -157,7 +157,7 @@ a::before {
}

/** h-[calc] was getting dropped so moved to styles. Virtual list requires defined height. */
/** Header is 64px, padding-bottom is 8px **/
/** Header is 64px, padding-top is 8px **/
.specs-list-container {
height: calc(100vh - 64px - 8px);
}
Expand Down
6 changes: 6 additions & 0 deletions packages/driver/src/cy/commands/screenshot.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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')

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// in the case that an element might change size on scroll
// we trigger a scroll event to ensure that all elements are
// at their final size before we calculate the total height
Expand All @@ -112,6 +116,8 @@ const scrollOverrides = (win, doc) => {
doc.body.style.overflowY = originalBodyOverflowY
}

document.querySelector('body')?.classList.remove('screenshot-scrolling')

return win.scrollTo(originalX, originalY)
}
}
Expand Down