From 03f205c12ba7495e61d725f1f25dda08eff9fe85 Mon Sep 17 00:00:00 2001 From: Valentin Palkovic Date: Thu, 10 Oct 2024 12:00:32 +0200 Subject: [PATCH 1/5] Refactor test runner error handling and logging --- code/addons/test/src/node/boot-test-runner.ts | 4 +++- .../manager/components/sidebar/SidebarBottom.tsx | 6 +++--- .../manager/components/sidebar/TestingModule.tsx | 14 +++++++------- 3 files changed, 13 insertions(+), 11 deletions(-) diff --git a/code/addons/test/src/node/boot-test-runner.ts b/code/addons/test/src/node/boot-test-runner.ts index 08bbbe6b3e7c..bb9bdc5570ee 100644 --- a/code/addons/test/src/node/boot-test-runner.ts +++ b/code/addons/test/src/node/boot-test-runner.ts @@ -85,10 +85,12 @@ export const bootTestRunner = async (channel: Channel, initEvent?: string, initA resolve(); } else if (result.type === 'error') { killChild(); - log(`${result.message}: ${result.error}`); + const fullMessage = `${result.message}: ${result.error}` + log(fullMessage); if (attempt >= MAX_START_ATTEMPTS) { log(`Aborting test runner process after ${attempt} restart attempts`); + stderr.push(fullMessage); reject(); } else if (!aborted) { log(`Restarting test runner process (attempt ${attempt}/${MAX_START_ATTEMPTS})`); diff --git a/code/core/src/manager/components/sidebar/SidebarBottom.tsx b/code/core/src/manager/components/sidebar/SidebarBottom.tsx index 984d85b459c3..e5a8c6000e5d 100644 --- a/code/core/src/manager/components/sidebar/SidebarBottom.tsx +++ b/code/core/src/manager/components/sidebar/SidebarBottom.tsx @@ -33,6 +33,7 @@ const initialTestProviderState: TestProviderState = { running: false, watching: false, failed: false, + crashed: false, }; const filterNone: API_FilterFunction = () => true; @@ -129,6 +130,7 @@ export const SidebarBottomBase = ({ api, notifications = [], status = {} }: Side cancelling: false, running: true, failed: false, + crashed: false, }; setTestProviders((state) => ({ ...state, [id]: { ...state[id], ...startingState } })); api.experimental_updateStatus(id, (state = {}) => @@ -171,7 +173,7 @@ export const SidebarBottomBase = ({ api, notifications = [], status = {} }: Side useEffect(() => { const onCrashReport = ({ providerId, ...details }: { providerId: string }) => { - updateTestProvider(providerId, { details, failed: true }); + updateTestProvider(providerId, { details, crashed: true }); }; const onProgressReport = ({ providerId, ...payload }: TestingModuleProgressReportPayload) => { @@ -226,8 +228,6 @@ export const SidebarBottomBase = ({ api, notifications = [], status = {} }: Side ); }; -const TESTING_MODULE_ID = 'storybook-testing-module'; - export const SidebarBottom = () => { const api = useStorybookApi(); const { notifications, status } = useStorybookState(); diff --git a/code/core/src/manager/components/sidebar/TestingModule.tsx b/code/core/src/manager/components/sidebar/TestingModule.tsx index 3c8b473a0348..6ab7cac23141 100644 --- a/code/core/src/manager/components/sidebar/TestingModule.tsx +++ b/code/core/src/manager/components/sidebar/TestingModule.tsx @@ -24,7 +24,7 @@ const spin = keyframes({ '100%': { transform: 'rotate(360deg)' }, }); -const Outline = styled.div<{ failed: boolean; running: boolean }>(({ failed, running, theme }) => ({ +const Outline = styled.div<{ crashed: boolean; running: boolean }>(({ crashed, running, theme }) => ({ position: 'relative', lineHeight: '20px', width: '100%', @@ -33,7 +33,7 @@ const Outline = styled.div<{ failed: boolean; running: boolean }>(({ failed, run background: `var(--sb-sidebar-bottom-card-background, ${theme.background.content})`, borderRadius: `var(--sb-sidebar-bottom-card-border-radius, ${theme.appBorderRadius + 1}px)` as any, - boxShadow: `inset 0 0 0 1px ${failed && !running ? theme.color.negative : theme.appBorderColor}, var(--sb-sidebar-bottom-card-box-shadow, 0 1px 2px 0 rgba(0, 0, 0, 0.05), 0px -5px 20px 10px ${theme.background.app})`, + boxShadow: `inset 0 0 0 1px ${crashed && !running ? theme.color.negative : theme.appBorderColor}, var(--sb-sidebar-bottom-card-box-shadow, 0 1px 2px 0 rgba(0, 0, 0, 0.05), 0px -5px 20px 10px ${theme.background.app})`, transitionProperty: 'color, background-color, border-color, text-decoration-color, fill, stroke', transitionTimingFunction: 'cubic-bezier(0.4, 0, 0.2, 1)', transitionDuration: '0.15s', @@ -49,7 +49,7 @@ const Outline = styled.div<{ failed: boolean; running: boolean }>(({ failed, run height: 'max(100vw, 100vh)', width: 'max(100vw, 100vh)', animation: `${spin} 3s linear infinite`, - background: failed + background: crashed ? `conic-gradient(transparent 90deg, ${theme.color.orange} 150deg, ${theme.color.gold} 210deg, transparent 270deg)` : `conic-gradient(transparent 90deg, ${theme.color.secondary} 150deg, ${theme.color.seafoam} 210deg, transparent 270deg)`, opacity: 1, @@ -171,13 +171,13 @@ const DynamicInfo = ({ state }: { state: TestProviders[keyof TestProviders] }) = }, []); return ( - {state.title(state)} + {state.title(state)} {state.description(state)} ); }; -export interface TestingModuleProps { +interface TestingModuleProps { testProviders: TestProviders[keyof TestProviders][]; errorCount: number; errorsActive: boolean; @@ -216,11 +216,11 @@ export const TestingModule = ({ }; const running = testProviders.some((tp) => tp.running); - const failed = testProviders.some((tp) => tp.failed); + const crashed = testProviders.some((tp) => tp.crashed); const testing = testProviders.length > 0; return ( - 0}> + Date: Thu, 10 Oct 2024 17:30:53 +0200 Subject: [PATCH 2/5] Integrate GlobalErrorModal into testing UI module --- code/addons/test/src/manager.tsx | 86 +++++++-- code/addons/test/src/node/vitest-manager.ts | 6 + .../components/sidebar/SidebarBottom.tsx | 16 +- .../components/sidebar/TestingModule.tsx | 96 +++++----- code/core/src/types/modules/addons.ts | 1 + code/e2e-tests/util.ts | 8 - .../react/e2e-tests/component-testing.spec.ts | 164 +++++++++--------- .../react/package.json | 4 +- .../react/stories/AddonTest.stories.tsx | 29 +++- 9 files changed, 249 insertions(+), 161 deletions(-) diff --git a/code/addons/test/src/manager.tsx b/code/addons/test/src/manager.tsx index 74d6a78a690c..f56ad800f2b0 100644 --- a/code/addons/test/src/manager.tsx +++ b/code/addons/test/src/manager.tsx @@ -1,6 +1,7 @@ -import React, { useCallback } from 'react'; +import React, { useCallback, useEffect, useState } from 'react'; -import { AddonPanel, Badge, Spaced } from 'storybook/internal/components'; +import { AddonPanel, Badge, Link as LinkComponent, Spaced } from 'storybook/internal/components'; +import { TESTING_MODULE_RUN_ALL_REQUEST } from 'storybook/internal/core-events'; import type { Combo } from 'storybook/internal/manager-api'; import { Consumer, addons, types, useAddonState } from 'storybook/internal/manager-api'; import { @@ -11,6 +12,7 @@ import { } from 'storybook/internal/types'; import { Panel } from './Panel'; +import { GlobalErrorModal } from './components/GlobalErrorModal'; import { ADDON_ID, PANEL_ID, TEST_PROVIDER_ID } from './constants'; import type { TestResult } from './node/reporter'; @@ -56,29 +58,79 @@ export function getRelativeTimeString(date: Date): string { return rtf.format(Math.floor(delta / divisor), units[unitIndex]); } -addons.register(ADDON_ID, () => { +const RelativeTime = ({ timestamp }: { timestamp: Date }) => { + const [relativeTimeString, setRelativeTimeString] = useState(null); + + useEffect(() => { + if (timestamp) { + setRelativeTimeString(getRelativeTimeString(timestamp).replace(/^now$/, 'just now')); + + const interval = setInterval(() => { + setRelativeTimeString(getRelativeTimeString(timestamp).replace(/^now$/, 'just now')); + }, 10000); + + return () => clearInterval(interval); + } + }, [timestamp]); + + return relativeTimeString && `Ran ${relativeTimeString}`; +}; + +addons.register(ADDON_ID, (api) => { addons.add(TEST_PROVIDER_ID, { type: Addon_TypesEnum.experimental_TEST_PROVIDER, runnable: true, watchable: true, - title: ({ failed }) => (failed ? "Component tests didn't complete" : 'Component tests'), - description: ({ failed, running, watching, progress }) => { + title: ({ crashed }) => (crashed ? "Component tests didn't complete" : 'Component tests'), + description: ({ failed, running, watching, progress, crashed, details }) => { + const [isModalOpen, setIsModalOpen] = useState(false); + + let message: string | React.ReactNode = 'Not run'; + if (running) { - return progress + message = progress ? `Testing... ${progress.numPassedTests}/${progress.numTotalTests}` : 'Starting...'; + } else if (failed) { + message = 'Component tests failed'; + } else if (watching) { + message = 'Watching for file changes'; + } else if (progress?.finishedAt) { + message = ; + } else if (crashed) { + message = ( + <> + { + setIsModalOpen(true); + }} + > + View full error + + + ); } - if (failed) { - return 'Component tests failed'; - } - if (watching) { - return 'Watching for file changes'; - } - if (progress?.finishedAt) { - return `Ran ${getRelativeTimeString(progress.finishedAt).replace(/^now$/, 'just now')}`; - } - return 'Not run'; + + return ( + <> + {message} + { + setIsModalOpen(false); + }} + onRerun={() => { + setIsModalOpen(false); + api + .getChannel() + .emit(TESTING_MODULE_RUN_ALL_REQUEST, { providerId: TEST_PROVIDER_ID }); + }} + /> + + ); }, mapStatusUpdate: (state) => @@ -101,7 +153,7 @@ addons.register(ADDON_ID, () => { .filter(Boolean) ) ), - } as Addon_TestProviderType<{ testResults: TestResult[] }>); + } as Addon_TestProviderType<{ testResults: TestResult[]; message: string }>); addons.add(PANEL_ID, { type: types.PANEL, diff --git a/code/addons/test/src/node/vitest-manager.ts b/code/addons/test/src/node/vitest-manager.ts index bc2af6e0bf6b..2939d972c84e 100644 --- a/code/addons/test/src/node/vitest-manager.ts +++ b/code/addons/test/src/node/vitest-manager.ts @@ -37,6 +37,12 @@ export class VitestManager { }, }); + if (this.vitest) { + this.vitest.onCancel(() => { + // TODO: handle cancelation + }); + } + await this.vitest.init(); } diff --git a/code/core/src/manager/components/sidebar/SidebarBottom.tsx b/code/core/src/manager/components/sidebar/SidebarBottom.tsx index e5a8c6000e5d..e448b133af65 100644 --- a/code/core/src/manager/components/sidebar/SidebarBottom.tsx +++ b/code/core/src/manager/components/sidebar/SidebarBottom.tsx @@ -124,8 +124,8 @@ export const SidebarBottomBase = ({ api, notifications = [], status = {} }: Side [] ); - const onRunTests = useCallback( - (id: TestProviderId) => { + const clearState = useCallback( + ({ providerId: id }: { providerId: TestProviderId }) => { const startingState: Partial = { cancelling: false, running: true, @@ -136,6 +136,12 @@ export const SidebarBottomBase = ({ api, notifications = [], status = {} }: Side api.experimental_updateStatus(id, (state = {}) => Object.fromEntries(Object.keys(state).map((key) => [key, null])) ); + }, + [api] + ); + + const onRunTests = useCallback( + (id: TestProviderId) => { api.emit(TESTING_MODULE_RUN_ALL_REQUEST, { providerId: id }); }, [api] @@ -173,7 +179,7 @@ export const SidebarBottomBase = ({ api, notifications = [], status = {} }: Side useEffect(() => { const onCrashReport = ({ providerId, ...details }: { providerId: string }) => { - updateTestProvider(providerId, { details, crashed: true }); + updateTestProvider(providerId, { details, running: false, crashed: true }); }; const onProgressReport = ({ providerId, ...payload }: TestingModuleProgressReportPayload) => { @@ -192,13 +198,15 @@ export const SidebarBottomBase = ({ api, notifications = [], status = {} }: Side }; api.getChannel()?.on(TESTING_MODULE_CRASH_REPORT, onCrashReport); + api.getChannel()?.on(TESTING_MODULE_RUN_ALL_REQUEST, clearState); api.getChannel()?.on(TESTING_MODULE_PROGRESS_REPORT, onProgressReport); return () => { api.getChannel()?.off(TESTING_MODULE_CRASH_REPORT, onCrashReport); api.getChannel()?.off(TESTING_MODULE_PROGRESS_REPORT, onProgressReport); + api.getChannel()?.off(TESTING_MODULE_RUN_ALL_REQUEST, clearState); }; - }, [api, testProviders, updateTestProvider]); + }, [api, testProviders, updateTestProvider, clearState]); const testProvidersArray = Object.values(testProviders); if (!hasWarnings && !hasErrors && !testProvidersArray.length) { diff --git a/code/core/src/manager/components/sidebar/TestingModule.tsx b/code/core/src/manager/components/sidebar/TestingModule.tsx index 6ab7cac23141..240fb0f3c6d3 100644 --- a/code/core/src/manager/components/sidebar/TestingModule.tsx +++ b/code/core/src/manager/components/sidebar/TestingModule.tsx @@ -24,38 +24,41 @@ const spin = keyframes({ '100%': { transform: 'rotate(360deg)' }, }); -const Outline = styled.div<{ crashed: boolean; running: boolean }>(({ crashed, running, theme }) => ({ - position: 'relative', - lineHeight: '20px', - width: '100%', - padding: 1, - overflow: 'hidden', - background: `var(--sb-sidebar-bottom-card-background, ${theme.background.content})`, - borderRadius: - `var(--sb-sidebar-bottom-card-border-radius, ${theme.appBorderRadius + 1}px)` as any, - boxShadow: `inset 0 0 0 1px ${crashed && !running ? theme.color.negative : theme.appBorderColor}, var(--sb-sidebar-bottom-card-box-shadow, 0 1px 2px 0 rgba(0, 0, 0, 0.05), 0px -5px 20px 10px ${theme.background.app})`, - transitionProperty: 'color, background-color, border-color, text-decoration-color, fill, stroke', - transitionTimingFunction: 'cubic-bezier(0.4, 0, 0.2, 1)', - transitionDuration: '0.15s', +const Outline = styled.div<{ crashed: boolean; running: boolean }>( + ({ crashed, running, theme }) => ({ + position: 'relative', + lineHeight: '20px', + width: '100%', + padding: 1, + overflow: 'hidden', + background: `var(--sb-sidebar-bottom-card-background, ${theme.background.content})`, + borderRadius: + `var(--sb-sidebar-bottom-card-border-radius, ${theme.appBorderRadius + 1}px)` as any, + boxShadow: `inset 0 0 0 1px ${crashed && !running ? theme.color.negative : theme.appBorderColor}, var(--sb-sidebar-bottom-card-box-shadow, 0 1px 2px 0 rgba(0, 0, 0, 0.05), 0px -5px 20px 10px ${theme.background.app})`, + transitionProperty: + 'color, background-color, border-color, text-decoration-color, fill, stroke', + transitionTimingFunction: 'cubic-bezier(0.4, 0, 0.2, 1)', + transitionDuration: '0.15s', - '&:after': { - content: '""', - display: running ? 'block' : 'none', - position: 'absolute', - left: '50%', - top: '50%', - marginLeft: 'calc(max(100vw, 100vh) * -0.5)', - marginTop: 'calc(max(100vw, 100vh) * -0.5)', - height: 'max(100vw, 100vh)', - width: 'max(100vw, 100vh)', - animation: `${spin} 3s linear infinite`, - background: crashed - ? `conic-gradient(transparent 90deg, ${theme.color.orange} 150deg, ${theme.color.gold} 210deg, transparent 270deg)` - : `conic-gradient(transparent 90deg, ${theme.color.secondary} 150deg, ${theme.color.seafoam} 210deg, transparent 270deg)`, - opacity: 1, - willChange: 'auto', - }, -})); + '&:after': { + content: '""', + display: running ? 'block' : 'none', + position: 'absolute', + left: '50%', + top: '50%', + marginLeft: 'calc(max(100vw, 100vh) * -0.5)', + marginTop: 'calc(max(100vw, 100vh) * -0.5)', + height: 'max(100vw, 100vh)', + width: 'max(100vw, 100vh)', + animation: `${spin} 3s linear infinite`, + background: crashed + ? `conic-gradient(transparent 90deg, ${theme.color.orange} 150deg, ${theme.color.gold} 210deg, transparent 270deg)` + : `conic-gradient(transparent 90deg, ${theme.color.secondary} 150deg, ${theme.color.seafoam} 210deg, transparent 270deg)`, + opacity: 1, + willChange: 'auto', + }, + }) +); const Card = styled.div(({ theme }) => ({ position: 'relative', @@ -153,26 +156,28 @@ const Actions = styled.div({ gap: 6, }); -const Title = styled.div<{ failed?: boolean }>(({ failed, theme }) => ({ - fontSize: theme.typography.size.s2, - fontWeight: failed ? 'bold' : 'normal', +const TitleWrapper = styled.div<{ crashed?: boolean }>(({ crashed, theme }) => ({ + fontSize: theme.typography.size.s1, + fontWeight: crashed ? 'bold' : 'normal', + color: crashed ? theme.color.negativeText : theme.color.defaultText, })); -const Description = styled.div(({ theme }) => ({ +const DescriptionWrapper = styled.div(({ theme }) => ({ fontSize: theme.typography.size.s1, color: theme.barTextColor, })); const DynamicInfo = ({ state }: { state: TestProviders[keyof TestProviders] }) => { - const [iterator, setIterator] = useState(0); - useEffect(() => { - const interval = setInterval(() => setIterator((i) => i + 1), 10000); - return () => clearInterval(interval); - }, []); + const Description = state.description; + const Title = state.title; return ( - - {state.title(state)} - {state.description(state)} + + + + </TitleWrapper> + <DescriptionWrapper> + <Description {...state} /> + </DescriptionWrapper> </Info> ); }; @@ -220,7 +225,7 @@ export const TestingModule = ({ const testing = testProviders.length > 0; return ( - <Outline running={running} failed={crashed}> + <Outline running={running} crashed={crashed}> <Card> <Collapsible style={{ @@ -240,6 +245,7 @@ export const TestingModule = ({ padding="small" active={state.watching} onClick={() => onSetWatchMode(state.id, !state.watching)} + disabled={state.crashed} > <EyeIcon /> </Button> @@ -262,7 +268,7 @@ export const TestingModule = ({ variant="ghost" padding="small" onClick={() => onRunTests(state.id)} - disabled={state.running} + disabled={state.crashed || state.running} > <PlayHollowIcon /> </Button> diff --git a/code/core/src/types/modules/addons.ts b/code/core/src/types/modules/addons.ts index 24101a1af16a..3c874a6185f1 100644 --- a/code/core/src/types/modules/addons.ts +++ b/code/core/src/types/modules/addons.ts @@ -487,6 +487,7 @@ export type Addon_TestProviderState<Details extends { [key: string]: any } = Non running: boolean; watching: boolean; failed: boolean; + crashed: boolean; }; type Addon_TypeBaseNames = Exclude< diff --git a/code/e2e-tests/util.ts b/code/e2e-tests/util.ts index de959a656b0c..04333e039da4 100644 --- a/code/e2e-tests/util.ts +++ b/code/e2e-tests/util.ts @@ -141,12 +141,4 @@ export class SbPage { getCanvasBodyElement() { return this.previewIframe().locator('body'); } - - async emitChannelEvent(event: string, ...args: any[]) { - return this.page.evaluate( - ([channelEvent, channelArgs]) => - window.__STORYBOOK_ADDONS_CHANNEL__.emit(channelEvent, ...channelArgs), - [event, args] - ); - } } diff --git a/test-storybooks/portable-stories-kitchen-sink/react/e2e-tests/component-testing.spec.ts b/test-storybooks/portable-stories-kitchen-sink/react/e2e-tests/component-testing.spec.ts index c7665c5aca19..9a919e235e69 100644 --- a/test-storybooks/portable-stories-kitchen-sink/react/e2e-tests/component-testing.spec.ts +++ b/test-storybooks/portable-stories-kitchen-sink/react/e2e-tests/component-testing.spec.ts @@ -1,14 +1,17 @@ -import { promises as fs } from 'node:fs'; -import path from 'node:path'; +import { promises as fs } from "node:fs"; +import path from "node:path"; -import { TESTING_MODULE_RUN_PROGRESS_RESPONSE } from 'storybook/internal/core-events'; -import { expect, test } from '@playwright/test'; +import { expect, test } from "@playwright/test"; -import { SbPage } from '../../../../code/e2e-tests/util'; +import { SbPage } from "../../../../code/e2e-tests/util"; -const storybookUrl = 'http://localhost:6006'; -const testStoryPath = path.resolve(__dirname, '..', 'stories/AddonTest.stories.tsx'); +const storybookUrl = "http://localhost:6006"; +const testStoryPath = path.resolve( + __dirname, + "..", + "stories/AddonTest.stories.tsx" +); const setForceFailureFlag = async (value: boolean) => { // Read the story file content asynchronously @@ -18,14 +21,17 @@ const setForceFailureFlag = async (value: boolean) => { const forceFailureRegex = /forceFailure:\s*(true|false)/; // Replace the value of 'forceFailure' with the new value - const updatedContent = storyContent.replace(forceFailureRegex, `forceFailure: ${value}`); + const updatedContent = storyContent.replace( + forceFailureRegex, + `forceFailure: ${value}` + ); // Write the updated content back to the file asynchronously await fs.writeFile(testStoryPath, updatedContent); }; -test.describe('component testing', () => { - test.describe.configure({ mode: 'serial' }); +test.describe("component testing", () => { + test.describe.configure({ mode: "serial" }); test.beforeEach(async ({ page }) => { const sbPage = new SbPage(page, expect); @@ -34,122 +40,103 @@ test.describe('component testing', () => { await sbPage.waitUntilLoaded(); }); - test('should show discrepancy between test results', async ({ page, browserName }) => { - test.skip(browserName !== 'chromium', `Skipping tests for ${browserName}`); + test("should show discrepancy between test results", async ({ + page, + browserName, + }) => { + test.skip(browserName !== "chromium", `Skipping tests for ${browserName}`); await setForceFailureFlag(true); - // Emit a channel event so that statuses in the sidebar can be mocked. - // leave the full mock object as it might be useful in the future (like when we display amount, timing, etc) - const mockData = { - status: 'success', - payload: { - numFailedTests: 1, - numPassedTests: 2, - numPendingTests: 0, - numTotalTests: 3, - testResults: [ - { - results: [ - { - status: 'failed', - duration: 7, - failureMessages: [ - 'Error: Expected failure\n at play (http://localhost:5174/Users/yannbraga/open-source/storybook/test-storybooks/portable-stories-kitchen-sink/react/stories/AddonTest.stories.tsx?import&browserv=1728406664275:20:13)\n at runStory (http://localhost:5174/node_modules/.vite/deps/@storybook_experimental-addon-test_internal_test-utils.js?v=d915d166:9220:11)\n at async http://localhost:5174/node_modules/.vite/deps/@storybook_experimental-addon-test_internal_test-utils.js?v=d915d166:11524:5\n at async runTest (http://localhost:5174/node_modules/@vitest/runner/dist/index.js?v=d915d166:969:11)\n at async runSuite (http://localhost:5174/node_modules/@vitest/runner/dist/index.js?v=d915d166:1125:15)\n at async runFiles (http://localhost:5174/node_modules/@vitest/runner/dist/index.js?v=d915d166:1182:5)\n at async startTests (http://localhost:5174/node_modules/@vitest/runner/dist/index.js?v=d915d166:1191:3)\n at async executeTests (http://localhost:5174/__vitest_browser__/tester-C7y_vb57.js:11959:9)', - ], - storyId: 'addons-test--expected-success', - }, - { - status: 'passed', - duration: 1, - storyId: 'addons-test--expected-failure', - }, - { - status: 'passed', - duration: 803, - storyId: 'addons-test--long-running', - }, - ], - startTime: 1728406664407, - endTime: 1728406665218, - status: 'failed', - }, - ], - success: true, - progress: 0, - startTime: 1728406628146, - }, - providerId: 'storybook/test/test-provider', - }; - const sbPage = new SbPage(page, expect); - await sbPage.emitChannelEvent(TESTING_MODULE_RUN_PROGRESS_RESPONSE, mockData); - await sbPage.navigateToStory('addons/test', 'Expected Failure'); + await sbPage.navigateToStory("addons/test", "Mismatch Failure"); // For whatever reason, sometimes it takes longer for the story to load - const storyElement = sbPage.getCanvasBodyElement().getByRole('button', { name: 'test' }); + const storyElement = sbPage + .getCanvasBodyElement() + .getByRole("button", { name: "test" }); await expect(storyElement).toBeVisible({ timeout: 10000 }); - await sbPage.viewAddonPanel('Component Tests'); + await sbPage.viewAddonPanel("Component Tests"); // For whatever reason, when visiting a story sometimes the story element is collapsed and that causes flake - const testStoryElement = await page.getByRole('button', { - name: 'Test', + const testStoryElement = await page.getByRole("button", { + name: "Test", exact: true, }); - if ((await testStoryElement.getAttribute('aria-expanded')) !== 'true') { + if ((await testStoryElement.getAttribute("aria-expanded")) !== "true") { testStoryElement.click(); } + // TODO: This is just temporary, the UI will be different + await page.locator("#addons").getByRole("button").nth(2).click(); + // Assert discrepancy: CLI pass + Browser fail const failingStoryElement = page.locator( - '[data-item-id="addons-test--expected-failure"] [role="status"]' + '[data-item-id="addons-test--mismatch-failure"] [role="status"]' + ); + await expect(failingStoryElement).toHaveAttribute( + "aria-label", + "Test status: success" ); - await expect(failingStoryElement).toHaveAttribute('aria-label', 'Test status: success'); await expect(sbPage.panelContent()).toContainText( /This component test passed in CLI, but the tests failed in this browser./ ); // Assert discrepancy: CLI fail + Browser pass - await sbPage.navigateToStory('addons/test', 'Expected Success'); + await sbPage.navigateToStory("addons/test", "Mismatch Success"); const successfulStoryElement = page.locator( - '[data-item-id="addons-test--expected-success"] [role="status"]' + '[data-item-id="addons-test--mismatch-success"] [role="status"]' + ); + await expect(successfulStoryElement).toHaveAttribute( + "aria-label", + "Test status: error" ); - await expect(successfulStoryElement).toHaveAttribute('aria-label', 'Test status: error'); await expect(sbPage.panelContent()).toContainText( /This component test passed in this browser, but the tests failed in CLI/ ); }); - test('should execute tests via testing module UI', async ({ page, browserName }) => { - test.skip(browserName !== 'chromium', `Skipping tests for ${browserName}`); + test("should execute tests via testing module UI", async ({ + page, + browserName, + }) => { + test.skip(browserName !== "chromium", `Skipping tests for ${browserName}`); await setForceFailureFlag(true); const sbPage = new SbPage(page, expect); - await sbPage.navigateToStory('addons/test', 'Expected Failure'); + await sbPage.navigateToStory("addons/test", "Expected Failure"); // For whatever reason, sometimes it takes longer for the story to load - const storyElement = sbPage.getCanvasBodyElement().getByRole('button', { name: 'test' }); + const storyElement = sbPage + .getCanvasBodyElement() + .getByRole("button", { name: "test" }); await expect(storyElement).toBeVisible({ timeout: 10000 }); // TODO: This is just temporary, the UI will be different - await page.locator('#addons').getByRole('button').nth(2).click(); + await page.locator("#addons").getByRole("button").nth(2).click(); // Wait for test results to appear - const errorFilter = page.getByLabel('Show errors'); + const errorFilter = page.getByLabel("Show errors"); await expect(errorFilter).toBeVisible({ timeout: 30000 }); // Assert for expected success const successfulStoryElement = page.locator( '[data-item-id="addons-test--expected-success"] [role="status"]' ); - await expect(successfulStoryElement).toHaveAttribute('aria-label', 'Test status: success'); + await expect(successfulStoryElement).toHaveAttribute( + "aria-label", + "Test status: success" + ); // Assert for expected failure const failingStoryElement = page.locator( '[data-item-id="addons-test--expected-failure"] [role="status"]' ); - await expect(failingStoryElement).toHaveAttribute('aria-label', 'Test status: error'); + await expect(failingStoryElement).toHaveAttribute( + "aria-label", + "Test status: error" + ); // Assert that filter works as intended await errorFilter.click(); @@ -160,18 +147,23 @@ test.describe('component testing', () => { await expect(sidebarItems).toHaveCount(1); }); - test('should execute tests via testing module UI watch mode', async ({ page, browserName }) => { - test.skip(browserName !== 'chromium', `Skipping tests for ${browserName}`); + test("should execute tests via testing module UI watch mode", async ({ + page, + browserName, + }) => { + test.skip(browserName !== "chromium", `Skipping tests for ${browserName}`); await setForceFailureFlag(false); const sbPage = new SbPage(page, expect); - await sbPage.navigateToStory('addons/test', 'Expected Failure'); + await sbPage.navigateToStory("addons/test", "Expected Failure"); // For whatever reason, sometimes it takes longer for the story to load - const storyElement = sbPage.getCanvasBodyElement().getByRole('button', { name: 'test' }); + const storyElement = sbPage + .getCanvasBodyElement() + .getByRole("button", { name: "test" }); await expect(storyElement).toBeVisible({ timeout: 10000 }); - await page.getByLabel('Toggle watch mode').click(); + await page.getByLabel("Toggle watch mode").click(); // We shouldn't have to do an arbitrary wait, but because there is no UI for loading state yet, we have to await page.waitForTimeout(8000); @@ -179,20 +171,26 @@ test.describe('component testing', () => { await setForceFailureFlag(true); // Wait for test results to appear - const errorFilter = page.getByLabel('Show errors'); + const errorFilter = page.getByLabel("Show errors"); await expect(errorFilter).toBeVisible({ timeout: 30000 }); // Assert for expected success const successfulStoryElement = page.locator( '[data-item-id="addons-test--expected-success"] [role="status"]' ); - await expect(successfulStoryElement).toHaveAttribute('aria-label', 'Test status: success'); + await expect(successfulStoryElement).toHaveAttribute( + "aria-label", + "Test status: success" + ); // Assert for expected failure const failingStoryElement = page.locator( '[data-item-id="addons-test--expected-failure"] [role="status"]' ); - await expect(failingStoryElement).toHaveAttribute('aria-label', 'Test status: error'); + await expect(failingStoryElement).toHaveAttribute( + "aria-label", + "Test status: error" + ); // Assert that filter works as intended await errorFilter.click(); diff --git a/test-storybooks/portable-stories-kitchen-sink/react/package.json b/test-storybooks/portable-stories-kitchen-sink/react/package.json index 4c434abb9907..7932a5375e28 100644 --- a/test-storybooks/portable-stories-kitchen-sink/react/package.json +++ b/test-storybooks/portable-stories-kitchen-sink/react/package.json @@ -13,7 +13,7 @@ "playwright-e2e": "playwright test -c playwright-e2e.config.ts", "preview": "vite preview", "storybook": "storybook dev -p 6006", - "vitest": "vitest" + "vitest": "echo 'not running'" }, "resolutions": { "@playwright/test": "1.46.0", @@ -124,4 +124,4 @@ "vite": "^5.1.1", "vitest": "^2.1.1" } -} +} \ No newline at end of file diff --git a/test-storybooks/portable-stories-kitchen-sink/react/stories/AddonTest.stories.tsx b/test-storybooks/portable-stories-kitchen-sink/react/stories/AddonTest.stories.tsx index 4d2e39a6260b..e6579f3c72d6 100644 --- a/test-storybooks/portable-stories-kitchen-sink/react/stories/AddonTest.stories.tsx +++ b/test-storybooks/portable-stories-kitchen-sink/react/stories/AddonTest.stories.tsx @@ -1,6 +1,11 @@ import { instrument } from '@storybook/instrumenter' import type { StoryAnnotations } from 'storybook/internal/types'; +declare global { + // eslint-disable-next-line no-var, @typescript-eslint/naming-convention + var __vitest_browser__: boolean; +} + const Component = () => <button>test</button> export default { @@ -16,9 +21,9 @@ export const ExpectedFailure = { args: { forceFailure: false, }, - play: async (context) => { + play: async ({ args }) => { await pass(); - if (context.args.forceFailure) { + if(args.forceFailure) { throw new Error('Expected failure'); } } @@ -33,3 +38,23 @@ export const ExpectedSuccess = { export const LongRunning = { loaders: [async () => new Promise((resolve) => setTimeout(resolve, 800))], } satisfies StoryAnnotations; + +// Tests will pass in browser, but fail in CLI +export const MismatchFailure = { + play: async () => { + await pass(); + if(!globalThis.__vitest_browser__) { + throw new Error('Expected failure'); + } + } +} satisfies StoryAnnotations; + +// Tests will fail in browser, but pass in CLI +export const MismatchSuccess = { + play: async () => { + await pass(); + if(globalThis.__vitest_browser__) { + throw new Error('Unexpected success'); + } + } +} satisfies StoryAnnotations; \ No newline at end of file From 2356f454378698869feb311a59962b0ae6e59a76 Mon Sep 17 00:00:00 2001 From: Yann Braga <yannbf@gmail.com> Date: Thu, 10 Oct 2024 17:49:44 +0200 Subject: [PATCH 3/5] Refactor test runner error handling and logging --- code/addons/test/package.json | 2 +- code/addons/test/src/node/boot-test-runner.ts | 33 +++++++++---------- code/addons/test/src/node/reporter.ts | 24 +++++++++----- code/addons/test/src/node/vitest-manager.ts | 2 +- code/yarn.lock | 2 +- 5 files changed, 35 insertions(+), 28 deletions(-) diff --git a/code/addons/test/package.json b/code/addons/test/package.json index aae490900335..d8f7f4dba9a2 100644 --- a/code/addons/test/package.json +++ b/code/addons/test/package.json @@ -93,10 +93,10 @@ "@vitest/runner": "^2.1.1", "ansi-to-html": "^0.7.2", "boxen": "^8.0.1", + "es-toolkit": "^1.22.0", "execa": "^8.0.1", "find-up": "^7.0.0", "formik": "^2.2.9", - "lodash": "^4.17.21", "picocolors": "^1.1.0", "react": "^18.2.0", "react-dom": "^18.2.0", diff --git a/code/addons/test/src/node/boot-test-runner.ts b/code/addons/test/src/node/boot-test-runner.ts index bb9bdc5570ee..26fdf0d733ee 100644 --- a/code/addons/test/src/node/boot-test-runner.ts +++ b/code/addons/test/src/node/boot-test-runner.ts @@ -11,12 +11,12 @@ import { type TestingModuleCrashReportPayload, } from 'storybook/internal/core-events'; +// eslint-disable-next-line depend/ban-dependencies import { execaNode } from 'execa'; import { TEST_PROVIDER_ID } from '../constants'; import { log } from '../logger'; -const MAX_START_ATTEMPTS = 3; const MAX_START_TIME = 8000; // This path is a bit confusing, but essentially `boot-test-runner` gets bundled into the preset bundle @@ -24,7 +24,6 @@ const MAX_START_TIME = 8000; const vitestModulePath = join(__dirname, 'node', 'vitest.mjs'); export const bootTestRunner = async (channel: Channel, initEvent?: string, initArgs?: any[]) => { - let aborted = false; let child: null | ChildProcess; let stderr: string[] = []; @@ -55,7 +54,7 @@ export const bootTestRunner = async (channel: Channel, initEvent?: string, initA process.on('SIGINT', () => exit(0)); process.on('SIGTERM', () => exit(0)); - const startChildProcess = (attempt = 1) => + const startChildProcess = () => new Promise<void>((resolve, reject) => { child = execaNode(vitestModulePath); stderr = []; @@ -85,17 +84,11 @@ export const bootTestRunner = async (channel: Channel, initEvent?: string, initA resolve(); } else if (result.type === 'error') { killChild(); - const fullMessage = `${result.message}: ${result.error}` - log(fullMessage); - - if (attempt >= MAX_START_ATTEMPTS) { - log(`Aborting test runner process after ${attempt} restart attempts`); - stderr.push(fullMessage); - reject(); - } else if (!aborted) { - log(`Restarting test runner process (attempt ${attempt}/${MAX_START_ATTEMPTS})`); - setTimeout(() => startChildProcess(attempt + 1).then(resolve, reject), 1000); - } + + channel.emit(TESTING_MODULE_CRASH_REPORT, { + providerId: TEST_PROVIDER_ID, + message: stderr.join('\n'), + } as TestingModuleCrashReportPayload); } else { channel.emit(result.type, ...result.args); } @@ -103,14 +96,20 @@ export const bootTestRunner = async (channel: Channel, initEvent?: string, initA }); const timeout = new Promise((_, reject) => - setTimeout(reject, MAX_START_TIME, new Error('Aborting test runner process due to timeout')) + setTimeout( + reject, + MAX_START_TIME, + // eslint-disable-next-line local-rules/no-uncategorized-errors + new Error( + `Aborting test runner process because it took longer than ${MAX_START_TIME / 1000} seconds to start.` + ) + ) ); await Promise.race([startChildProcess(), timeout]).catch((e) => { - aborted = true; channel.emit(TESTING_MODULE_CRASH_REPORT, { providerId: TEST_PROVIDER_ID, - message: stderr.join('\n'), + message: String(e), } as TestingModuleCrashReportPayload); throw e; }); diff --git a/code/addons/test/src/node/reporter.ts b/code/addons/test/src/node/reporter.ts index 468fd67396df..29084deca5f4 100644 --- a/code/addons/test/src/node/reporter.ts +++ b/code/addons/test/src/node/reporter.ts @@ -15,8 +15,7 @@ import type { Suite } from '@vitest/runner'; // functions from the `@vitest/runner` package. It is not complex and does not have // any significant dependencies. import { getTests } from '@vitest/runner/utils'; -// @ts-expect-error we will very soon replace this library with es-toolkit -import throttle from 'lodash/throttle.js'; +import { throttle } from 'es-toolkit'; import { TEST_PROVIDER_ID } from '../constants'; import type { TestManager } from './test-manager'; @@ -64,7 +63,6 @@ export class StorybookReporter implements Reporter { sendReport: (payload: TestingModuleProgressReportPayload) => void; constructor(private testManager: TestManager) { - // @ts-expect-error we will very soon replace this library with es-toolkit this.sendReport = throttle((payload) => this.testManager.sendProgressReport(payload), 200); } @@ -187,11 +185,21 @@ export class StorybookReporter implements Reporter { } async onFinished() { - this.sendReport({ - providerId: TEST_PROVIDER_ID, - status: 'success', - ...this.getProgressReport(new Date()), - }); + const unhandledErrors = this.ctx.state.getUnhandledErrors(); + + if (unhandledErrors?.length) { + this.testManager.reportFatalError( + `Vitest caught ${unhandledErrors.length} unhandled error${unhandledErrors?.length > 1 ? 's' : ''} during the test run.`, + unhandledErrors[0] + ); + } else { + this.sendReport({ + providerId: TEST_PROVIDER_ID, + status: 'success', + ...this.getProgressReport(new Date()), + }); + } + this.clearVitestState(); } } diff --git a/code/addons/test/src/node/vitest-manager.ts b/code/addons/test/src/node/vitest-manager.ts index 2939d972c84e..5630e31657a8 100644 --- a/code/addons/test/src/node/vitest-manager.ts +++ b/code/addons/test/src/node/vitest-manager.ts @@ -23,7 +23,7 @@ export class VitestManager { this.vitest = await createVitest('test', { watch: watchMode, - passWithNoTests: true, + passWithNoTests: false, changed: watchMode, // TODO: // Do we want to enable Vite's default reporter? diff --git a/code/yarn.lock b/code/yarn.lock index f8b85275e999..70157510a949 100644 --- a/code/yarn.lock +++ b/code/yarn.lock @@ -6243,10 +6243,10 @@ __metadata: "@vitest/runner": "npm:^2.1.1" ansi-to-html: "npm:^0.7.2" boxen: "npm:^8.0.1" + es-toolkit: "npm:^1.22.0" execa: "npm:^8.0.1" find-up: "npm:^7.0.0" formik: "npm:^2.2.9" - lodash: "npm:^4.17.21" picocolors: "npm:^1.1.0" polished: "npm:^4.2.2" react: "npm:^18.2.0" From 11a0891f0b559b54fc7843b180bc5328fd04b544 Mon Sep 17 00:00:00 2001 From: Yann Braga <yannbf@gmail.com> Date: Thu, 10 Oct 2024 17:50:35 +0200 Subject: [PATCH 4/5] fix test --- code/addons/test/src/node/test-manager.test.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/code/addons/test/src/node/test-manager.test.ts b/code/addons/test/src/node/test-manager.test.ts index b5d6ecab179e..415c9bbc7bb6 100644 --- a/code/addons/test/src/node/test-manager.test.ts +++ b/code/addons/test/src/node/test-manager.test.ts @@ -12,6 +12,7 @@ const vitest = vi.hoisted(() => ({ projects: [{}], init: vi.fn(), close: vi.fn(), + onCancel: vi.fn(), runFiles: vi.fn(), cancelCurrentRun: vi.fn(), globTestSpecs: vi.fn(), From 2eb23c8e30c9a414978655527a32faa405140ab8 Mon Sep 17 00:00:00 2001 From: Yann Braga <yannbf@gmail.com> Date: Thu, 10 Oct 2024 18:03:22 +0200 Subject: [PATCH 5/5] remove test --- code/addons/test/src/node/boot-test-runner.test.ts | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/code/addons/test/src/node/boot-test-runner.test.ts b/code/addons/test/src/node/boot-test-runner.test.ts index f1d993a8a319..bb875771ff10 100644 --- a/code/addons/test/src/node/boot-test-runner.test.ts +++ b/code/addons/test/src/node/boot-test-runner.test.ts @@ -10,6 +10,7 @@ import { TESTING_MODULE_WATCH_MODE_REQUEST, } from '@storybook/core/core-events'; +// eslint-disable-next-line depend/ban-dependencies import { execaNode } from 'execa'; import { log } from '../logger'; @@ -87,16 +88,6 @@ describe('bootTestRunner', () => { await expect(promise).rejects.toThrow(); }); - it('should abort if vitest fails to start repeatedly', async () => { - const promise = bootTestRunner(mockChannel); - message({ type: 'error' }); - vi.advanceTimersByTime(1000); - message({ type: 'error' }); - vi.advanceTimersByTime(1000); - message({ type: 'error' }); - await expect(promise).rejects.toThrow(); - }); - it('should forward channel events', async () => { bootTestRunner(mockChannel); message({ type: 'ready' });