Skip to content
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
17 changes: 14 additions & 3 deletions code/core/src/cli/AddonVitestService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -114,12 +114,16 @@ export class AddonVitestService {
* @param options - Installation options
* @returns Array of error messages if installation fails
*/
async installPlaywright(options: { yes?: boolean } = {}): Promise<{ errors: string[] }> {
async installPlaywright(
options: { yes?: boolean } = {}
): Promise<{ errors: string[]; result: 'installed' | 'skipped' | 'aborted' | 'failed' }> {
const errors: string[] = [];

const playwrightCommand = ['playwright', 'install', 'chromium', '--with-deps'];
const playwrightCommandString = this.packageManager.getPackageCommand(playwrightCommand);

let result: 'installed' | 'skipped' | 'aborted' | 'failed';

try {
const shouldBeInstalled = options.yes
? true
Expand All @@ -135,7 +139,7 @@ export class AddonVitestService {
})();

if (shouldBeInstalled) {
await prompt.executeTaskWithSpinner(
const processAborted = await prompt.executeTaskWithSpinner(
(signal) =>
this.packageManager.runPackageCommand({
args: playwrightCommand,
Expand All @@ -150,10 +154,17 @@ export class AddonVitestService {
abortable: true,
}
);
if (processAborted) {
result = 'aborted';
} else {
result = 'installed';
}
} else {
logger.warn('Playwright installation skipped');
result = 'skipped';
}
} catch (e) {
result = 'failed';
ErrorCollector.addError(e);
if (e instanceof Error) {
errors.push(e.stack ?? e.message);
Expand All @@ -162,7 +173,7 @@ export class AddonVitestService {
}
}

return { errors };
return { errors, result };
}

/**
Expand Down
75 changes: 75 additions & 0 deletions code/core/src/node-logger/tasks.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
import { describe, expect, it, vi } from 'vitest';

// eslint-disable-next-line depend/ban-dependencies
import type { ExecaChildProcess } from 'execa';

import { executeTaskWithSpinner } from './tasks';

// Create a minimal fake ExecaChildProcess
const makeChild = (onStart?: (cp: Partial<ExecaChildProcess>) => void): ExecaChildProcess => {
const listeners: Record<string, Function[]> = {};
const stdout = {
on: vi.fn((event: string, cb: (data: Buffer) => void) => {
listeners[event] ||= [];
listeners[event].push(cb);
}),
} as any;

const cp: Partial<ExecaChildProcess> = {
stdout: stdout as any,
then: undefined as any,
catch: undefined as any,
finally: undefined as any,
};

const promise = Promise.resolve() as any;
Object.setPrototypeOf(cp, promise);
(cp as any).then = promise.then.bind(promise);
(cp as any).catch = promise.catch.bind(promise);
(cp as any).finally = promise.finally.bind(promise);

onStart?.(cp);
return cp as ExecaChildProcess;
};

describe('executeTaskWithSpinner', () => {
it('returns "aborted" when the child process rejects with an abort error', async () => {
const outcome = await executeTaskWithSpinner(() => makeChild(), {
id: 'test',
intro: 'Intro',
error: 'Error',
success: 'Success',
abortable: true,
});

// Non-abort path returns undefined
expect(outcome).toBeUndefined();

// Simulate an aborted child process by rejecting with an abort-like error message
const outcome2 = await executeTaskWithSpinner(
() => {
const err = new Error('The operation was aborted');
const p = Promise.reject(err);
// Avoid unhandled rejection warnings
p.catch(() => {});
const cp: any = makeChild();
// Make the thenable reject
const rejected = p as any;
Object.setPrototypeOf(cp, rejected);
cp.then = rejected.then.bind(rejected);
cp.catch = rejected.catch.bind(rejected);
cp.finally = rejected.finally.bind(rejected);
return cp;
},
{
id: 'test2',
intro: 'Intro',
error: 'Error',
success: 'Success',
abortable: true,
}
);

expect(outcome2).toBe('aborted');
});
});
10 changes: 6 additions & 4 deletions code/core/src/node-logger/tasks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ export const executeTask = async (
success,
abortable = false,
}: { intro: string; error: string; success: string; abortable?: boolean }
) => {
): Promise<'aborted' | void> => {
logTracker.addLog('info', intro);
log(intro);

Expand Down Expand Up @@ -99,7 +99,7 @@ export const executeTask = async (
if (isAborted) {
logTracker.addLog('info', `${intro} aborted`);
log(CLI_COLORS.error(`${intro} aborted`));
return;
return 'aborted';
}
const errorMessage = err instanceof Error ? (err.stack ?? err.message) : String(err);
logTracker.addLog('error', error, { error: errorMessage });
Expand All @@ -108,6 +108,7 @@ export const executeTask = async (
} finally {
cleanup?.();
}
return undefined;
};

export const executeTaskWithSpinner = async (
Expand All @@ -119,7 +120,7 @@ export const executeTaskWithSpinner = async (
success,
abortable = false,
}: { id: string; intro: string; error: string; success: string; abortable?: boolean }
) => {
): Promise<'aborted' | void> => {
logTracker.addLog('info', intro);

let abortController: AbortController | undefined;
Expand Down Expand Up @@ -159,7 +160,7 @@ export const executeTaskWithSpinner = async (
if (isAborted) {
logTracker.addLog('info', `${intro} aborted`);
task.cancel(CLI_COLORS.warning(`${intro} aborted`));
return;
return 'aborted';
}
const errorMessage = err instanceof Error ? (err.stack ?? err.message) : String(err);
logTracker.addLog('error', error, { error: errorMessage });
Expand All @@ -168,4 +169,5 @@ export const executeTaskWithSpinner = async (
} finally {
cleanup?.();
}
return undefined;
};
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,10 @@ import { AddonConfigurationCommand } from './AddonConfigurationCommand';

vi.mock('storybook/internal/node-logger', { spy: true });

vi.mock('storybook/internal/cli', () => ({
vi.mock('storybook/internal/cli', async (actualImport) => ({
...(await actualImport()),
AddonVitestService: vi.fn().mockImplementation(() => ({
installPlaywright: vi.fn().mockResolvedValue([]),
installPlaywright: vi.fn().mockResolvedValue({ errors: [] }),
})),
}));
Comment on lines +10 to 15
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Mock missing the result field from the updated return type.

The installPlaywright mock returns { errors: [] }, but the actual return type is { errors: string[]; result: 'installed' | 'skipped' | 'aborted' | 'failed' }. The production code accesses result to pass to telemetryService.trackPlaywrightPromptDecision(result). This may cause test failures or mask issues.

 vi.mock('storybook/internal/cli', async (actualImport) => ({
   ...(await actualImport()),
   AddonVitestService: vi.fn().mockImplementation(() => ({
-    installPlaywright: vi.fn().mockResolvedValue({ errors: [] }),
+    installPlaywright: vi.fn().mockResolvedValue({ errors: [], result: 'installed' }),
   })),
 }));

And in beforeEach:

     const mockInstance = {
-      installPlaywright: vi.fn().mockResolvedValue({ errors: [] }),
+      installPlaywright: vi.fn().mockResolvedValue({ errors: [], result: 'installed' }),
     };

Also applies to: 43-48

🤖 Prompt for AI Agents
In code/lib/create-storybook/src/commands/AddonConfigurationCommand.test.ts
around lines 10-15 (and similarly at lines 43-48), the mocked installPlaywright
return value is missing the required result field; update the mock to return the
full shape { errors: string[]; result: 'installed' | 'skipped' | 'aborted' |
'failed' } (e.g., { errors: [], result: 'installed' }) so the production code
can read result and telemetryService.trackPlaywrightPromptDecision(result) will
receive a valid value.


Expand Down Expand Up @@ -42,11 +43,14 @@ describe('AddonConfigurationCommand', () => {
const { AddonVitestService } = await import('storybook/internal/cli');
mockAddonVitestService = vi.mocked(AddonVitestService as any);
const mockInstance = {
installPlaywright: vi.fn().mockResolvedValue([]),
installPlaywright: vi.fn().mockResolvedValue({ errors: [] }),
};
mockAddonVitestService.mockImplementation(() => mockInstance);
mockAddonVitestService.mockImplementation(() => mockInstance as any);

command = new AddonConfigurationCommand(mockPackageManager);
command = new AddonConfigurationCommand(mockPackageManager, {
yes: true,
disableTelemetry: true,
} as any);

mockTask = {
success: vi.fn(),
Expand All @@ -64,16 +68,11 @@ describe('AddonConfigurationCommand', () => {
describe('execute', () => {
it('should skip configuration when no addons are provided', async () => {
const addons: string[] = [];
const options = {
packageManager: PackageManagerName.NPM,
features: [],
};

const result = await command.execute({
dependencyInstallationResult: { status: 'success' },
addons,
configDir: '.storybook',
options,
});

expect(result.status).toBe('success');
Expand All @@ -83,17 +82,11 @@ describe('AddonConfigurationCommand', () => {

it('should configure test addons when test feature is enabled', async () => {
const addons = ['@storybook/addon-a11y', '@storybook/addon-vitest'];
const options = {
packageManager: PackageManagerName.NPM,
features: [],
yes: true,
};

const result = await command.execute({
dependencyInstallationResult: { status: 'success' },
addons,
configDir: '.storybook',
options,
});

expect(result.status).toBe('success');
Expand All @@ -105,10 +98,6 @@ describe('AddonConfigurationCommand', () => {

it('should handle configuration errors gracefully', async () => {
const addons = ['@storybook/addon-a11y', '@storybook/addon-vitest'];
const options = {
packageManager: PackageManagerName.NPM,
features: [],
};
const error = new Error('Configuration failed');

mockPostinstallAddon.mockRejectedValue(error);
Expand All @@ -117,7 +106,6 @@ describe('AddonConfigurationCommand', () => {
dependencyInstallationResult: { status: 'success' },
addons,
configDir: '.storybook',
options,
});

expect(result.status).toBe('failed');
Expand All @@ -128,17 +116,11 @@ describe('AddonConfigurationCommand', () => {

it('should complete successfully with valid configuration', async () => {
const addons = ['@storybook/addon-a11y', '@storybook/addon-vitest'];
const options = {
packageManager: PackageManagerName.NPM,
features: [],
yes: true,
};

const result = await command.execute({
dependencyInstallationResult: { status: 'success' },
addons,
configDir: '.storybook',
options,
});

expect(result.status).toBe('success');
Expand Down
31 changes: 19 additions & 12 deletions code/lib/create-storybook/src/commands/AddonConfigurationCommand.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { ErrorCollector } from 'storybook/internal/telemetry';
import { dedent } from 'ts-dedent';

import type { CommandOptions } from '../generators/types';
import { TelemetryService } from '../services';

const ADDON_INSTALLATION_INSTRUCTIONS = {
'@storybook/addon-vitest':
Expand All @@ -14,7 +15,6 @@ const ADDON_INSTALLATION_INSTRUCTIONS = {

type ExecuteAddonConfigurationParams = {
addons: string[];
options: CommandOptions;
configDir?: string;
dependencyInstallationResult: { status: 'success' | 'failed' };
};
Expand All @@ -35,18 +35,19 @@ export type ExecuteAddonConfigurationResult = {
export class AddonConfigurationCommand {
constructor(
readonly packageManager: JsPackageManager,
private readonly addonVitestService = new AddonVitestService(packageManager)
private readonly commandOptions: CommandOptions,
private readonly addonVitestService = new AddonVitestService(packageManager),
private readonly telemetryService = new TelemetryService(commandOptions.disableTelemetry)
) {}

/** Execute addon configuration */
async execute({
options,
addons,
configDir,
dependencyInstallationResult,
}: ExecuteAddonConfigurationParams): Promise<ExecuteAddonConfigurationResult> {
const areDependenciesInstalled =
dependencyInstallationResult.status === 'success' && !options.skipInstall;
dependencyInstallationResult.status === 'success' && !this.commandOptions.skipInstall;

if (!areDependenciesInstalled && this.getAddonsWithInstructions(addons).length > 0) {
this.logManualAddonInstructions(addons);
Expand All @@ -58,12 +59,14 @@ export class AddonConfigurationCommand {
}

try {
const { hasFailures, addonResults } = await this.configureAddons(configDir, addons, options);
const { hasFailures, addonResults } = await this.configureAddons(configDir, addons);

if (addonResults.has('@storybook/addon-vitest')) {
await this.addonVitestService.installPlaywright({
yes: options.yes,
const { result } = await this.addonVitestService.installPlaywright({
yes: this.commandOptions.yes,
});
// Map outcome to telemetry decision
await this.telemetryService.trackPlaywrightPromptDecision(result);
}

return { status: hasFailures ? 'failed' : 'success' };
Expand Down Expand Up @@ -103,7 +106,7 @@ export class AddonConfigurationCommand {
}

/** Configure test addons (a11y and vitest) */
private async configureAddons(configDir: string, addons: string[], options: CommandOptions) {
private async configureAddons(configDir: string, addons: string[]) {
// Import postinstallAddon from cli-storybook package
const { postinstallAddon } = await import('../../../cli-storybook/src/postinstallAddon');

Expand All @@ -123,7 +126,7 @@ export class AddonConfigurationCommand {
await postinstallAddon(addon, {
packageManager: this.packageManager.type,
configDir,
yes: options.yes,
yes: this.commandOptions.yes,
skipInstall: true,
skipDependencyManagement: true,
logger,
Expand Down Expand Up @@ -161,7 +164,11 @@ export class AddonConfigurationCommand {

export const executeAddonConfiguration = ({
packageManager,
...options
}: ExecuteAddonConfigurationParams & { packageManager: JsPackageManager }) => {
return new AddonConfigurationCommand(packageManager).execute(options);
options,
...rest
}: ExecuteAddonConfigurationParams & {
packageManager: JsPackageManager;
options: CommandOptions;
}) => {
return new AddonConfigurationCommand(packageManager, options).execute(rest);
};
12 changes: 11 additions & 1 deletion code/lib/create-storybook/src/services/TelemetryService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ export class TelemetryService {

/** Track a new user check step */
async trackNewUserCheck(newUser: boolean): Promise<void> {
this.runTelemetryIfEnabled('init-step', {
await this.runTelemetryIfEnabled('init-step', {
step: 'new-user-check',
newUser,
});
Expand All @@ -32,6 +32,16 @@ export class TelemetryService {
});
}

/** Track Playwright prompt decision (install | skip | aborted) */
async trackPlaywrightPromptDecision(
result: 'installed' | 'skipped' | 'aborted' | 'failed'
): Promise<void> {
await this.runTelemetryIfEnabled('init-step', {
step: 'playwright-install',
result,
});
}

/** Track the main init event with all metadata */
async trackInit(data: {
projectType: ProjectType;
Expand Down