Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Core/React: Add testing utilities #17282

Merged
merged 37 commits into from
Apr 1, 2022
Merged

Core/React: Add testing utilities #17282

merged 37 commits into from
Apr 1, 2022

Conversation

yannbf
Copy link
Member

@yannbf yannbf commented Jan 19, 2022 •
edited
Loading

Issue:

What I did

Moved common logic from @storybook/testing-* to @storybook/store
Implemented framework-specific logic from @storybook/testing-react in @storybook/react

This will help with portable stories so they can be reused in multiple scenarios such as in jest, documentation, etc.

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

@nx-cloud
Copy link

nx-cloud bot commented Jan 19, 2022 •
edited
Loading

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 528e0fc. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@yannbf yannbf force-pushed the feat/testing-utilities branch from c38eb2a to dc1f310 Compare January 21, 2022 13:25
@yannbf
Copy link
Member Author

yannbf commented Jan 21, 2022

Hey @tmeasday @shilman @ndelangen I believe I have

import { composeStory, composeStories } from '@storybook/react'

working correctly and with correct types, so the PR is ready for review! I’ll likely need to do some cleanup or move files depending on what you think is sensible, so I'm looking forward for your feedback!

@yannbf yannbf requested a review from tmeasday January 21, 2022 14:05
tmeasday
tmeasday reviewed Jan 24, 2022
View reviewed changes
Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

Great stuff! A bunch of qns/comments

@ndelangen ndelangen added the core label Jan 24, 2022
@yannbf yannbf changed the title WIP: testing utilities Add testing utilities to @storybook/store and @storybook/react Jan 24, 2022
@yannbf yannbf force-pushed the feat/testing-utilities branch from ef7731b to 10bccb4 Compare January 24, 2022 18:04
@yannbf yannbf requested a review from tmeasday January 24, 2022 18:06
tmeasday
tmeasday reviewed Jan 25, 2022
View reviewed changes
@yannbf
Copy link
Member Author

yannbf commented Mar 28, 2022

@tmeasday @shilman @ndelangen I updated this PR by moving composeConfigs from preview-web to store, please re-review this PR!

shilman
shilman requested changes Mar 29, 2022
View reviewed changes
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Looking great @yannbf !! Just missing a deprecation warning for setGlobalConfig

@shilman shilman mentioned this pull request Mar 29, 2022
Merged
tmeasday
tmeasday approved these changes Mar 29, 2022
View reviewed changes
Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

OK, LGTM again.

shilman
shilman approved these changes Apr 1, 2022
View reviewed changes
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

LGTM!

@shilman
Copy link
Member

shilman commented Apr 1, 2022

Merging this. We're working on the CI errors in parallel.

@shilman shilman merged commit d3b4566 into next Apr 1, 2022
@shilman shilman deleted the feat/testing-utilities branch April 1, 2022 01:17
@rickdunkin rickdunkin mentioned this pull request Apr 2, 2022
Closed
@IanVS
Copy link
Member

IanVS commented Apr 2, 2022

This has broken the vite builder, how would you suggest we handle it? storybookjs/builder-vite#304

@yannbf
Copy link
Member Author

yannbf commented Apr 2, 2022

This has broken the vite builder, how would you suggest we handle it? storybookjs/builder-vite#304

Damn @IanVS, sorry about that! I'll make sure to add an E2E case for the vite builder.

Two possible fixes: import composeConfigs from store in preview-web and reexport it, or update vite builder to use composeStories from storybook store. Let me know and I'd be happy to help tomorrow (I'm heading to bed)

@IanVS
Copy link
Member

IanVS commented Apr 3, 2022 •
edited
Loading

or update vite builder to use composeStories from storybook store

The problem with that is that the builder supports many versions of storybook, and moving the import would break with older versions of storybook, right? I think it would be best not to remove exports of storybook packages except for in major version releases, right? So, re-exporting might be the best option for now, if you wouldn't mind.

Edit: just to close the loop, a fix has now been made in #17861.

@lili2311 lili2311 mentioned this pull request Nov 25, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kylegach kylegach kylegach left review comments

@tmeasday tmeasday tmeasday approved these changes

@shilman shilman shilman approved these changes

Assignees

@yannbf yannbf

Labels
core feature request react
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@yannbf @tmeasday @shilman @IanVS @kylegach @ndelangen