test(vrt): Don't clip scene snapshots (#19677)

* test(vrt): Don't clip scene snapshots

* Upgrade Playwright to 1.33

* Replace `excludeNavigationFromSnapshot` with `includeNavigationInSnapshot`

* Tune locator

* Fix `EmptyStates` flakiness

* Fix `main` fallback

* Try a different way of selecting `main` with fallback

* Remove last `excludeNavigationFromSnapshot`

* Use `.Navigation3000__scene` instead of `body`

* Restore `overflow: visible` on navigation in tests

* Fix top bar overlapping

* More anti-clipping

* Actually fix clipping

* Fix `layout` parameter

* Fix `.classList.add()` use

* Remove empty `testOptions`

* Tweak `waitForSelector`

* Extend loader timeout

* Update Surveys.stories.tsx

* Update `UserPaths`'s `waitForSelector`

* Print test errors

* Revert "Print test errors"

This reverts commit 71d305f.

* Capture whole scene in failure screenshot

* Fix `CommandBar` snapshotting

* Actually fix `CommandBar` snapshotting

* Also fix modals

* Force remount on snapshot retry

* Actually fix modal, sidebar, and paths snapshotting

* Fix observed flakiness

* Remove legacy theme from visual tests

* Update UI snapshots for `webkit` (2)

* Update UI snapshots for `chromium` (1)

* Update UI snapshots for `chromium` (2)

* Fix sizing of `Toolbar` stories

* Fix typing

* Don't render zero-width funnel bar

* Attempt to fix more flakiness

* Update UI snapshots for `chromium` (1)

* Update UI snapshots for `chromium` (2)

* Update UI snapshots for `chromium` (1)

* Update UI snapshots for `chromium` (1)

* Tweak selectors

* Just hide the damn bar

* Don't render grid layout without `gridWrapperWidth`

* Use container query instead of React hook in `LemonBanner`

* Update SavedInsights.stories.tsx

* Explicitly size `LemonBanner`

* Update Surveys.stories.tsx

* Update UI snapshots for `chromium` (1)

* Include navigation in side panel snapshots

* Update UI snapshots for `chromium` (1)

* Dispatch resize

* Force settings sections in snapshots

* Re-resize

* Stabilize settings

* Update UI snapshots for `chromium` (1)

* Update UI snapshots for `chromium` (1)

* Update UI snapshots for `chromium` (2)

* Update UI snapshots for `chromium` (2)

---------

Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>

.github/workflows/storybook-chromatic.yml

@@ -146,7 +146,7 @@ jobs:
                   VARIANT: ${{ github.event.pull_request.head.repo.full_name == github.repository && 'update' || 'verify' }}
                   STORYBOOK_SKIP_TAGS: 'test-skip,test-skip-${{ matrix.browser }}'
               run: |
-                  pnpm test:visual-regression:ci:$VARIANT --browsers ${{ matrix.browser }} --shard ${{ matrix.shard }}/$SHARD_COUNT
+                  pnpm test:visual:ci:$VARIANT --browsers ${{ matrix.browser }} --shard ${{ matrix.shard }}/$SHARD_COUNT
 
             - name: Archive failure screenshots
               if: ${{ failure() }}

.storybook/test-runner.ts

@@ -6,27 +6,27 @@ import { StoryContext } from '@storybook/csf'
 
 // 'firefox' is technically supported too, but as of June 2023 it has memory usage issues that make is unusable
 type SupportedBrowserName = 'chromium' | 'webkit'
-type SnapshotTheme = 'legacy' | 'light' | 'dark'
+type SnapshotTheme = 'light' | 'dark'
 
 // Extend Storybook interface `Parameters` with Chromatic parameters
 declare module '@storybook/types' {
     interface Parameters {
         options?: any
+        /** @default 'padded' */
         layout?: 'padded' | 'fullscreen' | 'centered'
         testOptions?: {
             /**
              * Whether we should wait for all loading indicators to disappear before taking a snapshot.
              * @default true
              */
             waitForLoadersToDisappear?: boolean
-            /** If set, we'll wait for the given selector to be satisfied. */
-            waitForSelector?: string
+            /** If set, we'll wait for the given selector (or all selectors, if multiple) to be satisfied. */
+            waitForSelector?: string | string[]
             /**
-             * Whether navigation (sidebar + topbar) should be excluded from the snapshot.
-             * Warning: Fails if enabled for stories in which navigation is not present.
+             * Whether navigation should be included in the snapshot. Only applies to `layout: 'fullscreen'` stories.
              * @default false
              */
-            excludeNavigationFromSnapshot?: boolean
+            includeNavigationInSnapshot?: boolean
             /**
              * The test will always run for all the browers, but snapshots are only taken in Chromium by default.
              * Override this to take snapshots in other browsers too.
@@ -48,13 +48,13 @@ declare module '@storybook/types' {
     }
 }
 
-const RETRY_TIMES = 3
+const RETRY_TIMES = 2
 const LOADER_SELECTORS = [
     '.ant-skeleton',
     '.Spinner',
     '.LemonSkeleton',
     '.LemonTableLoader',
-    '.Toastify__toast-container',
+    '.Toastify__toast',
     '[aria-busy="true"]',
     '.SessionRecordingPlayer--buffering',
     '.Lettermark--unknown',
@@ -65,13 +65,26 @@ const customSnapshotsDir = `${process.cwd()}/frontend/__snapshots__`
 const JEST_TIMEOUT_MS = 15000
 const PLAYWRIGHT_TIMEOUT_MS = 10000 // Must be shorter than JEST_TIMEOUT_MS
 
+const ATTEMPT_COUNT_PER_ID: Record<string, number> = {}
+
 module.exports = {
     setup() {
         expect.extend({ toMatchImageSnapshot })
         jest.retryTimes(RETRY_TIMES, { logErrorsBeforeRetry: true })
         jest.setTimeout(JEST_TIMEOUT_MS)
     },
     async postVisit(page, context) {
+        ATTEMPT_COUNT_PER_ID[context.id] = (ATTEMPT_COUNT_PER_ID[context.id] || 0) + 1
+        await page.evaluate(
+            ([retry, id]) => console.log(`[${id}] Attempt ${retry}`),
+            [ATTEMPT_COUNT_PER_ID[context.id], context.id]
+        )
+        if (ATTEMPT_COUNT_PER_ID[context.id] > 1) {
+            // When retrying, resize the viewport and then resize again to default,
+            // just in case the retry is due to a useResizeObserver fail
+            await page.setViewportSize({ width: 1920, height: 1080 })
+            await page.setViewportSize({ width: 1280, height: 720 })
+        }
         const browserContext = page.context()
         const storyContext = await getStoryContext(page, context)
         const { snapshotBrowsers = ['chromium'] } = storyContext.parameters?.testOptions ?? {}
@@ -96,7 +109,7 @@ async function expectStoryToMatchSnapshot(
     const {
         waitForLoadersToDisappear = true,
         waitForSelector,
-        excludeNavigationFromSnapshot = false,
+        includeNavigationInSnapshot = false,
     } = storyContext.parameters?.testOptions ?? {}
 
     let check: (
@@ -107,26 +120,29 @@ async function expectStoryToMatchSnapshot(
         targetSelector?: string
     ) => Promise<void>
     if (storyContext.parameters?.layout === 'fullscreen') {
-        if (excludeNavigationFromSnapshot) {
-            check = expectStoryToMatchSceneSnapshot
+        if (includeNavigationInSnapshot) {
+            check = expectStoryToMatchViewportSnapshot
         } else {
-            check = expectStoryToMatchFullPageSnapshot
+            check = expectStoryToMatchSceneSnapshot
         }
     } else {
         check = expectStoryToMatchComponentSnapshot
     }
 
     await waitForPageReady(page)
-    await page.evaluate(() => {
-        // Stop all animations for consistent snapshots
+    await page.evaluate((layout: string) => {
+        // Stop all animations for consistent snapshots, and adjust other styles
         document.body.classList.add('storybook-test-runner')
-    })
+        document.body.classList.add(`storybook-test-runner--${layout}`)
+    }, storyContext.parameters?.layout || 'padded')
     if (waitForLoadersToDisappear) {
         // The timeout is reduced so that we never allow toasts – they usually signify something wrong
-        await page.waitForSelector(LOADER_SELECTORS.join(','), { state: 'detached', timeout: 1000 })
+        await page.waitForSelector(LOADER_SELECTORS.join(','), { state: 'detached', timeout: 3000 })
     }
-    if (waitForSelector) {
+    if (typeof waitForSelector === 'string') {
         await page.waitForSelector(waitForSelector)
+    } else if (Array.isArray(waitForSelector)) {
+        await Promise.all(waitForSelector.map((selector) => page.waitForSelector(selector)))
     }
 
     await page.waitForTimeout(400) // Wait for effects to finish
@@ -151,7 +167,7 @@ async function expectStoryToMatchSnapshot(
     await check(page, context, browser, 'dark', storyContext.parameters?.testOptions?.snapshotTargetSelector)
 }
 
-async function expectStoryToMatchFullPageSnapshot(
+async function expectStoryToMatchViewportSnapshot(
     page: Page,
     context: TestContext,
     browser: SupportedBrowserName,
@@ -166,12 +182,10 @@ async function expectStoryToMatchSceneSnapshot(
     browser: SupportedBrowserName,
     theme: SnapshotTheme
 ): Promise<void> {
-    await page.evaluate(() => {
-        // The screenshot gets clipped by overflow hidden on .Navigation3000
-        document.querySelector('Navigation3000')?.setAttribute('style', 'overflow: visible;')
-    })
-
-    await expectLocatorToMatchStorySnapshot(page.locator('main'), context, browser, theme)
+    // If the `main` element isn't present, let's use `body` - this is needed in logged-out screens.
+    // We use .last(), because the order of selector matches is based on the order of elements in the DOM,
+    // and not the order of the selectors in the query.
+    await expectLocatorToMatchStorySnapshot(page.locator('body, main').last(), context, browser, theme)
 }
 
 async function expectStoryToMatchComponentSnapshot(
@@ -181,13 +195,11 @@ async function expectStoryToMatchComponentSnapshot(
     theme: SnapshotTheme,
     targetSelector: string = '#storybook-root'
 ): Promise<void> {
-    await page.evaluate((theme) => {
+    await page.evaluate(() => {
         const rootEl = document.getElementById('storybook-root')
         if (!rootEl) {
             throw new Error('Could not find root element')
         }
-        // Make the root element (which is the default screenshot reference) hug the component
-        rootEl.style.display = 'inline-block'
         // If needed, expand the root element so that all popovers are visible in the screenshot
         document.querySelectorAll('.Popover').forEach((popover) => {
             const currentRootBoundingClientRect = rootEl.getBoundingClientRect()
@@ -205,9 +217,7 @@ async function expectStoryToMatchComponentSnapshot(
                 rootEl.style.width = `${-popoverBoundingClientRect.left + currentRootBoundingClientRect.right}px`
             }
         })
-        // For legacy style, make the body transparent to take the screenshot without background
-        document.body.style.background = theme === 'legacy' ? 'transparent' : 'var(--bg-3000)'
-    }, theme)
+    })
 
     await expectLocatorToMatchStorySnapshot(page.locator(targetSelector), context, browser, theme, {
         omitBackground: true,
@@ -222,10 +232,7 @@ async function expectLocatorToMatchStorySnapshot(
     options?: LocatorScreenshotOptions
 ): Promise<void> {
     const image = await locator.screenshot({ ...options })
-    let customSnapshotIdentifier = context.id
-    if (theme !== 'legacy') {
-        customSnapshotIdentifier += `--${theme}`
-    }
+    let customSnapshotIdentifier = `${context.id}--${theme}`
     if (browser !== 'chromium') {
         customSnapshotIdentifier += `--${browser}`
     }