-
Notifications
You must be signed in to change notification settings - Fork 887
[Chore] Stabilize VRT flakes, update snapshots and fix local mobile timeout #9795
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
base: main
Are you sure you want to change the base?
Changes from all commits
81972d0
306887f
53cb38f
4f351a0
506d9f2
3106d1a
0022972
bee2324
aa52c6b
dd8056c
0415751
9a27add
c23d500
4d0efdc
d1655eb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| const { execSync } = require('child_process'); | ||
| const { execSync, spawn, spawnSync } = require('child_process'); | ||
| const fs = require('fs'); | ||
| const net = require('net'); | ||
| const path = require('path'); | ||
| const yargs = require('yargs/yargs'); | ||
| const { hideBin } = require('yargs/helpers'); | ||
|
|
@@ -13,6 +14,9 @@ function findRepoRoot(start) { | |
| return start; | ||
| } | ||
|
|
||
| const PACKAGE_ROOT = path.resolve(__dirname, '..'); | ||
| const REPO_ROOT = findRepoRoot(PACKAGE_ROOT); | ||
|
|
||
| /** | ||
| * The test-runner is invoked once per variant. | ||
| * These names must match the keys of the `VARIANTS` map in `.storybook/vrt.ts`. | ||
|
|
@@ -35,12 +39,116 @@ const { argv } = yargs(hideBin(process.argv)) | |
| type: 'boolean', | ||
| description: | ||
| 'Run tests inside a Linux Docker container (matching CI) to generate Linux-compatible screenshots. Defaults to true locally, false in CI.', | ||
| }) | ||
| .option('static', { | ||
| type: 'boolean', | ||
| description: | ||
| 'Serve a static Storybook build (like CI) instead of the dev server, avoiding the networkidle stalls that make `waitForPageReady` time out locally under emulation. Defaults on locally; use `--no-static` for the dev server. Ignored when `--url` is set.', | ||
| }) | ||
| .option('build', { | ||
| type: 'boolean', | ||
| description: | ||
| 'Rebuild the static Storybook before running (default in `--static` mode, so runs reflect story changes). Use `--no-build` to reuse an existing `storybook-static` directory.', | ||
| }); | ||
|
|
||
| const isUpdate = argv._.includes('update'); | ||
| const extraArgs = argv._.filter((arg) => arg !== 'update'); | ||
| const isCI = Boolean(process.env.CI || process.env.BUILDKITE); | ||
| const useDocker = argv.docker !== undefined ? argv.docker : !isCI; | ||
| const useStatic = | ||
| !argv.url && (argv.static !== undefined ? argv.static : !isCI); | ||
| const forceBuild = argv.build !== undefined ? argv.build : true; | ||
| const STATIC_DIR = 'storybook-static'; | ||
| const STATIC_PORT = 6006; | ||
|
|
||
| const staticBuildExists = () => | ||
| fs.existsSync(path.join(PACKAGE_ROOT, STATIC_DIR)); | ||
|
|
||
| const isPortOpen = (port) => | ||
| new Promise((resolve) => { | ||
| const socket = net.createConnection({ host: '127.0.0.1', port }); | ||
| const finish = (isOpen) => { | ||
| socket.destroy(); | ||
| resolve(isOpen); | ||
| }; | ||
|
|
||
| socket.once('connect', () => finish(true)); | ||
| socket.once('error', () => finish(false)); | ||
| }); | ||
|
|
||
| const waitForPort = async (port, timeout = 30000) => { | ||
| const deadline = Date.now() + timeout; | ||
|
|
||
| while (Date.now() < deadline) { | ||
| if (await isPortOpen(port)) return; | ||
| await new Promise((resolve) => setTimeout(resolve, 500)); | ||
| } | ||
|
|
||
| throw new Error(`Timed out waiting for port ${port}`); | ||
| }; | ||
|
|
||
| const stopStaticServer = (server) => { | ||
| if (!server?.pid) return; | ||
|
|
||
| if (process.platform === 'win32') { | ||
| spawnSync( | ||
| 'taskkill', | ||
| ['/pid', server.pid.toString(), '/t', '/f'], | ||
| { stdio: 'ignore' } | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| process.kill(-server.pid); | ||
| } catch (error) { | ||
| if (error.code !== 'ESRCH') throw error; | ||
| } | ||
| }; | ||
|
|
||
| const startStaticServer = async () => { | ||
| if (await isPortOpen(STATIC_PORT)) { | ||
| throw new Error(`Port ${STATIC_PORT} is already in use`); | ||
| } | ||
|
|
||
| // `detached` lets cleanup terminate both yarn and http-server. | ||
| const server = spawn('yarn', ['serve-storybook'], { | ||
| stdio: 'inherit', | ||
| detached: true, | ||
| cwd: PACKAGE_ROOT, | ||
| }); | ||
|
|
||
| const serverFailed = new Promise((_, reject) => { | ||
| server.once('error', reject); | ||
| server.once('exit', (code) => { | ||
| reject(new Error(`Static Storybook server exited with code ${code}`)); | ||
| }); | ||
| }); | ||
|
|
||
| try { | ||
| await Promise.race([waitForPort(STATIC_PORT), serverFailed]); | ||
| return server; | ||
| } catch (error) { | ||
| stopStaticServer(server); | ||
| throw error; | ||
| } | ||
| }; | ||
|
|
||
| // Run every variant even when an earlier variant fails. | ||
| const runVariants = (runVariant) => { | ||
| let failed = false; | ||
|
|
||
| for (const variant of VARIANTS) { | ||
| console.log(`\n--- Variant: ${variant}`); | ||
| try { | ||
| runVariant(variant); | ||
| } catch { | ||
| failed = true; | ||
| } | ||
| } | ||
|
|
||
| return failed; | ||
| }; | ||
|
|
||
| if (useDocker) { | ||
| let playwrightVersion; | ||
|
|
@@ -52,29 +160,31 @@ if (useDocker) { | |
|
|
||
| const dockerImage = `mcr.microsoft.com/playwright:v${playwrightVersion}-jammy`; | ||
|
|
||
| // On macOS, Docker containers can't reach the host through localhost; use `host.docker.internal` instead. | ||
| // If no URL is provided, inject the default port with the correct hostname so the container can reach the | ||
| // Storybook dev server running on the host. | ||
| let url = argv.url; | ||
|
|
||
| if (process.platform === 'darwin') { | ||
| // Static mode serves inside the container over localhost, so this only | ||
| // applies to the dev server: Docker Desktop containers can't reach the host | ||
| // through localhost, so rewrite to `host.docker.internal`. | ||
| if ( | ||
| !useStatic && | ||
| (process.platform === 'darwin' || process.platform === 'win32') | ||
| ) { | ||
| url = (url || 'http://localhost:6006') | ||
| .replace(/\blocalhost\b/g, 'host.docker.internal') | ||
| .replace(/127\.0\.0\.1/g, 'host.docker.internal'); | ||
| } | ||
|
|
||
| const testStorybookArgs = [ | ||
| isUpdate && '--updateSnapshot', | ||
| url && `--url ${url}`, | ||
| !useStatic && url && `--url ${url}`, | ||
| ...extraArgs, | ||
| ].filter(Boolean); | ||
|
|
||
| // Mount the full monorepo root so yarn can resolve `workspace:*` dependencies | ||
| // across sibling packages (e.g. `@elastic/eui-theme-common`). | ||
| const repoRoot = findRepoRoot(__dirname); | ||
| const workspaceDir = path.relative(repoRoot, process.cwd()); | ||
| const workspaceDir = path.relative(REPO_ROOT, PACKAGE_ROOT); | ||
| const nodeVersion = fs | ||
| .readFileSync(path.join(repoRoot, '.nvmrc'), 'utf8') | ||
| .readFileSync(path.join(REPO_ROOT, '.nvmrc'), 'utf8') | ||
| .trim(); | ||
|
|
||
| const argsSuffix = testStorybookArgs.length | ||
|
|
@@ -96,63 +206,97 @@ if (useDocker) { | |
| const networkFlag = process.platform === 'linux' ? '--network=host ' : ''; | ||
|
|
||
| const dockerRun = (innerCmd) => | ||
| `docker run --rm -i --platform linux/amd64 ${networkFlag}-v "${repoRoot}:/work" -w /work ${dockerImage} bash -c ${JSON.stringify( | ||
| `docker run --rm -i --platform linux/amd64 ${networkFlag}-v "${REPO_ROOT}:/work" -w /work ${dockerImage} bash -c ${JSON.stringify( | ||
| innerCmd | ||
| )}`; | ||
|
|
||
| console.log(`Running visual regression tests in Docker (${dockerImage})`); | ||
|
|
||
| // Run each variant in its OWN container. Sharing a single container let the | ||
| // first variant leak Chromium processes/memory that starve the next one under | ||
| // amd64 emulation, tripping the per-test timeout on the second variant. | ||
| // `--testTimeout` adds headroom for the slower emulated environment. All | ||
| // variants run even if an earlier one fails, so we still generate every | ||
| // baseline/diff; a non-zero exit is propagated if any variant reported diffs. | ||
| // Build in the container, not on the host: the container's `yarn` swaps | ||
| // mounted `node_modules` binaries to Linux, breaking host builds. | ||
| if (useStatic && (forceBuild || !staticBuildExists())) { | ||
| console.log('Building static Storybook inside the container...'); | ||
| execSync(dockerRun(`set -e; ${setup}; yarn build-storybook`), { | ||
| stdio: 'inherit', | ||
| }); | ||
| } else if (useStatic) { | ||
| console.log(`Reusing existing ${STATIC_DIR}/ (--no-build)`); | ||
| } | ||
|
|
||
| let failed = false; | ||
| // Serve the build over localhost and wait for it to bind before testing; | ||
| // flat files keep `networkidle` settling instantly (unlike the dev server). | ||
| const staticServe = useStatic | ||
| ? `yarn serve-storybook & server_pid=$!; ` + | ||
| `for attempt in $(seq 1 60); do ` + | ||
| `if ! kill -0 "$server_pid" 2>/dev/null; then echo "Static Storybook server exited before startup"; wait "$server_pid"; exit 1; fi; ` + | ||
| `if (echo > /dev/tcp/127.0.0.1/${STATIC_PORT}) 2>/dev/null; then break; fi; ` + | ||
| `sleep 0.5; done; ` + | ||
| `if ! (echo > /dev/tcp/127.0.0.1/${STATIC_PORT}) 2>/dev/null; then echo "Timed out waiting for static Storybook on port ${STATIC_PORT}"; kill "$server_pid"; exit 1; fi; ` | ||
| : ''; | ||
|
|
||
| for (const variant of VARIANTS) { | ||
| console.log(`\n--- Variant: ${variant}`); | ||
| const innerCmd = `set -e; ${setup}; VRT_VARIANT=${variant} yarn test-storybook --testTimeout=30000${argsSuffix}`; | ||
| // `--maxWorkers`/`--testTimeout` add headroom for the slower emulated env. | ||
| const failed = runVariants((variant) => { | ||
| const innerCmd = `set -e; ${setup}; ${staticServe}VRT_VARIANT=${variant} yarn test-storybook --maxWorkers=2 --testTimeout=60000${argsSuffix}`; | ||
| execSync(dockerRun(innerCmd), { stdio: 'inherit' }); | ||
| }); | ||
|
|
||
| try { | ||
| execSync(dockerRun(innerCmd), { stdio: 'inherit' }); | ||
| } catch { | ||
| failed = true; | ||
| } | ||
| } | ||
| process.exit(failed ? 1 : 0); | ||
| } | ||
|
|
||
| // Safe-guard to ensure the browser is installed before running the tests | ||
| execSync('yarn playwright install chromium', { stdio: 'inherit' }); | ||
| const runNativeTests = async () => { | ||
| // Safe-guard to ensure the browser is installed before running the tests | ||
| execSync('yarn playwright install chromium', { | ||
| stdio: 'inherit', | ||
| cwd: PACKAGE_ROOT, | ||
| }); | ||
|
|
||
| console.log('Running visual regression tests'); | ||
| console.log('Running visual regression tests'); | ||
|
|
||
| const baseCmd = [ | ||
| 'yarn test-storybook', | ||
| isUpdate && '--updateSnapshot', | ||
| argv.url && `--url ${argv.url}`, | ||
| ...extraArgs, | ||
| ] | ||
| .filter(Boolean) | ||
| .join(' '); | ||
| let staticServer; | ||
|
|
||
| // Run each variant in its own test-runner process (clean slate per variant). | ||
| // Run all variants even if one fails, then exit non-zero if any reported diffs. | ||
| let failed = false; | ||
| if (useStatic) { | ||
| if (forceBuild || !staticBuildExists()) { | ||
| console.log('Building static Storybook (yarn build-storybook)...'); | ||
| execSync('yarn build-storybook', { | ||
| stdio: 'inherit', | ||
| cwd: PACKAGE_ROOT, | ||
| }); | ||
| } else { | ||
| console.log(`Reusing existing ${STATIC_DIR}/ (--no-build)`); | ||
| } | ||
|
|
||
| for (const variant of VARIANTS) { | ||
| console.log(`\n--- Variant: ${variant}`); | ||
| staticServer = await startStaticServer(); | ||
|
|
||
| try { | ||
| process.on('exit', () => stopStaticServer(staticServer)); | ||
| process.on('SIGINT', () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❓ Do we also need the same for |
||
| stopStaticServer(staticServer); | ||
| process.exit(1); | ||
| }); | ||
| } | ||
|
|
||
| const baseCmd = [ | ||
| 'yarn test-storybook', | ||
| !isCI && '--maxWorkers=2', | ||
| !isCI && '--testTimeout=60000', | ||
| isUpdate && '--updateSnapshot', | ||
| !useStatic && argv.url && `--url ${argv.url}`, | ||
| ...extraArgs, | ||
| ] | ||
| .filter(Boolean) | ||
| .join(' '); | ||
|
|
||
| const failed = runVariants((variant) => { | ||
| execSync(baseCmd, { | ||
| stdio: 'inherit', | ||
| env: { ...process.env, VRT_VARIANT: variant }, | ||
| cwd: PACKAGE_ROOT, | ||
| }); | ||
| } catch { | ||
| failed = true; | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| process.exit(failed ? 1 : 0); | ||
| }; | ||
|
|
||
| process.exit(failed ? 1 : 0); | ||
| runNativeTests().catch((error) => { | ||
| console.error(error.message); | ||
| process.exit(1); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,9 +35,12 @@ type Story = StoryObj<EuiBetaBadgeProps>; | |
| export const Playground: Story = { | ||
| args: { | ||
| label: 'Beta', | ||
| // 'middle' avoids the badge being cropped in VRT when there is no surrounding text to anchor baseline alignment | ||
| alignment: 'middle', | ||
| }, | ||
| render: (args: EuiBetaBadgeProps) => ( | ||
| <div css={{ display: 'inline-block' }}> | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: It might be worth adding a comment why the wrapper is added (for VRT). |
||
| <EuiBetaBadge {...args} /> | ||
| </div> | ||
| ), | ||
| }; | ||
|
|
||
| export const TitleAlignment: Story = { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -77,7 +77,7 @@ type User = { | |
|
|
||
| const users: User[] = []; | ||
|
|
||
| for (let i = 0; i < 75; i++) { | ||
| for (let i = 0; i < 15; i++) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🙏 |
||
| users.push({ | ||
| id: i + 1, | ||
| firstName: faker.person.firstName(), | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ Should the
serverFailedget a.catch()?