From 90487944e326ee27b5533ed9fc75853eb5d5e026 Mon Sep 17 00:00:00 2001 From: Gert Hengeveld Date: Fri, 6 Dec 2024 10:20:05 +0100 Subject: [PATCH 1/8] Queue up events while Vitest is booting, and resend them as soon as Vitest is ready --- code/addons/test/src/node/boot-test-runner.ts | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/code/addons/test/src/node/boot-test-runner.ts b/code/addons/test/src/node/boot-test-runner.ts index 9cf4880ec35e..664117f8ea18 100644 --- a/code/addons/test/src/node/boot-test-runner.ts +++ b/code/addons/test/src/node/boot-test-runner.ts @@ -23,10 +23,13 @@ const MAX_START_TIME = 30000; // which is at the root. Then, from the root, we want to load `node/vitest.mjs` const vitestModulePath = join(__dirname, 'node', 'vitest.mjs'); +// Events that were triggered before Vitest was ready are queued up and resent once it's ready +const eventQueue: { type: string; args: any[] }[] = []; + let child: null | ChildProcess; let ready = false; -const bootTestRunner = async (channel: Channel, initEvent?: string, initArgs?: any[]) => { +const bootTestRunner = async (channel: Channel) => { let stderr: string[] = []; function reportFatalError(e: any) { @@ -81,9 +84,10 @@ const bootTestRunner = async (channel: Channel, initEvent?: string, initArgs?: a child.on('message', (result: any) => { if (result.type === 'ready') { - // Resend the event that triggered the boot sequence, now that the child is ready to handle it - if (initEvent && initArgs) { - child?.send({ type: initEvent, args: initArgs, from: 'server' }); + // Resend events that triggered (during) the boot sequence, now that Vitest is ready + while (eventQueue.length) { + const { type, args } = eventQueue.shift(); + child?.send({ type, args, from: 'server' }); } // Forward all events from the channel to the child process @@ -129,9 +133,12 @@ const bootTestRunner = async (channel: Channel, initEvent?: string, initArgs?: a }; export const runTestRunner = async (channel: Channel, initEvent?: string, initArgs?: any[]) => { + if (!ready && initEvent) { + eventQueue.push({ type: initEvent, args: initArgs }); + } if (!child) { ready = false; - await bootTestRunner(channel, initEvent, initArgs); + await bootTestRunner(channel); ready = true; } }; From d86f262c53fa94470f60e6dedf4fca2418b11b73 Mon Sep 17 00:00:00 2001 From: Gert Hengeveld Date: Fri, 6 Dec 2024 10:21:43 +0100 Subject: [PATCH 2/8] Move restartVitest to VitestManager and wait for restarts to finish before running tests --- code/addons/test/src/node/test-manager.ts | 23 +++++++++++---------- code/addons/test/src/node/vitest-manager.ts | 19 +++++++++++++++++ 2 files changed, 31 insertions(+), 11 deletions(-) diff --git a/code/addons/test/src/node/test-manager.ts b/code/addons/test/src/node/test-manager.ts index 139fffa3ef6b..39ba575d1518 100644 --- a/code/addons/test/src/node/test-manager.ts +++ b/code/addons/test/src/node/test-manager.ts @@ -39,12 +39,6 @@ export class TestManager { this.vitestManager.startVitest().then(() => options.onReady?.()); } - async restartVitest({ watchMode, coverage }: { watchMode: boolean; coverage: boolean }) { - await this.vitestManager.vitest?.runningPromise; - await this.vitestManager.closeVitest(); - await this.vitestManager.startVitest({ watchMode, coverage }); - } - async handleConfigChange( payload: TestingModuleConfigChangePayload<{ coverage: boolean; a11y: boolean }> ) { @@ -54,7 +48,10 @@ export class TestManager { if (this.coverage !== payload.config.coverage) { try { this.coverage = payload.config.coverage; - await this.restartVitest({ watchMode: this.watchMode, coverage: this.coverage }); + await this.vitestManager.restartVitest({ + watchMode: this.watchMode, + coverage: this.coverage, + }); } catch (e) { const isV8 = e.message?.includes('@vitest/coverage-v8'); const isIstanbul = e.message?.includes('@vitest/coverage-istanbul'); @@ -76,7 +73,7 @@ export class TestManager { if (this.watchMode !== payload.watchMode) { this.watchMode = payload.watchMode; - await this.restartVitest({ watchMode: this.watchMode, coverage: false }); + await this.vitestManager.restartVitest({ watchMode: this.watchMode, coverage: false }); } } catch (e) { this.reportFatalError('Failed to change watch mode', e); @@ -99,21 +96,25 @@ export class TestManager { we have to restart Vitest AND disable watch mode otherwise the coverage report will be incorrect, Vitest behaves wonky when re-using the same Vitest instance but with watch mode disabled, among other things it causes the coverage report to be incorrect and stale. - + If we're only running a subset of stories, we have to temporarily disable coverage, as a coverage report for a subset of stories is not useful. */ - await this.restartVitest({ + this.vitestManager.restartVitest({ watchMode: allTestsRun ? false : this.watchMode, coverage: allTestsRun, }); } + await this.vitestManager.vitestRestartPromise; await this.vitestManager.runTests(payload); if (this.coverage && !allTestsRun) { // Re-enable coverage if it was temporarily disabled because of a subset of stories was run - await this.restartVitest({ watchMode: this.watchMode, coverage: this.coverage }); + await this.vitestManager.restartVitest({ + watchMode: this.watchMode, + coverage: this.coverage, + }); } } catch (e) { this.reportFatalError('Failed to run tests', e); diff --git a/code/addons/test/src/node/vitest-manager.ts b/code/addons/test/src/node/vitest-manager.ts index 0296945b43c9..eeb9f4726a90 100644 --- a/code/addons/test/src/node/vitest-manager.ts +++ b/code/addons/test/src/node/vitest-manager.ts @@ -34,6 +34,8 @@ export class VitestManager { vitestStartupCounter = 0; + vitestRestartPromise: Promise | null = null; + storyCountForCurrentRun: number = 0; constructor(private testManager: TestManager) {} @@ -99,6 +101,21 @@ export class VitestManager { } } + async restartVitest({ watchMode, coverage }: { watchMode: boolean; coverage: boolean }) { + await this.vitestRestartPromise; + this.vitestRestartPromise = new Promise(async (resolve, reject) => { + try { + await this.vitest?.runningPromise; + await this.closeVitest(); + await this.startVitest({ watchMode, coverage }); + resolve(); + } catch (e) { + reject(e); + } + }); + return this.vitestRestartPromise; + } + private updateLastChanged(filepath: string) { const projects = this.vitest!.getModuleProjects(filepath); projects.forEach(({ server, browser }) => { @@ -148,6 +165,8 @@ export class VitestManager { async runTests(requestPayload: TestingModuleRunRequestPayload) { if (!this.vitest) { await this.startVitest(); + } else { + await this.vitestRestartPromise; } this.resetTestNamePattern(); From ef7a88983d80560173a1199c334f64abd649ac6c Mon Sep 17 00:00:00 2001 From: Gert Hengeveld Date: Fri, 6 Dec 2024 10:22:22 +0100 Subject: [PATCH 3/8] Handle potentially undefined value --- code/addons/test/src/node/vitest-manager.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/code/addons/test/src/node/vitest-manager.ts b/code/addons/test/src/node/vitest-manager.ts index eeb9f4726a90..78b527c615a9 100644 --- a/code/addons/test/src/node/vitest-manager.ts +++ b/code/addons/test/src/node/vitest-manager.ts @@ -119,9 +119,10 @@ export class VitestManager { private updateLastChanged(filepath: string) { const projects = this.vitest!.getModuleProjects(filepath); projects.forEach(({ server, browser }) => { - const serverMods = server.moduleGraph.getModulesByFile(filepath); - serverMods?.forEach((mod) => server.moduleGraph.invalidateModule(mod)); - + if (server) { + const serverMods = server.moduleGraph.getModulesByFile(filepath); + serverMods?.forEach((mod) => server.moduleGraph.invalidateModule(mod)); + } if (browser) { const browserMods = browser.vite.moduleGraph.getModulesByFile(filepath); browserMods?.forEach((mod) => browser.vite.moduleGraph.invalidateModule(mod)); From 63304de0e25b91d2bd15f5a71fd864cb07083ddc Mon Sep 17 00:00:00 2001 From: Gert Hengeveld Date: Fri, 6 Dec 2024 10:22:50 +0100 Subject: [PATCH 4/8] Clear progress when starting new test run --- .../src/manager-api/modules/experimental_testmodule.ts | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/code/core/src/manager-api/modules/experimental_testmodule.ts b/code/core/src/manager-api/modules/experimental_testmodule.ts index 294355c73bc6..4058333a3b10 100644 --- a/code/core/src/manager-api/modules/experimental_testmodule.ts +++ b/code/core/src/manager-api/modules/experimental_testmodule.ts @@ -70,7 +70,7 @@ export const init: ModuleFn = ({ store, fullAPI }) => { clearTestProviderState(id) { const update = { cancelling: false, - running: true, + running: false, failed: false, crashed: false, progress: undefined, @@ -85,7 +85,13 @@ export const init: ModuleFn = ({ store, fullAPI }) => { runTestProvider(id, options) { const index = store.getState().index; invariant(index, 'The index is currently unavailable'); - api.updateTestProviderState(id, { running: true, failed: false, crashed: false }); + + api.updateTestProviderState(id, { + running: true, + failed: false, + crashed: false, + progress: undefined, + }); const provider = store.getState().testProviders[id]; From a84dd4a21c5685fc4d4dd589d6eda955ae01f434 Mon Sep 17 00:00:00 2001 From: Gert Hengeveld Date: Fri, 6 Dec 2024 11:37:07 +0100 Subject: [PATCH 5/8] Bit of cleanup --- code/addons/test/src/node/test-manager.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/code/addons/test/src/node/test-manager.ts b/code/addons/test/src/node/test-manager.ts index 39ba575d1518..544a23f5da49 100644 --- a/code/addons/test/src/node/test-manager.ts +++ b/code/addons/test/src/node/test-manager.ts @@ -85,11 +85,13 @@ export class TestManager { if (payload.providerId !== TEST_PROVIDER_ID) { return; } + + const allTestsRun = (payload.storyIds ?? []).length === 0; + if (payload.config && this.coverage !== payload.config.coverage) { this.coverage = payload.config.coverage; } - const allTestsRun = (payload.storyIds ?? []).length === 0; if (this.coverage) { /* If we have coverage enabled and we're running all stories, @@ -100,13 +102,14 @@ export class TestManager { If we're only running a subset of stories, we have to temporarily disable coverage, as a coverage report for a subset of stories is not useful. */ - this.vitestManager.restartVitest({ + await this.vitestManager.restartVitest({ watchMode: allTestsRun ? false : this.watchMode, coverage: allTestsRun, }); + } else { + await this.vitestManager.vitestRestartPromise; } - await this.vitestManager.vitestRestartPromise; await this.vitestManager.runTests(payload); if (this.coverage && !allTestsRun) { From 3434fc67a07c3b16e065eec1a02340ad029e67f4 Mon Sep 17 00:00:00 2001 From: Gert Hengeveld Date: Fri, 6 Dec 2024 11:37:35 +0100 Subject: [PATCH 6/8] Clear the event queue when Vitest fails to start --- code/addons/test/src/node/boot-test-runner.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/code/addons/test/src/node/boot-test-runner.ts b/code/addons/test/src/node/boot-test-runner.ts index 664117f8ea18..4f4b69c45b0a 100644 --- a/code/addons/test/src/node/boot-test-runner.ts +++ b/code/addons/test/src/node/boot-test-runner.ts @@ -61,6 +61,7 @@ const bootTestRunner = async (channel: Channel) => { const exit = (code = 0) => { killChild(); + eventQueue.length = 0; process.exit(code); }; @@ -128,6 +129,7 @@ const bootTestRunner = async (channel: Channel) => { await Promise.race([startChildProcess(), timeout]).catch((e) => { reportFatalError(e); + eventQueue.length = 0; throw e; }); }; @@ -148,4 +150,5 @@ export const killTestRunner = () => { child.kill(); child = null; } + eventQueue.length = 0; }; From 0c272b8a21434ffcac6b38ece9fe23e6b4e7bda6 Mon Sep 17 00:00:00 2001 From: Gert Hengeveld Date: Fri, 6 Dec 2024 11:39:32 +0100 Subject: [PATCH 7/8] Clear vitestRestartPromise when finished --- code/addons/test/src/node/vitest-manager.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/code/addons/test/src/node/vitest-manager.ts b/code/addons/test/src/node/vitest-manager.ts index 78b527c615a9..9ce9139821bb 100644 --- a/code/addons/test/src/node/vitest-manager.ts +++ b/code/addons/test/src/node/vitest-manager.ts @@ -111,6 +111,8 @@ export class VitestManager { resolve(); } catch (e) { reject(e); + } finally { + this.vitestRestartPromise = null; } }); return this.vitestRestartPromise; From ff933dd287ab5ee692e4d8ea0f27c9a9999feb3d Mon Sep 17 00:00:00 2001 From: Gert Hengeveld Date: Fri, 6 Dec 2024 12:24:36 +0100 Subject: [PATCH 8/8] Clear ready state on kill --- code/addons/test/src/node/boot-test-runner.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/code/addons/test/src/node/boot-test-runner.ts b/code/addons/test/src/node/boot-test-runner.ts index 4f4b69c45b0a..3f0329807e98 100644 --- a/code/addons/test/src/node/boot-test-runner.ts +++ b/code/addons/test/src/node/boot-test-runner.ts @@ -150,5 +150,6 @@ export const killTestRunner = () => { child.kill(); child = null; } + ready = false; eventQueue.length = 0; };