Addon-Vitest: Add channel API to programmatically trigger test runs#33206
Addon-Vitest: Add channel API to programmatically trigger test runs#33206
Conversation
|
View your CI Pipeline Execution ↗ for commit a3aad16
☁️ Nx Cloud last updated this comment at |
|
Failed to publish canary version of this pull request, triggered by @JReinhold. See the failed workflow run at: https://github.com/storybookjs/storybook/actions/runs/19732250010 |
…don-vitest-trigger-api
|
Failed to publish canary version of this pull request, triggered by @JReinhold. See the failed workflow run at: https://github.com/storybookjs/storybook/actions/runs/21150762531 |
|
Failed to publish canary version of this pull request, triggered by @JReinhold. See the failed workflow run at: https://github.com/storybookjs/storybook/actions/runs/21150973271 |
|
Failed to publish canary version of this pull request, triggered by @JReinhold. See the failed workflow run at: https://github.com/storybookjs/storybook/actions/runs/21151373423 |
…don-vitest-trigger-api
…don-vitest-trigger-api
…don-vitest-trigger-api
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an a11y preset/export and renames A11YReport → A11yReport; exposes isAddonA11yEnabled; introduces TRIGGER_TEST_RUN channel APIs for the Vitest addon, switches Vitest to an in-memory StoryIndex with invalidation, threads a Channel through preset loading and dev/build server flows. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
code/addons/vitest/src/types.ts (1)
2-2: Avoid leakinganyin the public A11y report typing.
anyweakens downstream type safety; considerunknownor a minimal structural type as a temporary workaround.♻️ Suggested typing tweak
-type A11yReport = any; +type A11yReport = unknown;Also applies to: 5-8
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/addons/vitest/src/types.ts` at line 2, The public A11y report types leak `any`; update those exported types (e.g., A11yReport, A11yResult or any similarly named interfaces found in this file) to replace `any` with `unknown` or a small structural type (e.g., { id?: string; message?: string; nodes?: unknown[] }) so downstream code has safer typings; adjust any callsites or type assertions to narrow/validate the unknowns as needed and export the new types in place of the `any`-using definitions.code/addons/vitest/src/node/test-manager.ts (1)
269-274: Verify array concatenation doesn't cause memory issues with large test suites.The statuses are accumulated via
concat()on every throttled flush. For very large test suites, this could result in significant memory growth. Consider if there's a maximum expected size or if the arrays are cleared between runs.Looking at the code,
storeOptions.initialState.currentRunis spread intocurrentRunat the start of each run (line 143), which should reset these arrays. This appears safe.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/addons/vitest/src/node/test-manager.ts` around lines 269 - 274, The concatenation of componentTestStatuses, a11yStatuses and a11yReports on each throttled flush can grow unbounded across runs; ensure these arrays are reset at run start by using the initialized currentRun from storeOptions.initialState.currentRun (the code path that sets state.currentRun at run start) or explicitly clear them before concatenation in the flush code (references: componentTestStatuses, a11yStatuses, a11yReports, currentRun, storeOptions.initialState.currentRun); if a hard bound is needed also trim (slice) the arrays to a configured max length before storing to avoid memory growth for very large suites.code/addons/vitest/src/constants.ts (1)
31-31: Consider using a constant or importing the version from StoryIndex type.The hardcoded
v: 5could become stale if the StoryIndex version changes. Consider importing or referencing a shared constant to ensure consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/addons/vitest/src/constants.ts` at line 31, Replace the hardcoded StoryIndex version value (currently "v: 5" in the object literal with key `index`) with a shared constant or the exported version from the StoryIndex type/module so it stays in sync; locate the `index: { entries: {}, v: 5 }` entry in constants.ts and import or reference the canonical VERSION/SCHEMA_VERSION (or StoryIndex.VERSION) and use that symbol instead of the literal 5.code/addons/vitest/src/preset.ts (1)
28-28: Consider using public API instead of reaching into core internals.The import
'../../../core/src/node-logger/logger'directly accesses internal core modules. IfshouldLogis needed, consider exporting it fromstorybook/internal/node-loggerto maintain proper module boundaries.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/addons/vitest/src/preset.ts` at line 28, The code imports the internal helper shouldLog from core internals; update it to use the public API by importing shouldLog from the exported entry (e.g., storybook/internal/node-logger) instead of '../../../core/src/node-logger/logger'. If that symbol isn't currently exported, add an export of shouldLog from the public module (node-logger index or storybook/internal/node-logger) so code in preset.ts can import shouldLog via the public path; ensure you update the import statement in preset.ts to reference the public module name (storybook/internal/node-logger) and remove the direct core/src import.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/addons/a11y/src/types.ts`:
- Line 5: Add a deprecated compatibility alias so existing imports using the old
A11YReport name continue to work: export a new type alias A11YReport that points
to the new A11yReport (and optionally annotate it with a JSDoc `@deprecated` tag).
Update the same types file where A11yReport is defined to include this alias
(referencing the A11yReport type) so downstream code isn't broken by the rename.
In `@code/addons/vitest/src/node/vitest-manager.ts`:
- Around line 221-227: getStories may return undefined items when
requestStoryIds contains IDs not present in
this.testManager.store.getState().index.entries; change the mapping logic in
getStories to skip missing entries (e.g., map then filter out undefined or use
flatMap to only include found StoryIndexEntry) so the returned array contains
only StoryIndexEntry objects and not undefined values. Update the code path that
uses getStories to rely on the cleaned array but do not rely on the previous
cast to StoryIndexEntry[] to hide missing entries. Ensure you reference the
getStories method and the index.entries lookup when making the fix.
In `@code/addons/vitest/src/preset.ts`:
- Around line 86-88: The result of options.presets.apply('storyIndexGenerator')
(stored in storyIndexGenerator) may be null/undefined; before calling
storyIndexGenerator.getIndex(...) and storyIndexGenerator.onInvalidated(...),
add a null check (e.g., if (!storyIndexGenerator) return or skip those calls) so
you don't invoke methods on undefined; update the code paths that call getIndex
and onInvalidated (referencing storyIndexGenerator, getIndex, onInvalidated, and
options.presets.apply) to guard them and handle the absent generator case
gracefully (either no-op, log a warning, or provide a fallback).
In `@code/addons/vitest/src/types.ts`:
- Around line 69-78: The initial call to storyIndexGenerator.getIndex() should
be wrapped in the same try-catch used in the onInvalidated callback: call await
storyIndexGenerator.getIndex() inside a try block and if it throws or returns
undefined, set index to the fallback default from storeOptions.initialState
(e.g. { entries: {}, v: 5 }) and populate the store's fatalError field with the
caught error; update the store initialization logic that sets index (and
fatalError) so the rest of the store (and components reading index.entries)
never sees undefined.
---
Nitpick comments:
In `@code/addons/vitest/src/constants.ts`:
- Line 31: Replace the hardcoded StoryIndex version value (currently "v: 5" in
the object literal with key `index`) with a shared constant or the exported
version from the StoryIndex type/module so it stays in sync; locate the `index:
{ entries: {}, v: 5 }` entry in constants.ts and import or reference the
canonical VERSION/SCHEMA_VERSION (or StoryIndex.VERSION) and use that symbol
instead of the literal 5.
In `@code/addons/vitest/src/node/test-manager.ts`:
- Around line 269-274: The concatenation of componentTestStatuses, a11yStatuses
and a11yReports on each throttled flush can grow unbounded across runs; ensure
these arrays are reset at run start by using the initialized currentRun from
storeOptions.initialState.currentRun (the code path that sets state.currentRun
at run start) or explicitly clear them before concatenation in the flush code
(references: componentTestStatuses, a11yStatuses, a11yReports, currentRun,
storeOptions.initialState.currentRun); if a hard bound is needed also trim
(slice) the arrays to a configured max length before storing to avoid memory
growth for very large suites.
In `@code/addons/vitest/src/preset.ts`:
- Line 28: The code imports the internal helper shouldLog from core internals;
update it to use the public API by importing shouldLog from the exported entry
(e.g., storybook/internal/node-logger) instead of
'../../../core/src/node-logger/logger'. If that symbol isn't currently exported,
add an export of shouldLog from the public module (node-logger index or
storybook/internal/node-logger) so code in preset.ts can import shouldLog via
the public path; ensure you update the import statement in preset.ts to
reference the public module name (storybook/internal/node-logger) and remove the
direct core/src import.
In `@code/addons/vitest/src/types.ts`:
- Line 2: The public A11y report types leak `any`; update those exported types
(e.g., A11yReport, A11yResult or any similarly named interfaces found in this
file) to replace `any` with `unknown` or a small structural type (e.g., { id?:
string; message?: string; nodes?: unknown[] }) so downstream code has safer
typings; adjust any callsites or type assertions to narrow/validate the unknowns
as needed and export the new types in place of the `any`-using definitions.
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 2 | 2 | 0 |
| Self size | 391 KB | 402 KB | 🚨 +12 KB 🚨 |
| Dependency size | 338 KB | 338 KB | 0 B |
| Bundle Size Analyzer | Link | Link |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
code/addons/vitest/src/node/test-manager.test.ts (2)
185-185: Consider moving mock implementations tobeforeEachblock.The
globTestSpecificationsmock implementation appears inline in multiple test cases (lines 185, 200, 215, 230, 247):vitest.globTestSpecifications.mockImplementation(() => tests);Per coding guidelines, mock implementations should be in
beforeEachblocks rather than inline within test cases. Consider adding this to a sharedbeforeEach:♻️ Suggested refactor
beforeEach(() => { createVitest.mockResolvedValue(vitest); + vitest.globTestSpecifications.mockImplementation(() => tests); });Then remove the inline
mockImplementationcalls from individual tests.Based on learnings: "Implement mock behaviors in
beforeEachblocks in Vitest tests" and "Avoid inline mock implementations within test cases in Vitest tests."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/addons/vitest/src/node/test-manager.test.ts` at line 185, Move the repeated inline mock setup for vitest.globTestSpecifications into a shared beforeEach so tests reuse the same mock behavior: add a beforeEach that calls vitest.globTestSpecifications.mockImplementation(() => tests) (or equivalent using the existing tests variable) and remove the individual inline mockImplementation calls from the tests; ensure any tests that need a different behavior override the mock inside their own test or a nested describe, and keep the symbol references vitest.globTestSpecifications, beforeEach, and tests to locate and update the code.
127-135: Consider moving mock implementation tobeforeEach.The
runWithStatemock has its implementation defined at module scope:runWithState: vi.fn((callback) => callback()),Per coding guidelines, mock implementations should be in
beforeEachblocks. Consider:♻️ Suggested refactor
const mockTestProviderStore: TestProviderStoreById = { getState: vi.fn(), setState: vi.fn(), settingsChanged: vi.fn(), onRunAll: vi.fn(), onClearAll: vi.fn(), - runWithState: vi.fn((callback) => callback()), + runWithState: vi.fn(), testProviderId: 'test-provider-id', };Then in
beforeEach:beforeEach(() => { createVitest.mockResolvedValue(vitest); vi.mocked(mockTestProviderStore.runWithState).mockImplementation((callback) => callback()); });Based on learnings: "Avoid mock implementations outside of
beforeEachblocks in Vitest tests."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/addons/vitest/src/node/test-manager.test.ts` around lines 127 - 135, The mock implementation for runWithState is defined at module scope on mockTestProviderStore; move that implementation into a beforeEach so mocks are reset per-test. Remove the inline vi.fn((callback) => callback()) from mockTestProviderStore and in the beforeEach set vi.mocked(mockTestProviderStore.runWithState).mockImplementation((callback) => callback()); also ensure any related setup such as createVitest.mockResolvedValue(vitest) is configured in the same beforeEach so all test-scoped mocks are initialized there.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@code/addons/vitest/src/node/test-manager.test.ts`:
- Line 185: Move the repeated inline mock setup for
vitest.globTestSpecifications into a shared beforeEach so tests reuse the same
mock behavior: add a beforeEach that calls
vitest.globTestSpecifications.mockImplementation(() => tests) (or equivalent
using the existing tests variable) and remove the individual inline
mockImplementation calls from the tests; ensure any tests that need a different
behavior override the mock inside their own test or a nested describe, and keep
the symbol references vitest.globTestSpecifications, beforeEach, and tests to
locate and update the code.
- Around line 127-135: The mock implementation for runWithState is defined at
module scope on mockTestProviderStore; move that implementation into a
beforeEach so mocks are reset per-test. Remove the inline vi.fn((callback) =>
callback()) from mockTestProviderStore and in the beforeEach set
vi.mocked(mockTestProviderStore.runWithState).mockImplementation((callback) =>
callback()); also ensure any related setup such as
createVitest.mockResolvedValue(vitest) is configured in the same beforeEach so
all test-scoped mocks are initialized there.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/core/src/core-server/build-dev.ts (1)
160-160: Avoidas unknown ascasts; align channel types.The double-casts at lines 160, 218, and 229 weaken type safety.
getServerChannelreturns aChannelinstance that TypeScript cannot directly align with the type expected byloadAllPresets'schannelparameter andOptions['channel']. Consider updatinggetServerChannel's return type to match the expected channel type used byloadAllPresets, or introduce a sharedChanneltype alias to eliminate theunknownintermediate cast.This pattern also appears in
load.tsandbuild-static.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/core-server/build-dev.ts` at line 160, The double-cast using "as unknown as" for channel should be removed by aligning types: update getServerChannel's return type to the Channel type expected by loadAllPresets (the same type used in Options['channel']), or introduce a shared Channel type alias and use that everywhere; then replace occurrences like "channel as unknown as Parameters<typeof loadAllPresets>[0]['channel']" (in build-dev.ts, load.ts, build-static.ts) with a direct typed value so no unknown intermediate cast is needed. Ensure function signatures (getServerChannel) and variable declarations import/use the shared Channel type so callers (loadAllPresets and any Options['channel'] consumers) accept the returned type without casting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@code/core/src/core-server/build-dev.ts`:
- Line 160: The double-cast using "as unknown as" for channel should be removed
by aligning types: update getServerChannel's return type to the Channel type
expected by loadAllPresets (the same type used in Options['channel']), or
introduce a shared Channel type alias and use that everywhere; then replace
occurrences like "channel as unknown as Parameters<typeof
loadAllPresets>[0]['channel']" (in build-dev.ts, load.ts, build-static.ts) with
a direct typed value so no unknown intermediate cast is needed. Ensure function
signatures (getServerChannel) and variable declarations import/use the shared
Channel type so callers (loadAllPresets and any Options['channel'] consumers)
accept the returned type without casting.
… multiple story files
There was a problem hiding this comment.
🧹 Nitpick comments (2)
code/addons/vitest/src/node/vitest-manager.ts (1)
257-264: Consider graceful handling when parent story is missing.If the index has a stale or inconsistent
parentreference, this throws at runtime and aborts the test run. Consider either skipping the story with a warning or using a fallback pattern.💡 Alternative: fallback to simple pattern
if (story.parent) { const parentStory = storiesById[story.parent]; if (!parentStory) { - throw new Error(`Parent story not found for story ${story.id}`); + // Fallback if parent reference is stale; match just the child name + return `^${escapeRegExp(story.name)}$`; } return `^${escapeRegExp(getTestName(parentStory.name))} ${escapeRegExp(story.name)}$`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/addons/vitest/src/node/vitest-manager.ts` around lines 257 - 264, The current block in vitest-manager.ts throws when a story's parent id is missing (uses story.parent and looks up storiesById), which can abort test runs; change this to handle missing parent gracefully by either logging a warning via the existing logger and returning a fallback pattern (e.g., use only escapeRegExp(getTestName(story.name)) or a looser regex) or by skipping the story with a warning instead of throwing; update the branch that calls getTestName(parentStory.name) and escapeRegExp so it checks for parentStory presence and uses the chosen fallback (or skip) to avoid runtime errors when storiesById lacks the parent.code/addons/vitest/src/node/test-manager.test.ts (1)
47-50: Missingspy: trueoption in vi.mock call.As per coding guidelines, all
vi.mock()calls should use thespy: trueoption for consistent mocking patterns.♻️ Proposed fix
-vi.mock('vitest/node', async (importOriginal) => ({ +vi.mock('vitest/node', { spy: true }, async (importOriginal) => ({ ...(await importOriginal()), createVitest: mockCreateVitest, }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/addons/vitest/src/node/test-manager.test.ts` around lines 47 - 50, The vi.mock call that replaces createVitest with mockCreateVitest must include the spy: true option; update the vi.mock invocation for 'vitest/node' (the one that returns ...(await importOriginal()) and createVitest: mockCreateVitest) to pass the third argument { spy: true } so the mock is created with spying enabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@code/addons/vitest/src/node/test-manager.test.ts`:
- Around line 47-50: The vi.mock call that replaces createVitest with
mockCreateVitest must include the spy: true option; update the vi.mock
invocation for 'vitest/node' (the one that returns ...(await importOriginal())
and createVitest: mockCreateVitest) to pass the third argument { spy: true } so
the mock is created with spying enabled.
In `@code/addons/vitest/src/node/vitest-manager.ts`:
- Around line 257-264: The current block in vitest-manager.ts throws when a
story's parent id is missing (uses story.parent and looks up storiesById), which
can abort test runs; change this to handle missing parent gracefully by either
logging a warning via the existing logger and returning a fallback pattern
(e.g., use only escapeRegExp(getTestName(story.name)) or a looser regex) or by
skipping the story with a warning instead of throwing; update the branch that
calls getTestName(parentStory.name) and escapeRegExp so it checks for
parentStory presence and uses the chosen fallback (or skip) to avoid runtime
errors when storiesById lacks the parent.
1ffe647 to
aaa7410
Compare
The Channel class has a private `sender` field which causes TypeScript nominal incompatibility between source and dist declarations. This introduces a ChannelLike structural interface and uses it in Options, loadAllPresets, and Builder types, eliminating all `as unknown as` channel casts.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/core/src/core-server/utils/index-json.ts (1)
11-11: Consider reordering imports for consistency.The
ChannelLikeimport fromstorybook/internal/channelsshould be grouped with otherstorybook/internal/*imports (lines 4-5) rather than placed after local imports.Suggested reordering
import { STORY_INDEX_INVALIDATED } from 'storybook/internal/core-events'; import type { NormalizedStoriesSpecifier } from 'storybook/internal/types'; +import type { ChannelLike } from 'storybook/internal/channels'; import { debounce } from 'es-toolkit/function'; import type { Polka } from 'polka'; import type { StoryIndexGenerator } from './StoryIndexGenerator'; -import type { ChannelLike } from 'storybook/internal/channels'; import { watchStorySpecifiers } from './watch-story-specifiers';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/core-server/utils/index-json.ts` at line 11, Move the ChannelLike import so it is grouped with the other storybook internal imports for consistency: locate the import "ChannelLike" from 'storybook/internal/channels' and reposition it alongside the existing 'storybook/internal/*' imports (the ones near the top that import Storybook internals) rather than after local imports; ensure import ordering follows the project's import-grouping convention (external libs, storybook internals together, then local modules).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@code/core/src/core-server/utils/index-json.ts`:
- Line 11: Move the ChannelLike import so it is grouped with the other storybook
internal imports for consistency: locate the import "ChannelLike" from
'storybook/internal/channels' and reposition it alongside the existing
'storybook/internal/*' imports (the ones near the top that import Storybook
internals) rather than after local imports; ensure import ordering follows the
project's import-grouping convention (external libs, storybook internals
together, then local modules).
kasperpeulen
left a comment
There was a problem hiding this comment.
LGTM! Seems like some good architecture changes, couple of comments, and I fixed the channel TS issues
…don-vitest-trigger-api
…cy between: ws token -> server channel -> loading all presets -> core preset creates ws token -> ... Co-authored-by: Norbert de Langen <norbert@chromatic.com>
Works on storybookjs/mcp#138
Companion PR: storybookjs/mcp#131
What I did
Added a new programmatic trigger to
addon-vitest, which allows external actors (like addon-mcp) to trigger a test run and get results back via events on the channel.To support this, I had to make additional modifications:
options, so that any preset property (likeexperimental_devServer) automatically has access to the channel and can do stuff with it.Previously, it would fetch the
index.jsonwhen necessary; however, it wouldn't know which origin/url to fetch from. To know that, the manager would send the origin to the server (indexUrl) on startup, because the client does know where to fetch the index from. If this sounds bad, it's because it was, but it was a good compromise at the time.But not anymore, because now you can trigger tests without even opening the manager UI once, which would leave
indexUrlundefined, and crash the test run. So now it does the more clear thing, which is possible as of recently, but wasn't before. Nowaddon-vitestgets the index directly fromStoryIndexGeneratoron the server, and subscribes to changes to the index, so that when tests are triggered, the index is already available and up-to-date.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
addon-mcpinstalled from Add testing toolset mcp#131run-story-teststool and report which stories passed or failed."Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This pull request has been released as version
0.0.0-pr-33206-sha-06900f2a. Try it out in a new sandbox by runningnpx storybook@0.0.0-pr-33206-sha-06900f2a sandboxor in an existing project withnpx storybook@0.0.0-pr-33206-sha-06900f2a upgrade.More information
0.0.0-pr-33206-sha-06900f2ajeppe/addon-vitest-trigger-api06900f2a1770382401)To request a new release of this pull request, mention the
@storybookjs/coreteam.core team members can create a new canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=33206Summary by CodeRabbit
New Features
Refactor
Tests