Skip to content

Commit

Permalink
fix(browser): avoid safaridriver collision
Browse files Browse the repository at this point in the history
Removes code from node/providers/webdriver.ts causing the following
error when running `vitest --browser.name=safari` on macOS:

  Error: There is already a Safaridriver instance running on port
  undefined!

safaridriver.start() maintains a static reference to a safaridriver
instance, and will fail with this error if start() is called again
before stop().

The original node/providers/webdriver.ts calls safaridriver.start()
directly, and then immediately imports and calls webdriverio.remote().
This call also eventually calls safaridriver.start(), producing the
error.

The fix is to remove the direct safaridriver access, as webdriverio
seems to manage safaridriver successfully now.

---

Testing:

Since Safari can't run in headless mode, I couldn't think of a way to
add a test that would run in CI. However, I added a new
test:safaridriver target intended to be run manually as needed, which
runs all the existing test/browser tests under Safari.

---

One extra observation:

Command line options like --browser.headless=false are _not_ converted
to boolean values; they remain string values.

This is why the `--browser.headless` argument is now pushed onto the
extracted argv[], after I had originally tried updating the existing
argument string to:

  `--browser.headless=${browser !== 'safari'}`

When I updated the `pnpm:safaridriver` target with `PROVIDER=playwright`
to see what would happen (even though this is only a webdriver change),
it produced this error:

  Error: browserType.launch: headless: expected boolean, got string
	 ❯ PlaywrightBrowserProvider.openBrowserPage
     ../../packages/browser/dist/providers.js:22:52
       20|     const options = this.ctx.config.browser;
       21|     const playwright = await import('playwright');
			 22|     const browser = await playwright[this.browser].launch({
         |                                                    ^
       23|       ...this.options?.launch,
       24|       headless: options.headless

You can actually see this by running:

  npx vitest --run --browser.name=webkit --browser.provider=playwright
    --browser.headless=false

This seems like a bug worth filing, but not addressing in this change.
  • Loading branch information
mbland committed Jan 3, 2024
1 parent 1efc29b commit 86de156
Show file tree
Hide file tree
Showing 4 changed files with 6 additions and 12 deletions.
1 change: 0 additions & 1 deletion packages/browser/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@
},
"peerDependencies": {
"playwright": "*",
"safaridriver": "*",
"vitest": "^1.0.0",
"webdriverio": "*"
},
Expand Down
10 changes: 0 additions & 10 deletions packages/browser/src/node/providers/webdriver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ export class WebdriverBrowserProvider implements BrowserProvider {
public name = 'webdriverio'

private cachedBrowser: WebdriverIO.Browser | null = null
private stopSafari: () => void = () => {}
private browser!: WebdriverBrowser
private ctx!: WorkspaceProject

Expand All @@ -39,14 +38,6 @@ export class WebdriverBrowserProvider implements BrowserProvider {
if (this.browser === 'safari') {
if (options.headless)
throw new Error('You\'ve enabled headless mode for Safari but it doesn\'t currently support it.')

const safaridriver = await import('safaridriver')
safaridriver.start({ diagnose: true })
this.stopSafari = () => safaridriver.stop()

process.on('beforeExit', () => {
safaridriver.stop()
})
}

const { remote } = await import('webdriverio')
Expand Down Expand Up @@ -97,7 +88,6 @@ export class WebdriverBrowserProvider implements BrowserProvider {

async close() {
await Promise.all([
this.stopSafari(),
this.cachedBrowser?.sessionId ? this.cachedBrowser?.deleteSession?.() : null,
])
// TODO: right now process can only exit with timeout, if we use browser
Expand Down
1 change: 1 addition & 0 deletions test/browser/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
"test": "pnpm run test:webdriverio && pnpm run test:playwright",
"test:webdriverio": "PROVIDER=webdriverio node --test specs/",
"test:playwright": "PROVIDER=playwright node --test specs/",
"test:safaridriver": "PROVIDER=webdriverio BROWSER=safari node --test specs/",
"coverage": "vitest --coverage.enabled --coverage.provider=istanbul --browser.headless=yes"
},
"devDependencies": {
Expand Down
6 changes: 5 additions & 1 deletion test/browser/specs/runner.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,12 @@ import test from 'node:test'
import { execa } from 'execa'

const browser = process.env.BROWSER || (process.env.PROVIDER === 'playwright' ? 'chromium' : 'chrome')
const argv = ['vitest', '--run', `--browser.name=${browser}`]

const { stderr, stdout } = await execa('npx', ['vitest', '--run', `--browser.name=${browser}`, '--browser.headless'], {
if (browser !== 'safari')
argv.push('--browser.headless')

const { stderr, stdout } = await execa('npx', argv, {
env: {
...process.env,
CI: 'true',
Expand Down

0 comments on commit 86de156

Please sign in to comment.