Addon-Vitest: Make Playwright --with-deps platform-aware to avoid sudo prompt on Linux#34121
Conversation
…rement Co-authored-by: valentinpalkovic <5889929+valentinpalkovic@users.noreply.github.com>
--with-deps from Playwright install to avoid sudo prompt
valentinpalkovic
changed the title
--with-deps from Playwright install to avoid sudo prompt--with-deps from Playwright install to avoid sudo prompt
|
@copilot Add back '--with-deps' in CI environments (process.env.CI) |
|
View your CI Pipeline Execution ↗ for commit bfb242e
☁️ Nx Cloud last updated this comment at |
Co-authored-by: valentinpalkovic <5889929+valentinpalkovic@users.noreply.github.com>
--with-deps from Playwright install to avoid sudo prompt--with-deps from Playwright install to avoid sudo prompt on local installs
📝 WalkthroughWalkthroughMake Playwright's Chromium install command conditional on platform and CI: include Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AddonVitestService
participant OS as node:os
participant Child as ChildProcess
User->>AddonVitestService: trigger installPlaywright
AddonVitestService->>OS: os.platform()
OS-->>AddonVitestService: platform
AddonVitestService->>AddonVitestService: compute useWithDeps (CI || darwin || win32)
AddonVitestService->>Child: spawn("npx playwright install chromium" [+ "--with-deps" if useWithDeps])
Child-->>AddonVitestService: exit/ok or error
AddonVitestService-->>User: print success or platform-specific warning (Linux deps note when omitted)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
✨ Finishing Touches
📝 Coding Plan
Comment Tip CodeRabbit can generate a title for your PR based on the changes.Add |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/core/src/cli/AddonVitestService.test.ts`:
- Around line 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.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3b4a9129-cc60-4912-83cc-73ce9aacecc3
📒 Files selected for processing (3)
code/addons/vitest/src/postinstall.tscode/core/src/cli/AddonVitestService.test.tscode/core/src/cli/AddonVitestService.ts
| vi.mocked(prompt.executeTaskWithSpinner).mockImplementation( | ||
| async (factory: ChildProcessFactory | ChildProcessFactory[]) => { | ||
| commandFactory = Array.isArray(factory) ? factory[0] : factory; | ||
| // Simulate the child process completion | ||
| commandFactory(); | ||
| } | ||
| ); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This is SO MUCH NICER. No more buggy sudo prompt.
I would suggest using os.platform, on top of CI, to still install deps on win and darwin as we know it works for those users.
I would also suggest writing a big warning stating we skipped deps install for playwright and users may have extra work to do depending on their distro. I would print the "--with-deps" command as part of that warning for Debian/Ubuntu users.
| const errors: string[] = []; | ||
|
|
||
| const playwrightCommand = ['playwright', 'install', 'chromium', '--with-deps']; | ||
| const playwrightCommand = process.env.CI |
There was a problem hiding this comment.
@copilot Here I would suggest also enabling --with-deps for os.platform darwin and win32 as they are officially supported.
There was a problem hiding this comment.
Done in 8de539a. --with-deps is now enabled for darwin and win32 (in addition to CI).
| 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')} |
There was a problem hiding this comment.
@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)There was a problem hiding this comment.
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.
…tros Co-authored-by: Sidnioulz <5108577+Sidnioulz@users.noreply.github.com>
Done in 8de539a. |
--with-deps from Playwright install to avoid sudo prompt on local installs--with-deps platform-aware to avoid sudo prompt on Linux
Manual testing
Create a sandbox on Linux. There should be no sudo prompt.
During
storybook init, the addon-vitest postinstall was runningnpx playwright install chromium --with-deps, which triggers system-level package installation (apt/pacman/etc.) requiringsudoon Linux — prompting users for a root password mid-init with no clear context.Changes
AddonVitestService.ts: Use--with-depsonly when running in CI (process.env.CI) or on macOS/Windows (os.platform() === 'darwin' | 'win32') — officially supported platforms where it works reliably. Omit it on other platforms (Linux) to avoid thesudoprompt. After a successful install without--with-deps, a formatted warning is printed informing users they may need to install additional OS libraries for Playwright to work correctly, directing them to run Storybook Test in the Storybook UI to identify missing dependencies, and showing the manual--with-depscommand. Updated JSDoc to reflect the conditional behavior.postinstall.ts: Update the skipped-install warning to be platform-aware: showsnpx playwright install chromium --with-depson macOS/Windows, andnpx playwright install chromiumwith an appended note pointing Debian/Ubuntu users to add--with-depson other Linux platforms.AddonVitestService.test.ts: Update assertions to cover all code paths: CI (with--with-deps), Linux non-CI (without--with-deps), macOS/Windows (with--with-deps), and the post-install warning on Linux.Without
--with-depson Linux local installs, Playwright downloads the Chromium binary only, skipping OS-level dependency installation. Browser dependencies are typically already present on Linux systems via the distro package manager. On macOS, Windows, and in CI environments,--with-depsis retained to ensure all required system dependencies are installed. On Linux, users are shown a post-install warning with guidance on how to check for and install any missing dependencies.Original prompt
This section details on the original issue you should resolve
<issue_title>[Bug]: Storybook init can require
sudobecause of Playwright install</issue_title><issue_description>### Describe the bug
When initting Storybook and getting addon-vitest, the addon's postinstall installs Playwright with dependencies:
storybook/code/addons/vitest/src/postinstall.ts
Lines 294 to 297 in bd03a61
This can lead to the terminal require
sudo, because that can be necessary to install the browser dependencies. This requirement can scare some users off, because they are not just givingsudoto the playwright install, but to the whole init process, which is opaque to the user and less secure.Additional context
Originally reported by @dominikg. I have not myself been able to reproduce this, it seems like Playwright refuses to install browsers on my machine, relying on some cached installs that I can't remove.
This the log shown:
If we don't add
--with-depsit just downloads the binaries without usingsudo. The deps are usually present from the system installed chromium and managed by the packaging system of the linux distro. It can lead to issues if the system installed chromium has deps that are incompatible from the one playwright wants to use, but that might not be an issue in practice.</issue_description>Comments on the Issue (you are @copilot in this section)
@Sidnioulz I want to add to this that some commands, like `yarn start` in the monorepo, run install commands in quiet mode, so users don't even know it's Playwright asking for a password.Besides, Playwright fails install on many Linux distros, and it tries for those distros to serve a 5 year old APT package for Ubuntu, which of course fails (all the package's dependencies have had multiple updates since then).
microsoft/playwright#26482 (comment) provides a command which apparently installs browsers without triggering this whole package download and sudo request. We might want to look into adding that
--shell-onlycommand if it has no relevant side effects for us.</comment_new><comment_new>@Sidnioulz
Thanks @dominikg! I suspect this might be enough for Addon Vitest, as it runs in CLI and outputs results in the UI and/or CLI. Let's see if the team believe --headed must be available by default.</comment_new>
<comment_new>@Sidnioulz
Just tested rewriting addon-vitest's postinstall and
sudowill keep being triggered until--with-depsis removed. Now, on my Arch system I can install with--only-shelland without--with-deps, and the addon runs fine because I already have playwright locally installed.Obviously this means users without playwright will run into trouble.
One thing we could possibly do is use
execato runplaywright --versionand check if it's already installed; in which case--with-depsis redundant on Linux.We could also try and detect distros. Playwright only supports Debian/Ubuntu so that shouldn't be too much of a stretch. Plenty of options to detect distros.
ANd if we figure out that users do need to install Playwright themselves, provide a big fat visible warning for them so they know they have to do it.</comment_new>
<comment_new>@Sidnioulz
Agreed, it's the NPM install DX that's broken. They don't know how to install it so they prompt you into typing a password and start downloading an Ubuntu 20 package, then they cry wolf because the package is using different dependency versions from the ones locally installed.
Meanwhile, not installing Playwright via NPM results in a functional experience for me on Arch.</comment_new>
<comment_new>@Sidnioulz
Unfortunately it's not exactly the last step. It still does one or two things with Vitest config files afterwards.
@JReinhold, do you reckon it's doable to put the playwright install step at the very end of the process?
sudobecause of Playwright install #32043💬 Send tasks to Copilot coding agent from Slack and Teams to turn conversations into code. Copilot posts an update in your thread when it's finished.
Summary by CodeRabbit