From 3becd2008c8898674548ac14e75c3747c9d10a54 Mon Sep 17 00:00:00 2001 From: Mike Bland Date: Wed, 6 Dec 2023 19:14:20 -0500 Subject: [PATCH 1/4] chore(test/browser): extract runVitest helper This is in preparation for adding a new test file which will run the tests with config.base set to confirm the fix for #4686. --- test/browser/specs/run-vitest.mjs | 30 +++++++++++++++++++++++++++ test/browser/specs/runner.test.mjs | 33 ++++++++---------------------- 2 files changed, 38 insertions(+), 25 deletions(-) create mode 100644 test/browser/specs/run-vitest.mjs diff --git a/test/browser/specs/run-vitest.mjs b/test/browser/specs/run-vitest.mjs new file mode 100644 index 000000000000..79d2528201a7 --- /dev/null +++ b/test/browser/specs/run-vitest.mjs @@ -0,0 +1,30 @@ +import { readFile } from 'node:fs/promises' +import { execa } from 'execa' + +const browser = process.env.BROWSER || (process.env.PROVIDER === 'playwright' ? 'chromium' : 'chrome') + +export default async function runVitest(moreArgs = []) { + const argv = ['vitest', '--run', `--browser.name=${browser}`] + + if (browser !== 'safari') + argv.push('--browser.headless') + + const { stderr, stdout } = await execa('npx', argv.concat(moreArgs), { + env: { + ...process.env, + CI: 'true', + NO_COLOR: 'true', + }, + reject: false, + }) + const browserResult = await readFile('./browser.json', 'utf-8') + const browserResultJson = JSON.parse(browserResult) + + const getPassed = results => results.filter(result => result.status === 'passed') + const getFailed = results => results.filter(result => result.status === 'failed') + + const passedTests = getPassed(browserResultJson.testResults) + const failedTests = getFailed(browserResultJson.testResults) + + return { stderr, stdout, browserResultJson, passedTests, failedTests } +} diff --git a/test/browser/specs/runner.test.mjs b/test/browser/specs/runner.test.mjs index 556b749f2a14..ea67aee7c889 100644 --- a/test/browser/specs/runner.test.mjs +++ b/test/browser/specs/runner.test.mjs @@ -1,31 +1,14 @@ import assert from 'node:assert' -import { readFile } from 'node:fs/promises' import test from 'node:test' -import { execa } from 'execa' +import runVitest from './run-vitest.mjs' -const browser = process.env.BROWSER || (process.env.PROVIDER === 'playwright' ? 'chromium' : 'chrome') -const argv = ['vitest', '--run', `--browser.name=${browser}`] - -if (browser !== 'safari') - argv.push('--browser.headless') - -const { stderr, stdout } = await execa('npx', argv, { - env: { - ...process.env, - CI: 'true', - NO_COLOR: 'true', - }, - reject: false, -}) - -const browserResult = await readFile('./browser.json', 'utf-8') -const browserResultJson = JSON.parse(browserResult) - -const getPassed = results => results.filter(result => result.status === 'passed') -const getFailed = results => results.filter(result => result.status === 'failed') - -const passedTests = getPassed(browserResultJson.testResults) -const failedTests = getFailed(browserResultJson.testResults) +const { + stderr, + stdout, + browserResultJson, + passedTests, + failedTests, +} = await runVitest() await test('tests are actually running', async () => { assert.ok(browserResultJson.testResults.length === 10, 'Not all the tests have been run') From 29b4f6647f54df2aef3067e4ae72e33e64deddb4 Mon Sep 17 00:00:00 2001 From: Mike Bland Date: Wed, 6 Dec 2023 21:43:04 -0500 Subject: [PATCH 2/4] fix(browser): handle config.base (#4686) Ensures that every import() is properly prefixed with either config.base or '/'. --- packages/browser/src/client/logger.ts | 4 ++-- packages/browser/src/client/main.ts | 13 ++++++++----- packages/browser/src/client/runner.ts | 7 ++++--- packages/browser/src/client/utils.ts | 4 ++-- test/browser/specs/fix-4686.test.mjs | 18 ++++++++++++++++++ test/browser/vitest.config-basepath.mts | 4 ++++ 6 files changed, 38 insertions(+), 12 deletions(-) create mode 100644 test/browser/specs/fix-4686.test.mjs create mode 100644 test/browser/vitest.config-basepath.mts diff --git a/packages/browser/src/client/logger.ts b/packages/browser/src/client/logger.ts index c65eaa3f3290..8a4e615f0ea4 100644 --- a/packages/browser/src/client/logger.ts +++ b/packages/browser/src/client/logger.ts @@ -3,8 +3,8 @@ import { importId } from './utils' const { Date, console } = globalThis -export async function setupConsoleLogSpy() { - const { stringify, format, inspect } = await importId('vitest/utils') as typeof import('vitest/utils') +export async function setupConsoleLogSpy(basePath: string) { + const { stringify, format, inspect } = await importId('vitest/utils', basePath) as typeof import('vitest/utils') const { log, info, error, dir, dirxml, trace, time, timeEnd, timeLog, warn, debug, count, countReset } = console const formatInput = (input: unknown) => { if (input instanceof Node) diff --git a/packages/browser/src/client/main.ts b/packages/browser/src/client/main.ts index 1e8420e0867b..61ec052e4ec7 100644 --- a/packages/browser/src/client/main.ts +++ b/packages/browser/src/client/main.ts @@ -3,7 +3,7 @@ import type { ResolvedConfig } from 'vitest' import type { CancelReason, VitestRunner } from '@vitest/runner' import type { VitestExecutor } from '../../../vitest/src/runtime/execute' import { createBrowserRunner } from './runner' -import { importId } from './utils' +import { importId as importIdImpl } from './utils' import { setupConsoleLogSpy } from './logger' import { createSafeRpc, rpc, rpcDone } from './rpc' import { setupDialogsSpy } from './dialog' @@ -24,6 +24,10 @@ const url = new URL(location.href) const testId = url.searchParams.get('id') || 'unknown' const reloadTries = Number(url.searchParams.get('reloadTries') || '0') +const basePath = () => config!.base! || '/' +const importId = (id: string) => importIdImpl(id, basePath()) +const viteClientPath = () => `${basePath()}@vite/client` + function getQueryPaths() { return url.searchParams.getAll('path') } @@ -181,15 +185,14 @@ ws.addEventListener('open', async () => { const iFrame = document.getElementById('vitest-ui') as HTMLIFrameElement iFrame.setAttribute('src', '/__vitest__/') - await setupConsoleLogSpy() + await setupConsoleLogSpy(basePath()) setupDialogsSpy() await runTests(paths, config!) }) async function prepareTestEnvironment(config: ResolvedConfig) { // need to import it before any other import, otherwise Vite optimizer will hang - const viteClientPath = '/@vite/client' - await import(viteClientPath) + await import(viteClientPath()) const { startTests, @@ -204,7 +207,7 @@ async function prepareTestEnvironment(config: ResolvedConfig) { if (!runner) { const { VitestTestRunner } = await importId('vitest/runners') as typeof import('vitest/runners') - const BrowserRunner = createBrowserRunner(VitestTestRunner, { takeCoverage: () => takeCoverageInsideWorker(config.coverage, executor) }) + const BrowserRunner = createBrowserRunner(VitestTestRunner, { takeCoverage: () => takeCoverageInsideWorker(config.coverage, executor) }, basePath()) runner = new BrowserRunner({ config, browserHashMap }) } diff --git a/packages/browser/src/client/runner.ts b/packages/browser/src/client/runner.ts index cfa79438d840..4b00a5f24c97 100644 --- a/packages/browser/src/client/runner.ts +++ b/packages/browser/src/client/runner.ts @@ -14,6 +14,7 @@ interface CoverageHandler { export function createBrowserRunner( original: { new(config: ResolvedConfig): VitestRunner }, coverageModule: CoverageHandler | null, + basePath: string, ): { new(options: BrowserRunnerOptions): VitestRunner } { return class BrowserTestRunner extends original { public config: ResolvedConfig @@ -71,9 +72,9 @@ export function createBrowserRunner( } // on Windows we need the unit to resolve the test file - const importpath = /^\w:/.test(filepath) - ? `/@fs/${filepath}?${test ? 'browserv' : 'v'}=${hash}` - : `${filepath}?${test ? 'browserv' : 'v'}=${hash}` + const prefix = `${basePath}${/^\w:/.test(filepath) ? '@fs/' : ''}` + const query = `${test ? 'browserv' : 'v'}=${hash}` + const importpath = `${prefix}${filepath}?${query}`.replace(/\/+/g, '/') await import(importpath) } } diff --git a/packages/browser/src/client/utils.ts b/packages/browser/src/client/utils.ts index 27a390af601d..a4c1a856117f 100644 --- a/packages/browser/src/client/utils.ts +++ b/packages/browser/src/client/utils.ts @@ -1,5 +1,5 @@ -export async function importId(id: string) { - const name = `/@id/${id}` +export async function importId(id: string, basePath: string) { + const name = `${basePath}@id/${id}` // @ts-expect-error mocking vitest apis return __vi_wrap_module__(import(name)) } diff --git a/test/browser/specs/fix-4686.test.mjs b/test/browser/specs/fix-4686.test.mjs new file mode 100644 index 000000000000..9caf2d4169e0 --- /dev/null +++ b/test/browser/specs/fix-4686.test.mjs @@ -0,0 +1,18 @@ +import assert from 'node:assert' +import test from 'node:test' +import runVitest from './run-vitest.mjs' + +const { + stderr, + browserResultJson, + passedTests, + failedTests, +} = await runVitest(['--config', 'vitest.config-basepath.mts']) + +await test('tests run in presence of config.base', async () => { + assert.ok(browserResultJson.testResults.length === 8, 'Not all the tests have been run') + assert.ok(passedTests.length === 7, 'Some tests failed') + assert.ok(failedTests.length === 1, 'Some tests have passed but should fail') + + assert.doesNotMatch(stderr, /Unhandled Error/, 'doesn\'t have any unhandled errors') +}) diff --git a/test/browser/vitest.config-basepath.mts b/test/browser/vitest.config-basepath.mts new file mode 100644 index 000000000000..360753c0586d --- /dev/null +++ b/test/browser/vitest.config-basepath.mts @@ -0,0 +1,4 @@ +import { defineConfig, mergeConfig } from 'vitest/config' +import baseConfig from './vitest.config.mts' + +export default mergeConfig(baseConfig, defineConfig({ base: '/fix-4686' })) From c7564418dd9498ed7c1d521aef6d3faca7dd526d Mon Sep 17 00:00:00 2001 From: Mike Bland Date: Thu, 7 Dec 2023 14:39:30 -0500 Subject: [PATCH 3/4] fix(test/browser): set test-concurrency=1 (#4686) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adding the fix-4686.test.mjs introduced a second `vitest` process into the test/browser/specs suite. This attempts to resolve subsequent flakiness in the test-browser CI job by ensuring one test file completely finishes before the next. - https://nodejs.org/docs/latest-v20.x/api/cli.html#--test-concurrency --- This particular build showed ETXTBSY errors thrown when launching the chrome via WebdriverIO: - https://github.com/vitest-dev/vitest/actions/runs/7128065730/job/19409296285?pr=4692#step:10:1 ```text ⎯⎯⎯⎯⎯⎯ Unhandled Error ⎯⎯⎯⎯⎯⎯⎯ Error: spawn ETXTBSY ❯ ChildProcess.spawn node:internal/child_process:421:11 ❯ Object.spawn node:child_process:761:9 ❯ startWebDriver ../../node_modules/.pnpm/@wdio+utils@8.22.0/node_modules/@wdio/utils/build/node/startWebDriver.js:57:28 ❯ process.processTicksAndRejections node:internal/process/task_queues:95:5 ❯ WebDriver.newSession ../../node_modules/.pnpm/webdriver@8.22.1/node_modules/webdriver/build/index.js:18:31 ❯ remote ../../node_modules/.pnpm/webdriverio@8.22.1_typescript@5.2.2/node_modules/webdriverio/build/index.js:45:22 ❯ WebdriverBrowserProvider.openBrowser ../../packages/browser/dist/providers.js:81:26 [...snip...] ``` ETXTBSY is a relatively obscure error that isn't seen much anymore: - lwn.net: The shrinking role of ETXTBSY https://lwn.net/Articles/866493/ The results I found while searching all seemed to point to Chromium being a potential source of this issue, such as this excerpt from (emphasis mine): - https://github.com/alixaxel/chrome-aws-lambda/issues/69#issuecomment-584678457 ```text The `ETXTBSY` error code is thrown when puppeteer is trying to **start the Chromium binary but the binary file is under a exclusive lock** - that can happen if you don't wait for `executablePath` to resolve, or, **if you call `executablePath` multiple times.** ``` This also showed up with older versions of Node and Docker, but each of the versions in the current build are up to date. --- test/browser/package.json | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/browser/package.json b/test/browser/package.json index a190c82e6808..5131b1c5c03c 100644 --- a/test/browser/package.json +++ b/test/browser/package.json @@ -4,9 +4,9 @@ "private": true, "scripts": { "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/", + "test:webdriverio": "PROVIDER=webdriverio node --test --test-concurrency=1 specs/", + "test:playwright": "PROVIDER=playwright node --test --test-concurrency=1 specs/", + "test:safaridriver": "PROVIDER=webdriverio BROWSER=safari node --test --test-concurrency=1 specs/", "coverage": "vitest --coverage.enabled --coverage.provider=istanbul --browser.headless=yes" }, "devDependencies": { From f5f3251b6c6ae881ae19aee65bc102efd6943ccb Mon Sep 17 00:00:00 2001 From: Mike Bland Date: Wed, 3 Jan 2024 17:20:01 -0500 Subject: [PATCH 4/4] refactor:cleanup Originally by @sheremet-va in commit f077ec4ed49d3ad41504a8c4dccfd2c079051496. Co-authored-by: Vladimir Sheremet --- packages/browser/src/client/main.ts | 8 ++++---- packages/browser/src/client/runner.ts | 8 ++++---- test/browser/specs/fix-4686.test.mjs | 10 ++++++---- test/browser/vitest.config-basepath.mts | 2 +- 4 files changed, 15 insertions(+), 13 deletions(-) diff --git a/packages/browser/src/client/main.ts b/packages/browser/src/client/main.ts index 61ec052e4ec7..d0a9e029a352 100644 --- a/packages/browser/src/client/main.ts +++ b/packages/browser/src/client/main.ts @@ -3,7 +3,7 @@ import type { ResolvedConfig } from 'vitest' import type { CancelReason, VitestRunner } from '@vitest/runner' import type { VitestExecutor } from '../../../vitest/src/runtime/execute' import { createBrowserRunner } from './runner' -import { importId as importIdImpl } from './utils' +import { importId as _importId } from './utils' import { setupConsoleLogSpy } from './logger' import { createSafeRpc, rpc, rpcDone } from './rpc' import { setupDialogsSpy } from './dialog' @@ -24,8 +24,8 @@ const url = new URL(location.href) const testId = url.searchParams.get('id') || 'unknown' const reloadTries = Number(url.searchParams.get('reloadTries') || '0') -const basePath = () => config!.base! || '/' -const importId = (id: string) => importIdImpl(id, basePath()) +const basePath = () => config?.base || '/' +const importId = (id: string) => _importId(id, basePath()) const viteClientPath = () => `${basePath()}@vite/client` function getQueryPaths() { @@ -207,7 +207,7 @@ async function prepareTestEnvironment(config: ResolvedConfig) { if (!runner) { const { VitestTestRunner } = await importId('vitest/runners') as typeof import('vitest/runners') - const BrowserRunner = createBrowserRunner(VitestTestRunner, { takeCoverage: () => takeCoverageInsideWorker(config.coverage, executor) }, basePath()) + const BrowserRunner = createBrowserRunner(VitestTestRunner, { takeCoverage: () => takeCoverageInsideWorker(config.coverage, executor) }) runner = new BrowserRunner({ config, browserHashMap }) } diff --git a/packages/browser/src/client/runner.ts b/packages/browser/src/client/runner.ts index 4b00a5f24c97..b43facae8f2c 100644 --- a/packages/browser/src/client/runner.ts +++ b/packages/browser/src/client/runner.ts @@ -12,11 +12,10 @@ interface CoverageHandler { } export function createBrowserRunner( - original: { new(config: ResolvedConfig): VitestRunner }, + VitestRunner: { new(config: ResolvedConfig): VitestRunner }, coverageModule: CoverageHandler | null, - basePath: string, ): { new(options: BrowserRunnerOptions): VitestRunner } { - return class BrowserTestRunner extends original { + return class BrowserTestRunner extends VitestRunner { public config: ResolvedConfig hashMap = new Map() @@ -70,9 +69,10 @@ export function createBrowserRunner( hash = Date.now().toString() this.hashMap.set(filepath, [false, hash]) } + const base = this.config.base || '/' // on Windows we need the unit to resolve the test file - const prefix = `${basePath}${/^\w:/.test(filepath) ? '@fs/' : ''}` + const prefix = `${base}${/^\w:/.test(filepath) ? '@fs/' : ''}` const query = `${test ? 'browserv' : 'v'}=${hash}` const importpath = `${prefix}${filepath}?${query}`.replace(/\/+/g, '/') await import(importpath) diff --git a/test/browser/specs/fix-4686.test.mjs b/test/browser/specs/fix-4686.test.mjs index 9caf2d4169e0..d95964ef866e 100644 --- a/test/browser/specs/fix-4686.test.mjs +++ b/test/browser/specs/fix-4686.test.mjs @@ -1,3 +1,5 @@ +// fix #4686 + import assert from 'node:assert' import test from 'node:test' import runVitest from './run-vitest.mjs' @@ -7,12 +9,12 @@ const { browserResultJson, passedTests, failedTests, -} = await runVitest(['--config', 'vitest.config-basepath.mts']) +} = await runVitest(['--config', 'vitest.config-basepath.mts', 'basic.test.ts']) await test('tests run in presence of config.base', async () => { - assert.ok(browserResultJson.testResults.length === 8, 'Not all the tests have been run') - assert.ok(passedTests.length === 7, 'Some tests failed') - assert.ok(failedTests.length === 1, 'Some tests have passed but should fail') + assert.ok(browserResultJson.testResults.length === 1, 'Not all the tests have been run') + assert.ok(passedTests.length === 1, 'Some tests failed') + assert.ok(failedTests.length === 0, 'Some tests have passed but should fail') assert.doesNotMatch(stderr, /Unhandled Error/, 'doesn\'t have any unhandled errors') }) diff --git a/test/browser/vitest.config-basepath.mts b/test/browser/vitest.config-basepath.mts index 360753c0586d..b772007fc8b8 100644 --- a/test/browser/vitest.config-basepath.mts +++ b/test/browser/vitest.config-basepath.mts @@ -1,4 +1,4 @@ import { defineConfig, mergeConfig } from 'vitest/config' -import baseConfig from './vitest.config.mts' +import baseConfig from './vitest.config.mjs' export default mergeConfig(baseConfig, defineConfig({ base: '/fix-4686' }))