fix(browser): avoid safaridriver collision #4863
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
✅ Deploy Preview for fastidious-cascaron-4ded94 canceled.
|
6 tasks
sheremet-va
approved these changes
Jan 4, 2024
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixes #4706.
Removes code from node/providers/webdriver.ts causing the following error when running
vitest --browser.name=safarion macOS:Cause
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:safaridrivertarget intended to be run manually as needed, which runs all the existing test/browser tests under Safari.Enabling/documentation
Note that you have to enable automation in Safari via Developer > Allow remote automation, or else running with the safaridriver will fail with:
If it would be helpful to add a doc change for this, or to open a new PR for one, I'm happy to do that.
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
pnpm-lock.yamlunless you introduce a new test example.Tests
pnpm test:ci.Documentation
pnpm run docscommand.Changesets
feat:,fix:,perf:,docs:, orchore:.