Skip to content

Addon-Vitest: Make Playwright --with-deps platform-aware to avoid sudo prompt on Linux #34121

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.

Sign up for GitHub

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

Merged
valentinpalkovic merged 6 commits into from
Mar 13, 2026
Merged

Addon-Vitest: Make Playwright --with-deps platform-aware to avoid sudo prompt on Linux #34121

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
05cf78f
Initial plan
Copilot Mar 13, 2026
388e33b
fix: remove --with-deps from playwright install to prevent sudo requi…
Copilot Mar 13, 2026
590dffb
fix: use --with-deps only in CI environments for playwright install
Copilot Mar 13, 2026
8de539a
fix: use --with-deps on macOS/Windows and CI, skip on other Linux dis…
Copilot Mar 13, 2026
c9f38b0
fix: warn about missing system deps after Playwright install without …
Copilot Mar 13, 2026
bfb242e
Apply suggestion from @Sidnioulz
Sidnioulz Mar 13, 2026
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
11 changes: 10 additions & 1 deletion code/addons/vitest/src/postinstall.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { existsSync } from 'node:fs';
import * as fs from 'node:fs/promises';
import { writeFile } from 'node:fs/promises';
import os from 'node:os';

import { babelParse, generate, traverse } from 'storybook/internal/babel';
import { AddonVitestService } from 'storybook/internal/cli';
Expand Down Expand Up @@ -165,9 +166,17 @@ export default async function postInstall(options: PostinstallOptions) {
useRemotePkg: !!options.skipInstall,
});
} else {
const platform = os.platform();
const useWithDeps = platform === 'darwin' || platform === 'win32';
Copy link
Copy Markdown
Contributor

@valentinpalkovic valentinpalkovic Mar 13, 2026

Choose a reason for hiding this comment

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

cc @Sidnioulz: Isn't win32 a check for Windows? Do they need to run the command with --with-deps?

Copy link
Copy Markdown
Member

@Sidnioulz Sidnioulz Mar 13, 2026

Choose a reason for hiding this comment

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

@valentinpalkovic we know that the --with-deps command has worked well so far for MacOS and Windows users, so there is no reason to risk degrading their experience. We know all other OSes are partially or totally unsupported though.

Copy link
Copy Markdown
Contributor

@valentinpalkovic valentinpalkovic Mar 13, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I understand your thinking now. I've seen it as an opportunity to fasten up the init step by a bit. But fair enough. Let's go the safe route first.

const manualCommand = useWithDeps
? 'npx playwright install chromium --with-deps'
: 'npx playwright install chromium';
const linuxNote = !useWithDeps
? '\n Note: add --with-deps to the command above if you are on Debian or Ubuntu.'
: '';
logger.warn(dedent`
Playwright browser binaries installation skipped. Please run the following command manually later:
${CLI_COLORS.cta('npx playwright install chromium --with-deps')}
${CLI_COLORS.cta(manualCommand)}${linuxNote}
`);
}
}
Expand Down
142 changes: 123 additions & 19 deletions code/core/src/cli/AddonVitestService.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import * as fs from 'node:fs/promises';
import os from 'node:os';

import { beforeEach, describe, expect, it, vi } from 'vitest';

Expand All @@ -14,6 +15,7 @@ import { SupportedBuilder, SupportedFramework } from '../types';
import { AddonVitestService } from './AddonVitestService';

vi.mock('node:fs/promises', { spy: true });
vi.mock('node:os', { spy: true });
vi.mock('storybook/internal/common', { spy: true });
vi.mock('storybook/internal/node-logger', { spy: true });
vi.mock('empathic/find', { spy: true });
Expand Down Expand Up @@ -391,7 +393,7 @@ describe('AddonVitestService', () => {
vi.mocked(logger.warn).mockImplementation(() => {});
// Mock getPackageCommand to return a string
vi.mocked(mockPackageManager.getPackageCommand).mockReturnValue(
'npx playwright install chromium --with-deps'
'npx playwright install chromium'
);
});

Expand All @@ -416,25 +418,127 @@ describe('AddonVitestService', () => {
});

it('should execute playwright install command', async () => {
type ChildProcessFactory = (signal?: AbortSignal) => ResultPromise;
let commandFactory: ChildProcessFactory | ChildProcessFactory[];
vi.mocked(prompt.confirm).mockResolvedValue(true);
vi.mocked(prompt.executeTaskWithSpinner).mockImplementation(
async (factory: ChildProcessFactory | ChildProcessFactory[]) => {
commandFactory = Array.isArray(factory) ? factory[0] : factory;
// Simulate the child process completion
commandFactory();
const originalCI = process.env.CI;
delete process.env.CI;
vi.mocked(os.platform).mockReturnValue('linux');
try {
type ChildProcessFactory = (signal?: AbortSignal) => ResultPromise;
let commandFactory: ChildProcessFactory | ChildProcessFactory[];
vi.mocked(prompt.confirm).mockResolvedValue(true);
vi.mocked(prompt.executeTaskWithSpinner).mockImplementation(
async (factory: ChildProcessFactory | ChildProcessFactory[]) => {
commandFactory = Array.isArray(factory) ? factory[0] : factory;
// Simulate the child process completion
commandFactory();
}
);
Comment on lines +428 to +434
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Mar 13, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Move spinner mock implementations out of test cases and into beforeEach.

prompt.executeTaskWithSpinner is mocked inline in both tests; that breaks the repo’s Vitest mocking rules for centralized setup.

♻️ Suggested refactor 
 describe('installPlaywright', () => {
+  type ChildProcessFactory = (signal?: AbortSignal) => ResultPromise;
+
   beforeEach(() => {
     // Mock the logger methods used in installPlaywright
     vi.mocked(logger.log).mockImplementation(() => {});
     vi.mocked(logger.warn).mockImplementation(() => {});
     // Mock getPackageCommand to return a string
     vi.mocked(mockPackageManager.getPackageCommand).mockReturnValue(
       'npx playwright install chromium'
     );
+    vi.mocked(prompt.executeTaskWithSpinner).mockImplementation(
+      async (factory: ChildProcessFactory | ChildProcessFactory[]) => {
+        const commandFactory = Array.isArray(factory) ? factory[0] : factory;
+        commandFactory();
+        return undefined;
+      }
+    );
   });

   it('should execute playwright install command', async () => {
     const originalCI = process.env.CI;
     delete process.env.CI;
     try {
-      type ChildProcessFactory = (signal?: AbortSignal) => ResultPromise;
-      let commandFactory: ChildProcessFactory | ChildProcessFactory[];
       vi.mocked(prompt.confirm).mockResolvedValue(true);
-      vi.mocked(prompt.executeTaskWithSpinner).mockImplementation(
-        async (factory: ChildProcessFactory | ChildProcessFactory[]) => {
-          commandFactory = Array.isArray(factory) ? factory[0] : factory;
-          // Simulate the child process completion
-          commandFactory();
-        }
-      );

       await service.installPlaywright();

       expect(mockPackageManager.runPackageCommand).toHaveBeenCalledWith({
         args: ['playwright', 'install', 'chromium'],
@@
   it('should execute playwright install command with --with-deps in CI', async () => {
     const originalCI = process.env.CI;
     process.env.CI = 'true';
     try {
-      type ChildProcessFactory = (signal?: AbortSignal) => ResultPromise;
-      let commandFactory: ChildProcessFactory | ChildProcessFactory[];
       vi.mocked(prompt.confirm).mockResolvedValue(true);
-      vi.mocked(prompt.executeTaskWithSpinner).mockImplementation(
-        async (factory: ChildProcessFactory | ChildProcessFactory[]) => {
-          commandFactory = Array.isArray(factory) ? factory[0] : factory;
-          commandFactory();
-        }
-      );

       await service.installPlaywright();

       expect(mockPackageManager.runPackageCommand).toHaveBeenCalledWith({

As per coding guidelines “Avoid inline mock implementations within test cases in Vitest tests” and “Implement mock behaviors in beforeEach blocks in Vitest tests”.

Also applies to: 454-459

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@code/core/src/cli/AddonVitestService.test.ts` around lines 425 - 431, Move
the inline vi.mocked(prompt.executeTaskWithSpinner) implementations into the
test suite's beforeEach: create a beforeEach that sets
vi.mocked(prompt.executeTaskWithSpinner).mockImplementation(async (factory:
ChildProcessFactory | ChildProcessFactory[]) => { commandFactory =
Array.isArray(factory) ? factory[0] : factory; commandFactory(); }); also reset
commandFactory to undefined (or a known initial state) at the start of
beforeEach so each test gets a clean setup, and remove the duplicate inline
mocks found in the two test cases (including the other occurrence around the
second test). Ensure both tests rely on the shared behavior provided by the
beforeEach mock rather than defining their own mock implementations.


await service.installPlaywright();

expect(mockPackageManager.runPackageCommand).toHaveBeenCalledWith({
args: ['playwright', 'install', 'chromium'],
signal: undefined,
stdio: ['inherit', 'pipe', 'pipe'],
});
} finally {
if (originalCI !== undefined) {
process.env.CI = originalCI;
}
);

await service.installPlaywright();

expect(mockPackageManager.runPackageCommand).toHaveBeenCalledWith({
args: ['playwright', 'install', 'chromium', '--with-deps'],
signal: undefined,
stdio: ['inherit', 'pipe', 'pipe'],
});
});
}
Comment on lines +443 to +447
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Mar 13, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Environment variable restoration logic differs across tests.

The finally blocks have inconsistent logic for restoring process.env.CI:

  • Lines 443-447: Only restores if originalCI !== undefined
  • Lines 502-508: Deletes if originalCI === undefined, else restores
  • Lines 535-539: Only restores if originalCI !== undefined

Lines 443-447 and 535-539 may leave process.env.CI set if it was originally undefined but the test set it to a value. The pattern in lines 502-508 is correct.

🛠️ Suggested fix for consistent cleanup 
       } finally {
-        if (originalCI !== undefined) {
-          process.env.CI = originalCI;
+        if (originalCI === undefined) {
+          delete process.env.CI;
+        } else {
+          process.env.CI = originalCI;
         }
       }

Apply this pattern to all three locations for consistency.

Also applies to: 502-508, 535-539

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@code/core/src/cli/AddonVitestService.test.ts` around lines 443 - 447, The
finally blocks currently restore process.env.CI inconsistently (some only
restore when originalCI !== undefined), which can leave CI set when it was
originally undefined; update all finally blocks in AddonVitestService.test.ts
that use originalCI/process.env.CI so they follow the correct pattern: if
originalCI === undefined then delete process.env.CI, else set process.env.CI =
originalCI. Locate uses of originalCI and the finally blocks around the tests
(the three spots shown around lines with the finally blocks) and apply this
consistent cleanup logic.

});

it('should warn about missing system dependencies after install on Linux', async () => {
const originalCI = process.env.CI;
delete process.env.CI;
vi.mocked(os.platform).mockReturnValue('linux');
try {
type ChildProcessFactory = (signal?: AbortSignal) => ResultPromise;
vi.mocked(prompt.confirm).mockResolvedValue(true);
vi.mocked(prompt.executeTaskWithSpinner).mockImplementation(
async (factory: ChildProcessFactory | ChildProcessFactory[]) => {
const commandFactory = Array.isArray(factory) ? factory[0] : factory;
commandFactory();
}
);

const { result } = await service.installPlaywright();

expect(result).toBe('installed');
expect(logger.warn).toHaveBeenCalledWith(
expect.stringContaining('installed without system dependencies')
);
expect(logger.warn).toHaveBeenCalledWith(
expect.stringContaining('run Storybook Test from the Storybook UI')
);
} finally {
if (originalCI !== undefined) {
process.env.CI = originalCI;
}
}
});

it('should execute playwright install command with --with-deps in CI', async () => {
const originalCI = process.env.CI;
process.env.CI = 'true';
vi.mocked(os.platform).mockReturnValue('linux');
try {
type ChildProcessFactory = (signal?: AbortSignal) => ResultPromise;
let commandFactory: ChildProcessFactory | ChildProcessFactory[];
vi.mocked(prompt.confirm).mockResolvedValue(true);
vi.mocked(prompt.executeTaskWithSpinner).mockImplementation(
async (factory: ChildProcessFactory | ChildProcessFactory[]) => {
commandFactory = Array.isArray(factory) ? factory[0] : factory;
commandFactory();
}
);

await service.installPlaywright();

expect(mockPackageManager.runPackageCommand).toHaveBeenCalledWith({
args: ['playwright', 'install', 'chromium', '--with-deps'],
signal: undefined,
stdio: ['inherit', 'pipe', 'pipe'],
});
} finally {
if (originalCI === undefined) {
delete process.env.CI;
} else {
process.env.CI = originalCI;
}
}
});

it.each(['darwin', 'win32'] as const)(
'should execute playwright install command with --with-deps on %s',
async (platform) => {
const originalCI = process.env.CI;
delete process.env.CI;
vi.mocked(os.platform).mockReturnValue(platform);
try {
type ChildProcessFactory = (signal?: AbortSignal) => ResultPromise;
let commandFactory: ChildProcessFactory | ChildProcessFactory[];
vi.mocked(prompt.confirm).mockResolvedValue(true);
vi.mocked(prompt.executeTaskWithSpinner).mockImplementation(
async (factory: ChildProcessFactory | ChildProcessFactory[]) => {
commandFactory = Array.isArray(factory) ? factory[0] : factory;
commandFactory();
}
);

await service.installPlaywright();

expect(mockPackageManager.runPackageCommand).toHaveBeenCalledWith({
args: ['playwright', 'install', 'chromium', '--with-deps'],
signal: undefined,
stdio: ['inherit', 'pipe', 'pipe'],
});
} finally {
if (originalCI !== undefined) {
process.env.CI = originalCI;
}
}
}
);

it('should capture error stack when installation fails', async () => {
const error = new Error('Installation failed');
Expand Down
20 changes: 18 additions & 2 deletions code/core/src/cli/AddonVitestService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import fs from 'node:fs/promises';
import os from 'node:os';

import * as babel from 'storybook/internal/babel';
import type { JsPackageManager } from 'storybook/internal/common';
Expand Down Expand Up @@ -106,7 +107,10 @@ export class AddonVitestService {
/**
* Install Playwright browser binaries for @storybook/addon-vitest
*
* Installs Chromium with dependencies via `npx playwright install chromium --with-deps`
* Installs Chromium via `npx playwright install chromium`. In CI environments and on
* macOS/Windows (officially supported platforms), also installs system-level browser dependencies
* via `--with-deps`. On other platforms (e.g. Linux), `--with-deps` is omitted to avoid requiring
* `sudo` — system packages are typically managed by the distro package manager.
*
* @param packageManager - The package manager to use for installation
* @param prompt - The prompt instance for displaying progress
Expand All @@ -123,7 +127,11 @@ export class AddonVitestService {
): Promise<{ errors: string[]; result: 'installed' | 'skipped' | 'aborted' | 'failed' }> {
const errors: string[] = [];

const playwrightCommand = ['playwright', 'install', 'chromium', '--with-deps'];
const platform = os.platform();
const useWithDeps = !!process.env.CI || platform === 'darwin' || platform === 'win32';
const playwrightCommand = useWithDeps
Copy link
Copy Markdown
Member

@Sidnioulz Sidnioulz Mar 13, 2026

Choose a reason for hiding this comment

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

Nice! @copilot now, when not using useWithDeps, I want you to print a nice formatted warning to users after the playwright install, if it succeeded, to tell them that they may need to install additional libraries on their operating system for Playwright to function properly. They can run Storybook Test in the Storybook UI to get a list of missing dependencies.

Copy link
Copy Markdown
Contributor Author

Copilot AI Mar 13, 2026

Choose a reason for hiding this comment

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

Done in c9f38b0. After a successful install without --with-deps, users on Linux now see:

⚠ Playwright was installed without system dependencies. Depending on your operating system, you may need to install additional libraries for Playwright to work correctly.
  To check for missing dependencies, run Storybook Test from the Storybook UI — it will report any libraries that need to be installed.
  If needed, you can install system dependencies manually by running:
  npx playwright install chromium --with-deps

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

let result: 'installed' | 'skipped' | 'aborted' | 'failed';
Expand Down Expand Up @@ -168,6 +176,14 @@ export class AddonVitestService {
result = 'aborted';
} else {
result = 'installed';
if (!useWithDeps) {
logger.warn(dedent`
Playwright was installed without system dependencies. Depending on your operating system, you may need to install additional libraries for Playwright to work correctly.
To check for missing dependencies, run Storybook Test from the Storybook UI — it will report any libraries that need to be installed.
On MacOS, Windows, Debian and Ubuntu, you can install system dependencies manually by running:
${CLI_COLORS.cta(this.packageManager.getPackageCommand(['playwright', 'install', 'chromium', '--with-deps']))}
Comment on lines +181 to +184
Copy link
Copy Markdown
Member

@Sidnioulz Sidnioulz Mar 13, 2026

Choose a reason for hiding this comment

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

@valentinpalkovic WDYT of this?

I would only change "If needed" with "On MacOS, Windows, Debian and Ubuntu,"

`);
}
}
} else {
logger.warn('Playwright installation skipped');
Expand Down
Loading
⚡