Merge pull request #29749 from storybookjs/jeppe/always-watch-vitest

Test: Always run Vitest in watch mode internally

code/addons/test/src/node/test-manager.test.ts

@@ -1,5 +1,5 @@
 import { describe, expect, it, vi } from 'vitest';
-import { createVitest } from 'vitest/node';
+import { createVitest as actualCreateVitest } from 'vitest/node';
 
 import { Channel, type ChannelTransport } from '@storybook/core/channels';
 import type { StoryIndex } from '@storybook/types';
@@ -34,6 +34,7 @@ const vitest = vi.hoisted(() => ({
 vi.mock('vitest/node', () => ({
   createVitest: vi.fn(() => Promise.resolve(vitest)),
 }));
+const createVitest = vi.mocked(actualCreateVitest);
 
 const transport = { setHandler: vi.fn(), send: vi.fn() } satisfies ChannelTransport;
 const mockChannel = new Channel({ transport });
@@ -109,7 +110,7 @@ describe('TestManager', () => {
 
     await testManager.handleWatchModeRequest({ providerId: TEST_PROVIDER_ID, watchMode: true });
     expect(testManager.watchMode).toBe(true);
-    expect(createVitest).toHaveBeenCalledTimes(2);
+    expect(createVitest).toHaveBeenCalledTimes(1); // shouldn't restart vitest
   });
 
   it('should handle run request', async () => {
@@ -145,4 +146,57 @@ describe('TestManager', () => {
     expect(setTestNamePattern).toHaveBeenCalledWith(/^One$/);
     expect(vitest.runFiles).toHaveBeenCalledWith(tests.slice(0, 1), true);
   });
+
+  it('should handle coverage toggling', async () => {
+    const testManager = await TestManager.start(mockChannel, options);
+    expect(testManager.coverage).toBe(false);
+    expect(createVitest).toHaveBeenCalledTimes(1);
+    createVitest.mockClear();
+
+    await testManager.handleConfigChange({
+      providerId: TEST_PROVIDER_ID,
+      config: { coverage: true, a11y: false },
+    });
+    expect(testManager.coverage).toBe(true);
+    expect(createVitest).toHaveBeenCalledTimes(1);
+    createVitest.mockClear();
+
+    await testManager.handleConfigChange({
+      providerId: TEST_PROVIDER_ID,
+      config: { coverage: false, a11y: false },
+    });
+    expect(testManager.coverage).toBe(false);
+    expect(createVitest).toHaveBeenCalledTimes(1);
+  });
+
+  it('should temporarily disable coverage on focused tests', async () => {
+    vitest.globTestSpecs.mockImplementation(() => tests);
+    const testManager = await TestManager.start(mockChannel, options);
+    expect(testManager.coverage).toBe(false);
+    expect(createVitest).toHaveBeenCalledTimes(1);
+
+    await testManager.handleConfigChange({
+      providerId: TEST_PROVIDER_ID,
+      config: { coverage: true, a11y: false },
+    });
+    expect(testManager.coverage).toBe(true);
+    expect(createVitest).toHaveBeenCalledTimes(2);
+
+    await testManager.handleRunRequest({
+      providerId: TEST_PROVIDER_ID,
+      indexUrl: 'http://localhost:6006/index.json',
+      storyIds: ['button--primary', 'button--secondary'],
+    });
+    // expect vitest to be restarted twice, without and with coverage
+    expect(createVitest).toHaveBeenCalledTimes(4);
+    expect(vitest.runFiles).toHaveBeenCalledWith([], true);
+
+    await testManager.handleRunRequest({
+      providerId: TEST_PROVIDER_ID,
+      indexUrl: 'http://localhost:6006/index.json',
+    });
+    // don't expect vitest to be restarted, as we're running all tests
+    expect(createVitest).toHaveBeenCalledTimes(4);
+    expect(vitest.runFiles).toHaveBeenCalledWith(tests, true);
+  });
 });

code/addons/test/src/node/test-manager.ts

@@ -39,20 +39,17 @@ export class TestManager {
     this.vitestManager.startVitest().then(() => options.onReady?.());
   }
 
-  async handleConfigChange(
-    payload: TestingModuleConfigChangePayload<{ coverage: boolean; a11y: boolean }>
-  ) {
+  async handleConfigChange(payload: TestingModuleConfigChangePayload<Config>) {
     if (payload.providerId !== TEST_PROVIDER_ID) {
       return;
     }
 
     process.env.VITEST_STORYBOOK_CONFIG = JSON.stringify(payload.config);
 
     if (this.coverage !== payload.config.coverage) {
+      this.coverage = payload.config.coverage;
       try {
-        this.coverage = payload.config.coverage;
         await this.vitestManager.restartVitest({
-          watchMode: this.watchMode,
           coverage: this.coverage,
         });
       } catch (e) {
@@ -68,25 +65,31 @@ export class TestManager {
     }
   }
 
-  async handleWatchModeRequest(payload: TestingModuleWatchModeRequestPayload) {
-    try {
-      if (payload.providerId !== TEST_PROVIDER_ID) {
-        return;
-      }
+  async handleWatchModeRequest(payload: TestingModuleWatchModeRequestPayload<Config>) {
+    if (payload.providerId !== TEST_PROVIDER_ID) {
+      return;
+    }
+    this.watchMode = payload.watchMode;
 
-      if (payload.config) {
-        this.handleConfigChange({
-          providerId: payload.providerId,
-          config: payload.config as any,
-        });
-      }
+    if (payload.config) {
+      this.handleConfigChange({
+        providerId: payload.providerId,
+        config: payload.config,
+      });
+    }
 
-      if (this.watchMode !== payload.watchMode) {
-        this.watchMode = payload.watchMode;
-        await this.vitestManager.restartVitest({ watchMode: this.watchMode, coverage: false });
+    if (this.coverage) {
+      try {
+        if (payload.watchMode) {
+          // if watch mode is toggled on and coverage is already enabled, restart vitest without coverage to automatically disable it
+          await this.vitestManager.restartVitest({ coverage: false });
+        } else {
+          // if watch mode is toggled off and coverage is already enabled, restart vitest with coverage to automatically re-enable it
+          await this.vitestManager.restartVitest({ coverage: this.coverage });
+        }
+      } catch (e) {
+        this.reportFatalError('Failed to change watch mode while coverage was enabled', e);
       }
-    } catch (e) {
-      this.reportFatalError('Failed to change watch mode', e);
     }
   }
 
@@ -96,38 +99,32 @@ export class TestManager {
         return;
       }
 
-      const allTestsRun = (payload.storyIds ?? []).length === 0;
-
-      if (payload.config && this.coverage !== payload.config.coverage) {
-        this.coverage = payload.config.coverage;
+      if (payload.config) {
+        this.handleConfigChange({
+          providerId: payload.providerId,
+          config: payload.config,
+        });
       }
 
-      if (this.coverage) {
-        /*
-           If we have coverage enabled and we're running all stories,
-           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.
-         */
+      /*
+        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.
+      */
+      const temporarilyDisableCoverage =
+        this.coverage && !this.watchMode && (payload.storyIds ?? []).length > 0;
+      if (temporarilyDisableCoverage) {
         await this.vitestManager.restartVitest({
-          watchMode: allTestsRun ? false : this.watchMode,
-          coverage: allTestsRun,
+          coverage: false,
         });
       } else {
         await this.vitestManager.vitestRestartPromise;
       }
 
       await this.vitestManager.runTests(payload);
 
-      if (this.coverage && !allTestsRun) {
+      if (temporarilyDisableCoverage) {
         // Re-enable coverage if it was temporarily disabled because of a subset of stories was run
-        await this.vitestManager.restartVitest({
-          watchMode: this.watchMode,
-          coverage: this.coverage,
-        });
+        await this.vitestManager.restartVitest({ coverage: this.coverage });
       }
     } catch (e) {
       this.reportFatalError('Failed to run tests', e);

code/addons/test/src/node/vitest-manager.ts

@@ -40,7 +40,7 @@ export class VitestManager {
 
   constructor(private testManager: TestManager) {}
 
-  async startVitest({ watchMode = false, coverage = false } = {}) {
+  async startVitest({ coverage = false } = {}) {
     const { createVitest } = await import('vitest/node');
 
     const storybookCoverageReporter: [string, StorybookCoverageReporterOptions] = [
@@ -55,7 +55,7 @@ export class VitestManager {
         ? {
             enabled: true,
             clean: false,
-            cleanOnRerun: !watchMode,
+            cleanOnRerun: false,
             reportOnFailure: true,
             reporter: [['html', {}], storybookCoverageReporter],
             reportsDirectory: resolvePathInStorybookCache(COVERAGE_DIRECTORY),
@@ -66,9 +66,8 @@ export class VitestManager {
     this.vitest = await createVitest(
       'test',
       {
-        watch: watchMode,
+        watch: true,
         passWithNoTests: false,
-        changed: watchMode,
         // TODO:
         // Do we want to enable Vite's default reporter?
         // The output in the terminal might be too spamy and it might be better to
@@ -110,18 +109,16 @@ export class VitestManager {
       this.testManager.reportFatalError('Failed to init Vitest', e);
     }
 
-    if (watchMode) {
-      await this.setupWatchers();
-    }
+    await this.setupWatchers();
   }
 
-  async restartVitest({ watchMode, coverage }: { watchMode: boolean; coverage: boolean }) {
+  async restartVitest({ coverage }: { 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 });
+        await this.startVitest({ coverage });
         resolve();
       } catch (e) {
         reject(e);
@@ -324,6 +321,11 @@ export class VitestManager {
     this.updateLastChanged(id);
     this.storyCountForCurrentRun = 0;
 
+    // when watch mode is disabled, don't trigger any tests (below)
+    // but still invalidate the cache for the changed file, which is handled above
+    if (!this.testManager.watchMode) {
+      return;
+    }
     await this.runAffectedTests(file);
   }
 

test-storybooks/portable-stories-kitchen-sink/react/yarn.lock

@@ -1841,7 +1841,7 @@ __metadata:
 
 "@storybook/experimental-addon-test@file:../../../code/addons/test::locator=portable-stories-react%40workspace%3A.":
   version: 8.5.0-alpha.18
-  resolution: "@storybook/experimental-addon-test@file:../../../code/addons/test#../../../code/addons/test::hash=f1c849&locator=portable-stories-react%40workspace%3A."
+  resolution: "@storybook/experimental-addon-test@file:../../../code/addons/test#../../../code/addons/test::hash=21369f&locator=portable-stories-react%40workspace%3A."
   dependencies:
     "@storybook/csf": "npm:0.1.12"
     "@storybook/global": "npm:^5.0.0"
@@ -1864,7 +1864,7 @@ __metadata:
       optional: true
     vitest:
       optional: true
-  checksum: 10/29f2db97577ff0a3a79ae37a8b0eaebd86d7eca43c3ad364abbb5f4a63e5f9d12dab2927723802752a2c2e2d36561ac89db32a7fd770e31c5b6e573e265bdd27
+  checksum: 10/2081814e214dc1dd31144870a6a4ea7637c9c241ab02044488be57e19402c206c0037d449197f77bb4262147703f6d0b27f09c9f6cc2ee358c97fd7d1cdfa908
   languageName: node
   linkType: hard