fix: Reinstate snapshots on CI

Our paths were all messed up, let's fix that. I've also added some better debugging tooling to the job which should hopefully make debugging this easier in the future
PostHog · Feb 25, 2025 · 9ce1f96 · 9ce1f96
1 parent 75d20bb
commit 9ce1f96
Show file tree

Hide file tree

Showing 2 changed files with 100 additions and 45 deletions.
diff --git a/.github/workflows/storybook-chromatic.yml b/.github/workflows/storybook-chromatic.yml
@@ -132,16 +132,47 @@ jobs:
 
             - name: Serve Storybook in the background
               run: |
-                  retries=3
+                  retries=5
+                  max_timeout=30
+                  pnpm exec http-server common/storybook/dist --port 6006 --silent &
+                  server_pid=$!
+                  echo "Started http-server with PID: $server_pid"
+
+                  # Give the server a moment to start
+                  sleep 2
+
                   while [ $retries -gt 0 ]; do
-                      pnpm exec http-server common/storybook/dist --port 6006 --silent &
-                      if pnpm wait-on http://127.0.0.1:6006 --timeout 15; then
+                      echo "Checking if Storybook is available (retries left: $retries, timeout: ${max_timeout}s)..."
+                      if pnpm wait-on http://127.0.0.1:6006 --timeout $max_timeout; then
+                          echo "✅ Storybook is available at http://127.0.0.1:6006"
                           break
                       fi
                       retries=$((retries-1))
-                      echo "Failed to serve Storybook, retrying... ($retries retries left)"
+                      if [ $retries -gt 0 ]; then
+                          echo "⚠️ Failed to connect to Storybook, retrying... ($retries retries left)"
+                          # Check if server is still running
+                          if ! kill -0 $server_pid 2>/dev/null; then
+                              echo "❌ http-server process is no longer running, restarting it..."
+                              pnpm exec http-server common/storybook/dist --port 6006 --silent &
+                              server_pid=$!
+                              echo "Restarted http-server with PID: $server_pid"
+                              sleep 2
+                          fi
+                      fi
                   done
 
+                  if [ $retries -eq 0 ]; then
+                      echo "❌ Failed to serve Storybook after all retries"
+                      # Try to get some diagnostic information
+                      echo "Checking port 6006 status:"
+                      netstat -tuln | grep 6006 || echo "Port 6006 is not in use"
+                      echo "Checking http-server process:"
+                      ps aux | grep http-server || echo "No http-server process found"
+                      echo "Checking Storybook dist directory:"
+                      ls -la common/storybook/dist || echo "Storybook dist directory not found"
+                      exit 1
+                  fi
+
             - name: Run @storybook/test-runner
               env:
                   # Solving this bug by overriding $HOME: https://github.com/microsoft/playwright/issues/6500
@@ -159,13 +190,17 @@ jobs:
                   name: failure-screenshots-${{ matrix.browser }}-${{ matrix.shard }}
                   path: frontend/__snapshots__/__failures__/
 
+            - name: Configure global git diff log
+              run: git config --global --add safe.directory '*'
+
             - name: Count and optimize updated snapshots
               id: diff
               # Skip on forks
               if: github.event.pull_request.head.repo.full_name == github.repository
               run: |
-                  git config --global --add safe.directory '*' # Calm git down about file ownership
-                  git diff --name-status frontend/__snapshots__/ # For debugging
+                  echo "Current directory: $(pwd)"
+                  FRONTEND_DIFF_OUTPUT=$(git diff --name-status frontend/__snapshots__)
+                  echo "$FRONTEND_DIFF_OUTPUT"
                   ADDED=$(git diff --name-status frontend/__snapshots__/ | grep '^A' | wc -l)
                   MODIFIED=$(git diff --name-status frontend/__snapshots__/ | grep '^M' | wc -l)
                   DELETED=$(git diff --name-status frontend/__snapshots__/ | grep '^D' | wc -l)
@@ -195,6 +230,11 @@ jobs:
                     fi
                   fi
 
+                  echo "Snapshot changes:"
+                  echo "Added: $ADDED"
+                  echo "Modified: $MODIFIED" 
+                  echo "Deleted: $DELETED"
+                  echo "Total: $TOTAL"
                   echo "${{ matrix.browser }}-${{ matrix.shard }}-added=$ADDED" >> $GITHUB_OUTPUT
                   echo "${{ matrix.browser }}-${{ matrix.shard }}-modified=$MODIFIED" >> $GITHUB_OUTPUT
                   echo "${{ matrix.browser }}-${{ matrix.shard }}-deleted=$DELETED" >> $GITHUB_OUTPUT

diff --git a/common/storybook/.storybook/test-runner.ts b/common/storybook/.storybook/test-runner.ts
@@ -3,6 +3,7 @@ import { getStoryContext, TestRunnerConfig, TestContext } from '@storybook/test-
 import type { Locator, Page, LocatorScreenshotOptions } from '@playwright/test'
 import type { Mocks } from '~/mocks/utils'
 import { StoryContext } from '@storybook/csf'
+import path from 'path'
 
 const DEFAULT_VIEWPORT = { width: 1280, height: 720 }
 
@@ -63,7 +64,8 @@ const LOADER_SELECTORS = [
     '.Lettermark--unknown',
 ]
 
-const customSnapshotsDir = `${process.cwd()}/__snapshots__`
+const customSnapshotsDir = path.resolve(__dirname, '../../../frontend/__snapshots__')
+console.log("[test-runner] Storybook snapshots will be saved to", customSnapshotsDir)
 
 const JEST_TIMEOUT_MS = 15000
 const PLAYWRIGHT_TIMEOUT_MS = 10000 // Must be shorter than JEST_TIMEOUT_MS
@@ -76,25 +78,30 @@ module.exports = {
         jest.retryTimes(RETRY_TIMES, { logErrorsBeforeRetry: true })
         jest.setTimeout(JEST_TIMEOUT_MS)
     },
+
     async preVisit(page, context) {
         const storyContext = await getStoryContext(page, context)
         const viewport = storyContext.parameters?.testOptions?.viewport || DEFAULT_VIEWPORT
         await page.setViewportSize(viewport)
     },
+
     async postVisit(page, context) {
         ATTEMPT_COUNT_PER_ID[context.id] = (ATTEMPT_COUNT_PER_ID[context.id] || 0) + 1
         const storyContext = await getStoryContext(page, context)
         const viewport = storyContext.parameters?.testOptions?.viewport || DEFAULT_VIEWPORT
+
         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(viewport)
         }
+
         const browserContext = page.context()
         const { snapshotBrowsers = ['chromium'] } = storyContext.parameters?.testOptions ?? {}
 
@@ -115,66 +122,70 @@ async function expectStoryToMatchSnapshot(
     storyContext: StoryContext,
     browser: SupportedBrowserName
 ): Promise<void> {
-    const {
-        waitForLoadersToDisappear = true,
-        waitForSelector,
-        includeNavigationInSnapshot = false,
-    } = storyContext.parameters?.testOptions ?? {}
-
-    let check: (
-        page: Page,
-        context: TestContext,
-        browser: SupportedBrowserName,
-        theme: SnapshotTheme,
-        targetSelector?: string
-    ) => Promise<void>
-    if (storyContext.parameters?.layout === 'fullscreen') {
-        if (includeNavigationInSnapshot) {
-            check = expectStoryToMatchViewportSnapshot
-        } else {
-            check = expectStoryToMatchSceneSnapshot
-        }
-    } else {
-        check = expectStoryToMatchComponentSnapshot
-    }
-
     await waitForPageReady(page)
     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')
+
+    const {
+        waitForLoadersToDisappear = true,
+        waitForSelector,
+    } = storyContext.parameters?.testOptions ?? {}
+
     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: 3000 })
     }
+
     if (typeof waitForSelector === 'string') {
         await page.waitForSelector(waitForSelector)
     } else if (Array.isArray(waitForSelector)) {
         await Promise.all(waitForSelector.map((selector) => page.waitForSelector(selector)))
     }
 
-    // Snapshot light theme
-    await page.evaluate(() => {
-        document.body.setAttribute('theme', 'light')
-    })
-
-    await waitForPageReady(page)
-    await page.waitForFunction(() => Array.from(document.images).every((i: HTMLImageElement) => !!i.naturalWidth))
-    await page.waitForTimeout(2000)
+    // Snapshot both light and dark themes
+    await takeSnapshotWithTheme(page, context, browser, 'light', storyContext)
+    await takeSnapshotWithTheme(page, context, browser, 'dark', storyContext)
+}
 
-    await check(page, context, browser, 'light', storyContext.parameters?.testOptions?.snapshotTargetSelector)
 
-    // Snapshot dark theme
-    await page.evaluate(() => {
-        document.body.setAttribute('theme', 'dark')
-    })
+async function takeSnapshotWithTheme(page: Page, context: TestContext, browser: SupportedBrowserName, theme: SnapshotTheme, storyContext: StoryContext) {
+    // Set the right theme
+    await page.evaluate((theme: SnapshotTheme) => document.body.setAttribute('theme', theme), theme)
 
+    // Wait until we're sure we've finished loading everything
     await waitForPageReady(page)
     await page.waitForFunction(() => Array.from(document.images).every((i: HTMLImageElement) => !!i.naturalWidth))
     await page.waitForTimeout(100)
 
-    await check(page, context, browser, 'dark', storyContext.parameters?.testOptions?.snapshotTargetSelector)
+    // Do take the snapshot
+    await doTakeSnapshotWithTheme(page, context, browser, theme, storyContext)
+}
+
+async function doTakeSnapshotWithTheme(page: Page, context: TestContext, browser: SupportedBrowserName, theme: SnapshotTheme, storyContext: StoryContext) {
+    const { includeNavigationInSnapshot = false, snapshotTargetSelector } = storyContext.parameters?.testOptions ?? {}
+
+    // Figure out what's the right check function depending on the parameters
+    let check: (
+        page: Page,
+        context: TestContext,
+        browser: SupportedBrowserName,
+        theme: SnapshotTheme,
+        targetSelector?: string
+    ) => Promise<void>
+    if (storyContext.parameters?.layout === 'fullscreen') {
+        if (includeNavigationInSnapshot) {
+            check = expectStoryToMatchViewportSnapshot
+        } else {
+            check = expectStoryToMatchSceneSnapshot
+        }
+    } else {
+        check = expectStoryToMatchComponentSnapshot
+    }
+
+    await check(page, context, browser, theme, snapshotTargetSelector)
 }
 
 async function expectStoryToMatchViewportSnapshot(
@@ -210,6 +221,7 @@ async function expectStoryToMatchComponentSnapshot(
         if (!rootEl) {
             throw new Error('Could not find root element')
         }
+
         // If needed, expand the root element so that all popovers are visible in the screenshot
         document.querySelectorAll('.Popover').forEach((popover) => {
             const currentRootBoundingClientRect = rootEl.getBoundingClientRect()
@@ -246,13 +258,14 @@ async function expectLocatorToMatchStorySnapshot(
     if (browser !== 'chromium') {
         customSnapshotIdentifier += `--${browser}`
     }
+
     expect(image).toMatchImageSnapshot({
         customSnapshotsDir,
         customSnapshotIdentifier,
         // Compare structural similarity instead of raw pixels - reducing false positives
         // See https://github.com/americanexpress/jest-image-snapshot#recommendations-when-using-ssim-comparison
         comparisonMethod: 'ssim',
-        // 0.01 would be a 1% difference
+        // 0.01 is a 1% difference
         failureThreshold: 0.01,
         failureThresholdType: 'percent',
     })
@@ -265,8 +278,10 @@ async function expectLocatorToMatchStorySnapshot(
 async function waitForPageReady(page: Page): Promise<void> {
     await page.waitForLoadState('domcontentloaded')
     await page.waitForLoadState('load')
+
     if (process.env.CI) {
         await page.waitForLoadState('networkidle')
     }
+
     await page.evaluate(() => document.fonts.ready)
 }