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

Addon Test: Integrate global error modal #29318

Merged
merged 5 commits into from
Oct 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion code/addons/test/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
86 changes: 69 additions & 17 deletions code/addons/test/src/manager.tsx
Original file line number Diff line number Diff line change
@@ -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 {
Expand All @@ -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';

Expand Down Expand Up @@ -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 = <RelativeTime timestamp={progress.finishedAt} />;
} else if (crashed) {
message = (
<>
<LinkComponent
isButton
onClick={() => {
setIsModalOpen(true);
}}
>
View full error
</LinkComponent>
</>
);
}
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}
<GlobalErrorModal
error={details?.message}
open={isModalOpen}
onClose={() => {
setIsModalOpen(false);
}}
onRerun={() => {
setIsModalOpen(false);
api
.getChannel()
.emit(TESTING_MODULE_RUN_ALL_REQUEST, { providerId: TEST_PROVIDER_ID });
}}
/>
</>
);
},

mapStatusUpdate: (state) =>
Expand All @@ -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,
Expand Down
11 changes: 1 addition & 10 deletions code/addons/test/src/node/boot-test-runner.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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' });
Expand Down
31 changes: 16 additions & 15 deletions code/addons/test/src/node/boot-test-runner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,20 +11,19 @@ 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
// which is at the root. Then, from the root, we want to load `node/vitest.mjs`
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[] = [];

Expand Down Expand Up @@ -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 = [];
Expand Down Expand Up @@ -85,30 +84,32 @@ export const bootTestRunner = async (channel: Channel, initEvent?: string, initA
resolve();
} else if (result.type === 'error') {
killChild();
log(`${result.message}: ${result.error}`);

if (attempt >= MAX_START_ATTEMPTS) {
log(`Aborting test runner process after ${attempt} restart attempts`);
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);
}
});
});

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;
});
Expand Down
24 changes: 16 additions & 8 deletions code/addons/test/src/node/reporter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -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();
}
}
1 change: 1 addition & 0 deletions code/addons/test/src/node/test-manager.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down
8 changes: 7 additions & 1 deletion code/addons/test/src/node/vitest-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ export class VitestManager {

this.vitest = await createVitest('test', {
watch: watchMode,
passWithNoTests: true,
passWithNoTests: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: Changing passWithNoTests to false might cause issues if there are no tests present. Consider the implications of this change on the test execution flow.

changed: watchMode,
// TODO:
// Do we want to enable Vite's default reporter?
Expand All @@ -37,6 +37,12 @@ export class VitestManager {
},
});

if (this.vitest) {
this.vitest.onCancel(() => {
// TODO: handle cancelation
});
}
Comment on lines +40 to +44
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: The onCancel handler is currently empty. Implement proper cancellation logic to ensure resources are cleaned up and the system is left in a consistent state.


await this.vitest.init();
}

Expand Down
20 changes: 14 additions & 6 deletions code/core/src/manager/components/sidebar/SidebarBottom.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ const initialTestProviderState: TestProviderState = {
running: false,
watching: false,
failed: false,
crashed: false,
};

const filterNone: API_FilterFunction = () => true;
Expand Down Expand Up @@ -123,17 +124,24 @@ export const SidebarBottomBase = ({ api, notifications = [], status = {} }: Side
[]
);

const onRunTests = useCallback(
(id: TestProviderId) => {
const clearState = useCallback(
({ providerId: id }: { providerId: TestProviderId }) => {
const startingState: Partial<TestProviderState> = {
cancelling: false,
running: true,
failed: false,
crashed: false,
};
setTestProviders((state) => ({ ...state, [id]: { ...state[id], ...startingState } }));
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]
Expand Down Expand Up @@ -171,7 +179,7 @@ export const SidebarBottomBase = ({ api, notifications = [], status = {} }: Side

useEffect(() => {
const onCrashReport = ({ providerId, ...details }: { providerId: string }) => {
updateTestProvider(providerId, { details, failed: true });
updateTestProvider(providerId, { details, running: false, crashed: true });
};

const onProgressReport = ({ providerId, ...payload }: TestingModuleProgressReportPayload) => {
Expand All @@ -190,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) {
Expand Down Expand Up @@ -226,8 +236,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();
Expand Down
Loading
Loading