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 3 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
2 changes: 1 addition & 1 deletion code/addons/vitest/src/postinstall.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ export default async function postInstall(options: PostinstallOptions) {
} else {
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('npx playwright install chromium')}
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.

@copilot here I would use os.platform to print the --with-deps command for darwin and win32 users, and for other users:

 ${CLI_COLORS.cta('npx playwright install chromium')} (add --with-deps if using Debian or Ubuntu)

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 8de539a. On macOS/Windows the warning shows npx playwright install chromium --with-deps; on other platforms it shows npx playwright install chromium with an appended note: Note: add --with-deps to the command above if you are on Debian or Ubuntu.

`);
}
}
Expand Down
74 changes: 56 additions & 18 deletions code/core/src/cli/AddonVitestService.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,7 +391,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,24 +416,62 @@ 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;
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 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({
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('should capture error stack when installation fails', async () => {
Expand Down
7 changes: 5 additions & 2 deletions code/core/src/cli/AddonVitestService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,9 @@
/**
* 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, also installs
* system-level browser dependencies via `--with-deps`.
*

Check failure on line 111 in code/core/src/cli/AddonVitestService.ts

View workflow job for this annotation

GitHub Actions / copilot 

Replace `⏎···*·dependencies·via·`--with-deps`.·On·other·platforms·(e.g.·Linux),·`--with-deps`·is·omitted⏎···*·to·avoid·requiring` with `·dependencies⏎···*·via·`--with-deps`.·On·other·platforms·(e.g.·Linux),·`--with-deps`·is·omitted·to·avoid·requiring⏎···*`
* @param packageManager - The package manager to use for installation
* @param prompt - The prompt instance for displaying progress
* @param logger - The logger instance for displaying messages
Expand All @@ -123,7 +124,9 @@
): Promise<{ errors: string[]; result: 'installed' | 'skipped' | 'aborted' | 'failed' }> {
const errors: string[] = [];

const playwrightCommand = ['playwright', 'install', 'chromium', '--with-deps'];
const playwrightCommand = process.env.CI
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.

@copilot Here I would suggest also enabling --with-deps for os.platform darwin and win32 as they are officially supported.

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 8de539a. --with-deps is now enabled for darwin and win32 (in addition to CI).

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

let result: 'installed' | 'skipped' | 'aborted' | 'failed';
Expand Down Expand Up @@ -319,7 +322,7 @@
isValid =
this.isWorkspaceConfigArray(declaration) || this.isDefineWorkspaceExpression(declaration);
},
});

Check warning on line 325 in code/core/src/cli/AddonVitestService.ts

View workflow job for this annotation

GitHub Actions / copilot 

Unexpected any. Specify a different type

return isValid;
}
Expand All @@ -335,7 +338,7 @@
isValidVitestConfig = this.isSafeToExtendWorkspace(path.node.declaration);
} else if (this.isMergeConfigExpression(path.node.declaration)) {
// the config could be anywhere in the mergeConfig call, so we need to check each argument
const mergeCall = path.node.declaration as CallExpression;

Check warning on line 341 in code/core/src/cli/AddonVitestService.ts

View workflow job for this annotation

GitHub Actions / copilot 

Unexpected any. Specify a different type
isValidVitestConfig = mergeCall.arguments.some((arg) =>
this.isSafeToExtendWorkspace(arg as CallExpression)
);
Expand All @@ -351,34 +354,34 @@
babel.types.isArrayExpression(node) &&
node?.elements.every(
(el: any) => babel.types.isStringLiteral(el) || babel.types.isObjectExpression(el)
)

Check warning on line 357 in code/core/src/cli/AddonVitestService.ts

View workflow job for this annotation

GitHub Actions / copilot 

Unexpected any. Specify a different type
);
}

private isDefineWorkspaceExpression(node: any): boolean {

Check warning on line 361 in code/core/src/cli/AddonVitestService.ts

View workflow job for this annotation

GitHub Actions / copilot 

Unexpected any. Specify a different type
return (
babel.types.isCallExpression(node) &&
node.callee &&
(node.callee as any)?.name === 'defineWorkspace' &&
this.isWorkspaceConfigArray(node.arguments?.[0])

Check warning on line 366 in code/core/src/cli/AddonVitestService.ts

View workflow job for this annotation

GitHub Actions / copilot 

Unexpected any. Specify a different type
);
}

private isDefineConfigExpression(node: any): boolean {

Check warning on line 370 in code/core/src/cli/AddonVitestService.ts

View workflow job for this annotation

GitHub Actions / copilot 

Unexpected any. Specify a different type
return (
babel.types.isCallExpression(node) &&
(node.callee as any)?.name === 'defineConfig' &&
babel.types.isObjectExpression(node.arguments?.[0])
);

Check warning on line 375 in code/core/src/cli/AddonVitestService.ts

View workflow job for this annotation

GitHub Actions / copilot 

Unexpected any. Specify a different type
}

private isMergeConfigExpression(path: babel.types.Node): boolean {

Check warning on line 378 in code/core/src/cli/AddonVitestService.ts

View workflow job for this annotation

GitHub Actions / copilot 

Unexpected any. Specify a different type
return babel.types.isCallExpression(path) && (path.callee as any)?.name === 'mergeConfig';
}

private isSafeToExtendWorkspace(node: babel.types.Node): boolean {
// Extract the object expression to validate
let objectToValidate: babel.types.ObjectExpression | null = null;

Check warning on line 384 in code/core/src/cli/AddonVitestService.ts

View workflow job for this annotation

GitHub Actions / copilot 

Unexpected any. Specify a different type

if (babel.types.isCallExpression(node)) {
// Handle function calls like defineConfig({...})
Expand Down
Loading
⚡