Browser mode boolean options (like headless) are sent as string from CLI

martypdx commented 9 months ago

Describe the bug

CLI is not converting browser.headless to boolean value which causes error and logic issues in downstream BrowserProviders.

For playwright it throws:

Error: browserType.launch: headless: expected boolean, got string
 ❯ PlaywrightBrowserProvider.openBrowserPage node_modules/.pnpm/@vitest+browser@1.2.2_vitest@1.2.2/node_modules/@vitest/browser/dist/providers.js:22:52

For webdriverio it always runs headless true from CLI because "false" is true 🤣

Looks like this code is what needs to enhanced.

Is there any behavioral testing for browser options?

I suspect problem also exists in browser.isolate and browser.slowHijackESM. browser.api is probably safe because it is turned into string based url settings.

Reproduction

I couldn't get headless to work in StackBlitz 🤷

Don't know if there are behavioral tests for options in the repo?

Bug is self-evident in the code.

System Info

System:
    OS: macOS 12.7
    CPU: (8) arm64 Apple M1
    Memory: 392.06 MB / 16.00 GB
    Shell: 5.8.1 - /bin/zsh
  Binaries:
    Node: 21.1.0 - ~/.nvm/versions/node/v21.1.0/bin/node
    npm: 10.3.0 - ~/.nvm/versions/node/v21.1.0/bin/npm
    pnpm: 8.14.3 - ~/.nvm/versions/node/v21.1.0/bin/pnpm
  Browsers:
    Chrome: 120.0.6099.234
    Safari: 17.0
  npmPackages:
    @vitest/browser: ^1.2.2 => 1.2.2 
    vite: ^5.0.12 => 5.0.12 
    vitest: ^1.2.2 => 1.2.2

Used Package Manager

pnpm

Validations

[X] Follow our Code of Conduct
[X] Read the Contributing Guidelines.
[X] Read the docs.
[X] Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
[X] Check that this is a concrete bug. For Q&A open a GitHub Discussion or join our Discord Chat Server.
[X] The provided reproduction is a minimal reproducible example of the bug.

sheremet-va commented 9 months ago

It's supposed to be fixed by https://github.com/vitest-dev/vitest/pull/3983

martypdx commented 9 months ago

@sheremet-va I scanned through that PR and don't see anything about typing.

sheremet-va commented 9 months ago

@sheremet-va I scanned through that PR and don't see anything about typing.

Typing is fixed by correctly defining all cli options automatically.

martypdx commented 9 months ago

BTW, if someone needs a workaround best option is to try and use -c to point at different config:

"scripts": {
    "test": "vitest",
    "test:ui": "vitest -c vite.config.in-browser.js"
},

martypdx commented 9 months ago

Typing is fixed by correctly defining all cli options automatically.

Are you depending on compile-time type checking?

AFAICT the .options call is taking two strings. How does that specify type?

sheremet-va commented 9 months ago

AFAICT the .options call is taking two strings. How does that specify type?

It parses the first string. No nested options are specified currently so they are treated as strings by default.

martypdx commented 9 months ago

It parses the first string. No nested options are specified currently so they are treated as strings by default.

Which causes the bug downstream.

I can't tell if you're processing through this or skeptical that this is a bug. If the later, I've updated the StackBlitz Repo to reveal the bug. When it runs "test": "vitest --browser.headless=false" it reports:

Error: You've enabled headless mode for "none" provider but it doesn't support it.
 ❯ NoneBrowserProvider.initialize node_modules/@vitest/browser/dist/providers.js:150:13

Demonstrating the interpretation as true because "false" evals to true

martypdx commented 9 months ago

Found the issue (default: process.env.CI):

.option('--browser.headless', 'Run[ ...]t in CI, it will be enabled by default (default: process.env.CI)')

Does it work to do: default: !!process.env.CI?

sheremet-va commented 9 months ago

As I said, https://github.com/vitest-dev/vitest/pull/3983 fixes it. All we need to do is specify nested options. Currently only --browser [options] option is specified. Please stop over-analyzing it. It is a bug that we don't specify it.

martypdx commented 9 months ago

Ah, for some reason I thought that PR was already merged which was why I was confused.

I realize now it is a WIP, makes more sense 👍

hi-ogawa commented 9 months ago

From yet another cac's quirk/feature, I think it's already possible to negate it like --no-browser.headless while having { browser: { headless: true } } in the config file.

vitest-dev / vitest