Build: Fix and improve mcp addon e2e test#33905
Conversation
|
View your CI Pipeline Execution ↗ for commit 5eb1409
☁️ Nx Cloud last updated this comment at |
📝 WalkthroughWalkthroughThis pull request updates an e2e test to verify a newly added third "test" toolset and its accessibility tool, replacing previous assertions that checked for two enabled toolsets with more granular visibility and status checks. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Possibly related PRs
✨ Finishing Touches
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
code/e2e-tests/addon-mcp.spec.ts (2)
134-134: Test name is now misleading.The test name says "should show both toolsets as enabled" but now verifies three toolsets (dev, docs, and test). Consider updating it to reflect the actual assertion count.
✏️ Suggested rename
- test('should show both toolsets as enabled', async ({ page }) => { + test('should show all toolsets as enabled', async ({ page }) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/e2e-tests/addon-mcp.spec.ts` at line 134, Rename the misleading test title in the test call whose current name is "should show both toolsets as enabled" to accurately reflect the assertions (e.g., "should show all three toolsets as enabled (dev, docs, test)"); update the string passed to the test(...) invocation so the test name matches the actual verification of the dev, docs, and test toolsets.
147-150: Consider using exact text matching for the "test" selector.The
text=testselector performs substring matching in Playwright, which could inadvertently match elements containing "test" elsewhere (e.g., "toolset", "contest", etc.). Using exact matching would make this more robust.Additionally, using
.first()on.toolset-statussuggests multiple status elements may exist within the toolset. If the first match isn't the intended element (e.g., due to DOM reordering), this could lead to flaky failures.♻️ Proposed fix using exact text matching
- const testToolset = page.locator('.toolset', { has: page.locator('text=test') }); + const testToolset = page.locator('.toolset', { has: page.locator('text="test"') });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/e2e-tests/addon-mcp.spec.ts` around lines 147 - 150, The selector uses substring matching and .first() which can be flaky; change the locator to use exact text matching (replace page.locator('.toolset', { has: page.locator('text=test') }) with page.locator('.toolset', { has: page.locator('text=\"test\"') }) or equivalent exact-text syntax) and stop relying on .first() — instead assert that the toolset contains a status element with the exact text 'enabled' (e.g., use testToolset.locator('.toolset-status', { hasText: 'enabled' }) and expect(...).toBeVisible() or expect(testToolset.locator('.toolset-status')).toHaveText('enabled') if a single status is guaranteed).
🤖 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/e2e-tests/addon-mcp.spec.ts`:
- Line 134: Rename the misleading test title in the test call whose current name
is "should show both toolsets as enabled" to accurately reflect the assertions
(e.g., "should show all three toolsets as enabled (dev, docs, test)"); update
the string passed to the test(...) invocation so the test name matches the
actual verification of the dev, docs, and test toolsets.
- Around line 147-150: The selector uses substring matching and .first() which
can be flaky; change the locator to use exact text matching (replace
page.locator('.toolset', { has: page.locator('text=test') }) with
page.locator('.toolset', { has: page.locator('text=\"test\"') }) or equivalent
exact-text syntax) and stop relying on .first() — instead assert that the
toolset contains a status element with the exact text 'enabled' (e.g., use
testToolset.locator('.toolset-status', { hasText: 'enabled' }) and
expect(...).toBeVisible() or
expect(testToolset.locator('.toolset-status')).toHaveText('enabled') if a single
status is guaranteed).
Closes #
What I did
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
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
Release Notes