-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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.
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
fix(browser): fix browser testing url for https #4855
Conversation
✅ Deploy Preview for fastidious-cascaron-4ded94 canceled.
|
@@ -143,6 +143,7 @@ jobs: | |||
strategy: | |||
matrix: | |||
browser: [[chrome, chromium], [firefox, firefox], [edge, webkit]] | |||
fail-fast: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the above test
job, I thought it's better to run all matrix without fail-fast
so that it's easier to identify potential browser-specific quirks.
// separate cacheDir from test/browser/vite.config.ts | ||
// to prevent pre-bundling related flakiness on Webkit | ||
cacheDir: path.join(path.dirname(fileURLToPath(import.meta.url)), "node_modules/.vite") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As commented in #4879 (comment), I found that webkit flakiness is related to Vite pre-bundling somehow. For now, separating vite cacheDir can workaround the issue, so I added this here.
To verify this, I repeated pnpm run test:browser:playwright
20 times consecutively on CI and it didn't fail https://github.com/vitest-dev/vitest/actions/runs/7454247103/job/20281233631#step:11:1
I'm still not sure the same issue could be reproduced outside of Vitest since this issue might be related to Vitest being a linked dep for this in-repository tests.
@@ -38,7 +38,7 @@ export function createBrowserPool(ctx: Vitest): ProcessPool { | |||
const provider = project.browserProvider! | |||
providers.add(provider) | |||
|
|||
const origin = `http://${ctx.config.browser.api?.host || 'localhost'}:${project.browser!.config.server.port}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a possibility that this might be null if server is restarted (due to a config change): vitejs/vite#15450
Does it affect us?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the reference. There could be something sketchy scenario. I'll look into it.
Also another thing, actually it might not be preferred to use resolveUrls
since it looks like Vite has some inconsistency when users set vite config like server.host: "local.example.com"
.
- Port 443 doesn't work (i.e. https://local.example.com/ does not work) vitejs/vite#11342
- read certificate and show the correct domain when startup vitejs/vite#8200
Let me think about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just remembered this issue about Vite's --open
default behavior:
Essentially Vite does a following when ViteDevServer.openBrowser
:
new URL(options.open, resolvedUrls.local[0] ?? resolvedUrls.network[0])
This also means that provider: "none"
is already forcing url origin to be resolvedUrls.local[0] ?? resolvedUrls.network[0]
, so aligning this behavior for all providers should not be that bad.
(Ah, actually that's not the case. new URL(x, y)
doesn't change origin to y
if x
already includes origin.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding Vite server's restart behavior, I don't think that's relevant for Vitest browser mode since browser mode uses a dedicated Vite dev server, which doesn't watch files on its own and is completely re-spawned when Vitest's main server restarts:
vitest/packages/vitest/src/integrations/browser/server.ts
Lines 21 to 27 in b2c79e4
// watch is handled by Vitest | |
server: { | |
hmr: false, | |
watch: { | |
ignored: ['**/**'], | |
}, | |
}, |
vitest/packages/vitest/src/node/workspace.ts
Lines 271 to 276 in b2c79e4
async initBrowserServer(configFile: string | undefined) { | |
if (!this.isBrowserEnabled()) | |
return | |
await this.browser?.close() | |
this.browser = await createBrowserServer(this, configFile) | |
} |
So, relying on resolvedUrls
should be safe before landing vitejs/vite#15450 in Vite v5.1.
Currently the log looks like this for playwright
provider:
Restarting due to config changes...
DEV v1.2.0 /home/hiroshi/code/others/vitest/test/browser/fixtures/server-url
Browser runner started at http://localhost:5174/
But webdriverio
actually doesn't support restart at all since it kills a whole process when Vitest main server tries to restart and close all existing pools:
vitest/packages/vitest/src/node/core.ts
Line 88 in b2c79e4
this.pool?.close?.() |
vitest/packages/browser/src/node/providers/webdriver.ts
Lines 93 to 95 in b2c79e4
// TODO: right now process can only exit with timeout, if we use browser | |
// needs investigating | |
process.exit() |
Description
Closes #4844
I used
ViteDevServer.resolvedUrls
to grab the url for browser testing. I think this should be always available and If this is not available, then it's probably a Vite bug, so I didn't put any fallback or validation here.On a related note, currently Vite's, so I think treatingbase
config is not supported yet (as reported in #4686), butViteDevServer.resolvedUrls
will be also generated according tobase
configresolvedUrls
as a single source of truth is appropriate for browser testing.(Actually
base
doesn't change browser testing url since it's always root/
, but I guess it would still make sense to useresolvedUrls
as a way to get origin.)Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
pnpm-lock.yaml
unless you introduce a new test example.Tests
pnpm test:ci
.Documentation
pnpm run docs
command.Changesets
feat:
,fix:
,perf:
,docs:
, orchore:
.