Test: Improve stack trace when failing on console.warn/error#34697
Conversation
…orrect stack trace
📝 WalkthroughWalkthroughThis PR bumps Vitest-related package versions from ^4.1.0 to ^4.1.5 across multiple manifests, widens a TypeScript coverage-options type, and refactors test-time console mocking in code/vitest-setup.ts from an ignore-list + throw helpers approach to a predicate-based helper that fails on unexpected console calls. ChangesVitest Dependency Upgrades & Type Adjustment
Console Mocking Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches📝 Generate docstrings
Review rate limit: 4/5 reviews remaining, refill in 12 minutes. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/vitest-setup.ts (1)
9-38: 💤 Low valueConsider adding a type guard for non-string console arguments.
The predicates assume the first argument is a string (calling
.includes()/.test()). Ifconsole.warnorconsole.erroris called with a non-string first argument (e.g.,console.warn(someObject)), this will throw aTypeErrorbefore reaching theexpect.fail()check, potentially producing a confusing error.🛡️ Suggested defensive approach
const ALLOWED_CONSOLE_PREDICATES: ((...args: any[]) => boolean)[] = [ - (message) => message.includes('":nth-child" is potentially unsafe'), + (message) => typeof message === 'string' && message.includes('":nth-child" is potentially unsafe'), // ... apply same pattern to other predicates ];Alternatively, add a single type guard at the start of the mock implementation before checking predicates.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/vitest-setup.ts` around lines 9 - 38, The predicates in ALLOWED_CONSOLE_PREDICATES assume the first console argument is a string and call .includes()/.test() on "message", which can throw for non-string values; update the predicates (or add a single wrapper where they are used) to guard for typeof message === 'string' (or coerce via String(message)) before calling .includes()/.test(), e.g., ensure each predicate first returns false if message is not a string so non-string console calls (objects/errors) won't cause a TypeError; refer to ALLOWED_CONSOLE_PREDICATES and the individual predicate functions that reference "message".
🤖 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/vitest-setup.ts`:
- Around line 9-38: The predicates in ALLOWED_CONSOLE_PREDICATES assume the
first console argument is a string and call .includes()/.test() on "message",
which can throw for non-string values; update the predicates (or add a single
wrapper where they are used) to guard for typeof message === 'string' (or coerce
via String(message)) before calling .includes()/.test(), e.g., ensure each
predicate first returns false if message is not a string so non-string console
calls (objects/errors) won't cause a TypeError; refer to
ALLOWED_CONSOLE_PREDICATES and the individual predicate functions that reference
"message".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f0ffe168-fd1a-4ddb-8182-2d2c9a81b7e7
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
code/addons/vitest/package.jsoncode/package.jsoncode/vitest-setup.tspackage.jsonscripts/package.json
…storybook into jeppe/improve-fail-on-error
AriPerkkio
left a comment
There was a problem hiding this comment.
Looks good. PR title's subject should likely be Test: instead of Build: though.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/addons/vitest/src/node/coverage-reporter.ts (1)
11-11: Align the widened reporter type with the manager callsite cast.Line 11 now accepts
ResolvedCoverageOptions(without provider generic), butcode/addons/vitest/src/node/vitest-manager.ts:64still assertsResolvedCoverageOptions<'v8'>. Remove the'v8'assertion to keep types consistent and avoid masking provider assumptions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/addons/vitest/src/node/coverage-reporter.ts` at line 11, The type widening on the CoverageReporter field coverageOptions now uses ResolvedCoverageOptions without a provider generic, so update the callsite in vitest-manager where it currently asserts ResolvedCoverageOptions<'v8'>: remove the explicit '<"v8">' cast and use ResolvedCoverageOptions (or let inference infer the type) so the manager and coverage-reporter types remain consistent and do not hardcode the 'v8' provider assumption.
🤖 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/coverage-reporter.ts`:
- Line 11: The type widening on the CoverageReporter field coverageOptions now
uses ResolvedCoverageOptions without a provider generic, so update the callsite
in vitest-manager where it currently asserts ResolvedCoverageOptions<'v8'>:
remove the explicit '<"v8">' cast and use ResolvedCoverageOptions (or let
inference infer the type) so the manager and coverage-reporter types remain
consistent and do not hardcode the 'v8' provider assumption.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 76c21b41-774a-49fe-ad49-7c3d564fc8d3
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (1)
code/addons/vitest/src/node/coverage-reporter.ts
Closes #
What I did
Ran into a test failing because of a console.warn. Couldn't figure out why or where. Used
vi.defineHelperinstead to cause the failure from console.error/warn, to get the actual stack trace instead. It worked.Later found out the test was already fixed on
next..Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Don't.
Caution
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
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 PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
Chores
Refactor