Skip to content

Conversation

@adamscybot
Copy link
Contributor

@adamscybot adamscybot commented May 1, 2025

Closes #31150

What I did

#31150 details the proposed fix. Essentially, we do not need to pass the props to the page to perform the storybook load/play steps, which are wrapped in page.evaluate(). They are extraneous. Passing them here is a problem since they have to be serialisable to go through page.evalaute() and functions are not, which prevents useful playwright CT native functionality that allows functions to be passed from tests.

So by omitting them from being passed to the page.evaluate() calls in the first place, but retaining them when calling the real "mount" function, we get the desired native functionality without disrupting storybook.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests --> or whatever the portable stories kitchen sink tests are classified as 🙂
  • end-to-end tests

Manual testing

None needed as this would be exactly the same as doing what I have added to the "renders story with props" portable stories kitchen sink test. Essentially, just pass a function from a playwright test to a portable story, and check that it actually works.

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli-storybook/src/sandbox-templates.ts

  • Make 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/core team here.

core team members can create a canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>

Greptile Summary

Fixed Playwright component testing to properly handle function props in portable stories by modifying the serialization process.

  • Separated props from story reference before page.evaluate() calls to avoid function serialization issues
  • Modified createPlaywrightTest to maintain full props for Playwright's mount function while excluding them from browser evaluation
  • Added test case in test-storybooks/portable-stories-kitchen-sink/react/stories/Button.playwright.tsx to verify function prop handling
  • Fixed regression where function event handlers couldn't be passed from test files
  • Ensures compatibility with Playwright's native component testing functionality

💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!

@adamscybot adamscybot force-pushed the fix-pass-functions-playwright-portable-stories branch from 2e2cf27 to baf2ddd Compare May 1, 2025 16:58
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

@adamscybot
Copy link
Contributor Author

Hi @valentinpalkovic , @yannbf . Sorry for the annoying ping.

Would it be possible to take a look at this? I'm back in this area now and it would be great to get rid of my horrible patches :). We've been running this fix for a little while now with quite a substantial number of tests on top of it with no problems thus far.

@shilman shilman changed the title Fixes #31150 Portable stories for playwright CT now allow functions to be passed as props from the test Portable stories: Fix playwright CT to allow functions to be passed as props from test Jun 7, 2025
@shilman shilman changed the title Portable stories: Fix playwright CT to allow functions to be passed as props from test Portable stories: Fix playwright CT to allow functions to be passed as props Jun 7, 2025
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

… functions to be passed as props from the test.
@adamscybot adamscybot force-pushed the fix-pass-functions-playwright-portable-stories branch from baf2ddd to 6e3b59e Compare June 23, 2025 00:32
@storybook-app-bot
Copy link

storybook-app-bot bot commented Jun 23, 2025

Package Benchmarks

Commit: a824e72, ran on 30 June 2025 at 15:28:23 UTC

No significant changes detected, all good. 👏

@yannbf
Copy link
Member

yannbf commented Jun 30, 2025

Thank you so much for your contribution!

@nx-cloud
Copy link

nx-cloud bot commented Jun 30, 2025

View your CI Pipeline Execution ↗ for commit a824e72.

Command Status Duration Result
nx run-many -t build --parallel=3 ✅ Succeeded 1m 12s View ↗

☁️ Nx Cloud last updated this comment at 2025-07-01 08:45:39 UTC

@yannbf yannbf added the patch:yes Bugfix & documentation PR that need to be picked to main branch label Jun 30, 2025
@yannbf yannbf enabled auto-merge June 30, 2025 14:57
@yannbf yannbf disabled auto-merge July 1, 2025 08:23
@yannbf yannbf merged commit d1eab95 into storybookjs:next Jul 1, 2025
52 of 53 checks passed
ghengeveld pushed a commit that referenced this pull request Jul 8, 2025
…ht-portable-stories

Portable stories: Fix playwright CT to allow functions to be passed as props

(cherry picked from commit d1eab95)
@github-actions github-actions bot added the patch:done Patch/release PRs already cherry-picked to main/release branch label Jul 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug ci:normal patch:done Patch/release PRs already cherry-picked to main/release branch portable stories

Projects

None yet

4 participants