From 0e666c1972af92d513145c0fcd8d09e2210d497f Mon Sep 17 00:00:00 2001 From: Vladimir Sheremet Date: Thu, 8 Oct 2026 16:46:14 +0200 Subject: [PATCH] fix(browser): settle the run after every instance - the pool run settles only when every instance settled, a failing page no longer ends the files of its siblings - prefer queued tasks whose instance is still open, so a rerun reopens one instance instead of all of them - type `project.browser.provider` as optional, it is unset after an eviction - a page that opens while the provider closes disposes only itself instead of clearing the shared maps --- packages/browser-playwright/src/playwright.ts | 4 - packages/browser/src/node/project.ts | 8 +- packages/browser/src/node/rpc.ts | 12 +- packages/browser/src/node/serverTester.ts | 2 +- packages/vitest/src/node/pools/browser.ts | 94 ++++++++++---- packages/vitest/src/node/project.ts | 5 +- packages/vitest/src/node/types/browser.ts | 6 +- .../specs/instance-concurrency.test.ts | 120 ++++++++++++++++++ 8 files changed, 208 insertions(+), 43 deletions(-) diff --git a/packages/browser-playwright/src/playwright.ts b/packages/browser-playwright/src/playwright.ts index 63b7bff0e..5251b6c02 100644 --- a/packages/browser-playwright/src/playwright.ts +++ b/packages/browser-playwright/src/playwright.ts @@ -789,10 +789,6 @@ export class PlaywrightBrowserProvider implements BrowserProvider { String(disposable), ) await disposable?.close() - this.pages.clear() - this.contexts.clear() - this.browser = null - this.browserPromise = null throw new Error(`[vitest] The provider was closed.`) } } diff --git a/packages/browser/src/node/project.ts b/packages/browser/src/node/project.ts index 12e138b3b..c41a2cec2 100644 --- a/packages/browser/src/node/project.ts +++ b/packages/browser/src/node/project.ts @@ -28,7 +28,7 @@ export class ProjectBrowser implements IProjectBrowser { // transformIndexHtml plugin pipeline each time public testerHtmlTransformed: Promise | undefined - public provider!: BrowserProvider + public provider: BrowserProvider | undefined public vitest: Vitest public vite: ViteDevServer public config: ResolvedConfig @@ -81,7 +81,9 @@ export class ProjectBrowser implements IProjectBrowser { if (name in this.parent.commands) { return this.parent.commands[name](context, ...args) } - throw new Error(`Provider ${this.provider.name} does not support command "${name}".`) + throw new Error( + `Provider ${this.config.browser.provider?.name} does not support command "${name}".`, + ) }) as any wrapSerializedConfig(): SerializedConfig { @@ -114,7 +116,7 @@ export class ProjectBrowser implements IProjectBrowser { return } // detach first so the next run can create a new provider while this one closes - this.provider = undefined as any + this.provider = undefined await provider.close(options) } diff --git a/packages/browser/src/node/rpc.ts b/packages/browser/src/node/rpc.ts index 069a5227b..547bbc62c 100644 --- a/packages/browser/src/node/rpc.ts +++ b/packages/browser/src/node/rpc.ts @@ -240,7 +240,7 @@ export function setupBrowserRpc( const mockResolver = new ServerMockResolver(globalServer.vite, { moduleDirectories: project.config?.deps?.moduleDirectories, }) - const mocker = project.browser?.provider.mocker + const mocker = project.browser?.provider?.mocker const rpc = createBirpc( { @@ -420,9 +420,9 @@ export function setupBrowserRpc( return vitest.state.getCountOfFailedTests() }, async wdioSwitchContext(direction) { - const provider = project.browser!.provider + const provider = project.browser?.provider if (!provider) { - throw new Error('Commands are only available for browser tests.') + throw new Error(`The browser of the "${project.name}" project is closed.`) } if (provider.name !== 'webdriverio') { throw new Error('Switch context is only available for WebDriverIO provider.') @@ -435,9 +435,11 @@ export function setupBrowserRpc( }, async triggerCommand(sessionId, command, testPath, payload) { debug?.('[%s] Triggering command "%s"', sessionId, command) - const provider = project.browser!.provider + const provider = project.browser?.provider if (!provider) { - throw new Error('Commands are only available for browser tests.') + throw new Error( + `Cannot run the "${command}" command because the browser of the "${project.name}" project is closed.`, + ) } const context = Object.assign( { diff --git a/packages/browser/src/node/serverTester.ts b/packages/browser/src/node/serverTester.ts index 1adde689d..25c88cd26 100644 --- a/packages/browser/src/node/serverTester.ts +++ b/packages/browser/src/node/serverTester.ts @@ -47,7 +47,7 @@ export async function resolveTester( : await globalServer.injectorJs const injector = replacer(injectorJs, { - __VITEST_PROVIDER__: JSON.stringify(project.browser!.provider.name), + __VITEST_PROVIDER__: JSON.stringify(browserProject.config.browser.provider?.name || 'preview'), __VITEST_CONFIG__: JSON.stringify(browserProject.wrapSerializedConfig()), __VITEST_VITE_CONFIG__: JSON.stringify({ root: browserProject.vite.config.root, diff --git a/packages/vitest/src/node/pools/browser.ts b/packages/vitest/src/node/pools/browser.ts index fbd595432..9073f42b5 100644 --- a/packages/vitest/src/node/pools/browser.ts +++ b/packages/vitest/src/node/pools/browser.ts @@ -48,7 +48,7 @@ export function createBrowserPool(vitest: Vitest): BrowserPool { function getThreadsCount(project: TestProject) { const config = project.config.browser - if (!config.headless || !project.browser!.provider.supportsParallelism) { + if (!config.headless || !project.browser?.provider?.supportsParallelism) { return 1 } @@ -103,12 +103,24 @@ export function createBrowserPool(vitest: Vitest): BrowserPool { return false } + // an instance that still has an idle browser runs first, so a slot + // is not freed by closing the browser of an instance that runs next + function nextTaskIndex(): number { + const head = queue[0] + const index = queue.findIndex((task) => { + const worker = workers.get(task.project) + return task.exclusive === head.exclusive && !!worker && !activeWorkers.has(worker) + }) + return index === -1 ? 0 : index + } + function schedule(): void { while (queue.length) { - const task = queue[0] if (exclusiveRunning || activeWorkers.size >= maxWorkers) { return } + const index = nextTaskIndex() + const task = queue[index] if (task.exclusive && activeWorkers.size > 0) { return } @@ -134,7 +146,7 @@ export function createBrowserPool(vitest: Vitest): BrowserPool { workers.set(task.project, worker) } - queue.shift() + queue.splice(index, 1) activeWorkers.add(worker) exclusiveRunning = task.exclusive @@ -185,9 +197,10 @@ export function createBrowserPool(vitest: Vitest): BrowserPool { async ([project, files]): Promise => { await project._initBrowserProvider() - if (!project.browser) { + const provider = project.browser?.provider + if (!provider) { throw new TypeError( - `The browser server was not initialized${project.name ? ` for the "${project.name}" project` : ''}. This is a bug in Vitest. Please, open a new issue with reproduction.`, + `The browser was not initialized${project.name ? ` for the "${project.name}" project` : ''}. This is a bug in Vitest. Please, open a new issue with reproduction.`, ) } @@ -202,7 +215,6 @@ export function createBrowserPool(vitest: Vitest): BrowserPool { files.map((f) => f.filepath), ) - const provider = project.browser.provider return { project, files, @@ -236,7 +248,16 @@ export function createBrowserPool(vitest: Vitest): BrowserPool { queue.push(...ordered) schedule() - await Promise.all(ordered.map((task) => task.result)) + const results = await Promise.allSettled(ordered.map((task) => task.result)) + const errors = results + .filter((result) => result.status === 'rejected') + .map((result) => result.reason) + if (errors.length === 1) { + throw errors[0] + } + if (errors.length > 1) { + throw new AggregateError(errors, 'Errors occurred while running browser tests.') + } } async function close() { @@ -321,6 +342,7 @@ class BrowserWorker { private _queue: FileSpecification[] = [] private _method: 'run' | 'collect' = 'run' private _promise: DeferPromise | undefined + private _error: Error | undefined private _providedContext: string | undefined // every page this worker opened or adopted, busy ones are running a file @@ -359,16 +381,32 @@ class BrowserWorker { this._otel.span.end() } - public reject(error: Error): void { + public reject(error: Error, sessionId?: string): void { // if user cancels the test run manually, ignore the error and exit gracefully - if (this.project.vitest.isCancelling && error instanceof BrowserConnectionError) { - this._promise?.resolve() - } else { - this._promise?.reject(error) + if (!this.project.vitest.isCancelling || !(error instanceof BrowserConnectionError)) { + this._error ??= error + } + if (sessionId) { + this.busySessions.delete(sessionId) } - this._promise = undefined - this.busySessions.clear() this.cancel() + // other pages finish the file they are running first + if (!this.busySessions.size) { + this.settle() + } + } + + private settle(): void { + const promise = this._promise + const error = this._error + this._promise = undefined + this._error = undefined + this._otel.span.end() + if (error) { + promise?.reject(error) + } else { + promise?.resolve() + } } get orchestrators() { @@ -425,7 +463,7 @@ class BrowserWorker { this.runNextTest(method, sessionId) } - await this.openPages() + await this.openPages().catch((error) => this.reject(error)) debug?.('all sessions are created') return promise } @@ -524,7 +562,7 @@ class BrowserWorker { private async openPage(sessionId: string, options: { parallel: boolean }): Promise { await this.project._openBrowserPage(sessionId, { - reject: (error) => this.reject(error), + reject: (error) => this.reject(error, sessionId), parallel: options.parallel, }) } @@ -572,9 +610,7 @@ class BrowserWorker { // the last page finished running tests if (!this.busySessions.size) { - this._otel.span.end() - this._promise?.resolve() - this._promise = undefined + this.settle() debug?.('[%s] all tests finished running', sessionId) } else { debug?.( @@ -661,18 +697,18 @@ class BrowserWorker { .catch((error) => { // if user cancels the test run manually, ignore the error and exit gracefully if (this.project.vitest.isCancelling && error instanceof BrowserConnectionError) { - this.cancel() - this._promise?.resolve() - this._promise = undefined - this.busySessions.clear() debug?.('[%s] browser connection was closed', sessionId) + this.reject(error, sessionId) return } debug?.('[%s] error during %s test run: %s', sessionId, file, error) - this.reject(new Error(`Failed to run the test ${file.filepath}.`, { cause: error })) + this.reject( + new Error(`Failed to run the test ${file.filepath}.`, { cause: error }), + sessionId, + ) }) }) - .catch((err) => this.reject(err)) + .catch((err) => this.reject(err, sessionId)) } async setBreakpoint(sessionId: string, file: string) { @@ -680,7 +716,10 @@ class BrowserWorker { return } - const provider = this.project.browser!.provider + const provider = this.project.browser?.provider + if (!provider) { + return + } const browser = this.project.config.browser.name if (shouldIgnoreDebugger(provider.name, browser)) { @@ -724,8 +763,9 @@ const debugGC = createDebugger('vitest:browser:gc') async function maybeCollectChromiumGarbage(project: TestProject, sessionId: string): Promise { // trigger only on linux/chromium/playwright - const provider = project.browser!.provider + const provider = project.browser?.provider if ( + !provider || (!forceChromiumGC && process.platform !== 'linux') || provider.name !== 'playwright' || project.config.browser.name !== 'chromium' || diff --git a/packages/vitest/src/node/project.ts b/packages/vitest/src/node/project.ts index 9c1555d71..3606c42df 100644 --- a/packages/vitest/src/node/project.ts +++ b/packages/vitest/src/node/project.ts @@ -473,7 +473,8 @@ export class TestProject { parallel?: boolean }, ): Promise { - if (!this.browser) { + const provider = this.browser?.provider + if (!this.browser || !provider) { throw new Error(`browser is not initialized`) } @@ -490,7 +491,7 @@ export class TestProject { const sessionPromise = this.vitest._browserSessions.createSession(sessionId, this, pool, { otelCarrier, }) - const pagePromise = this.browser.provider.openPage(sessionId, url.toString(), { + const pagePromise = provider.openPage(sessionId, url.toString(), { parallel: pool.parallel ?? false, }) await Promise.all([sessionPromise, pagePromise]) diff --git a/packages/vitest/src/node/types/browser.ts b/packages/vitest/src/node/types/browser.ts index 8a8e3fa51..bcccf03ce 100644 --- a/packages/vitest/src/node/types/browser.ts +++ b/packages/vitest/src/node/types/browser.ts @@ -466,7 +466,11 @@ export interface BrowserServerContribution { export interface ProjectBrowser { vite: ViteDevServer state: BrowserServerState - provider: BrowserProvider + /** + * The provider of the open browser instance. It is `undefined` until the instance + * starts running tests and after the instance is closed to free a worker slot. + */ + provider: BrowserProvider | undefined close: () => Promise initBrowserProvider: (project: TestProject) => Promise closeBrowserProvider: (options?: BrowserProviderCloseOptions) => Promise diff --git a/test/browser/specs/instance-concurrency.test.ts b/test/browser/specs/instance-concurrency.test.ts index 82687170e..9b8b73364 100644 --- a/test/browser/specs/instance-concurrency.test.ts +++ b/test/browser/specs/instance-concurrency.test.ts @@ -297,3 +297,123 @@ test.runIf(provider.name === 'playwright')( `) }, ) + +test.runIf(provider.name === 'playwright')( + 'finishes the run after every instance, even when one of them fails', + async () => { + const events: string[] = [] + + const { ctx, root, testTree } = await runInlineBrowserTests( + { + 'failing.test.ts': ` + import { test } from 'vitest' + import { commands } from 'vitest/browser' + test('closes its own page', async () => { + await commands.closePage() + }) + `, + 'slow.test.ts': ` + import { test } from 'vitest' + test('runs in slow', async () => { + await new Promise(resolve => setTimeout(resolve, 500)) + }) + `, + }, + { + maxWorkers: 2, + browser: { + instances: [ + { browser, name: 'failing', include: ['failing.test.ts'] }, + { browser, name: 'slow', include: ['slow.test.ts'] }, + ], + commands: { + async closePage(context) { + await context.page.close() + }, + }, + }, + reporters: [ + { + onTestModuleEnd(module: TestModule) { + events.push(`end ${module.project.name}`) + }, + onTestRunEnd() { + events.push('run end') + }, + }, + ], + }, + ) + + expect(events).toEqual(['end slow', 'run end']) + expect(testTree()).toMatchInlineSnapshot(` + { + "failing.test.ts": { + "closes its own page": "pending", + }, + "slow.test.ts": { + "runs in slow": "passed", + }, + } + `) + expect( + ctx!.state.getUnhandledErrors().map((error: any) => error.message.replace(root, '')), + ).toMatchInlineSnapshot(` + [ + "Failed to run the test /failing.test.ts.", + ] + `) + }, +) + +test.runIf(provider.name === 'playwright')( + 'reuses the pages of instances that stayed open on rerun', + async () => { + const files: Record = { + 'label.ts': `export const label = 'label'`, + } + for (const name of names) { + files[`${name}.test.ts`] = ` + import { test } from 'vitest' + import { commands } from 'vitest/browser' + import { label } from './label' + test('records the page in ${name}', async () => { + await commands.recordSession(label) + }) + ` + } + const runs: Record[] = [] + + const { fs, vitest } = await runInlineBrowserTests(files, { + watch: true, + maxWorkers: 2, + browser: { + instances: createInstances(), + commands: { + recordSession(context) { + runs.at(-1)![context.project.name] = context.sessionId + }, + }, + }, + reporters: [ + 'default', + { + onTestRunStart() { + runs.push({}) + }, + }, + ], + }) + + await vitest.waitForStdout(`Test Files ${names.length} passed`) + + vitest.resetOutput() + fs.editFile('label.ts', (content) => `${content}\n`) + await vitest.waitForStdout(`Test Files ${names.length} passed`) + + expect(vitest.stderr).toBe('') + const [first, second] = runs + // the closed instance takes the slot of whichever open instance finishes first + expect(names.filter((name) => first[name] === second[name])).toHaveLength(2) + }, +) -- 2.54.0