-
Notifications
You must be signed in to change notification settings - Fork 10
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.
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
Upgrade Storybook to 8.x (next) #2373
base: main
Are you sure you want to change the base?
Conversation
|
Size Change: 0 B Total Size: 97.3 kB ℹ️ View Unchanged
|
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-cfoaqzhznw.chromatic.com/ Chromatic results:
|
], | ||
staticDirs: ["../static"], | ||
core: { | ||
disableTelemetry: true, | ||
}, | ||
framework: getAbsolutePath("@storybook/react-vite"), | ||
async viteFinal(config, {configType}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: I'm moving this to a separate vite.config.ts
file for better integration with vitest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
y'all are switching to vitest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, it's just for running storybook tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct! I'm thinking that we should probably write an ADR if we want to adopt this more widely.
Just curious.... I know this is a big change, but I wonder if you have any opinions about switching from Jest to vitest for unit tests?
if (configType === "PRODUCTION") { | ||
config.plugins?.push( | ||
turbosnap({rootDir: config.root || process.cwd()}), | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: Dropping this as it is no longer needed as a separate package. SB 8 now includes it internally.
@@ -413,6 +396,23 @@ export const AutoUpdate: StoryComponentType = { | |||
> | |||
Click to update trigger position (fixed) | |||
</Button> | |||
<Tooltip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: Moved this instance to the right so it can work more consistently in different environments (browser and JSDOM).
The issue with the previous story was that the assertion in the play
function was not working properly in headless browsers (e.g. running npx vitest
) because the viewport/document size was different from the browser one.
This was causing that the Tooltip was not positioned horizontally, causing the test to fail in CI. With this change, we can now ensure that there will be a horizontal reposition.
vite.config.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: Moved this to a separate config file so it can be reused by Storybook
and vitest
.
vitest.config.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: This file was generated when I added the experimental addon. More info here: https://storybook.js.org/docs/writing-tests/test-addon#automatic-setup
.storybook/vitest.setup.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: This file was generated when I added the experimental addon. More info here: https://storybook.js.org/docs/writing-tests/test-addon#automatic-setup
// compare different color formats, so hex was converted to RGB. | ||
await expect(computedStyle.outline).toBe("rgb(24, 101, 242) solid 1px"); | ||
// rgb(24, 101, 242) is the same as Color.blue | ||
await expect(link).toHaveStyle("outline: rgb(24, 101, 242) solid 1px"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: This and other similar assertions where changed b/c getComputedStyle
doesn't work with the new testing approach.
I got these errors the first time I integrated the new SB test CLI in CI, and made these changes to fix the failures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do we think about tests that check for specific styles? I had started removing jest tests that were asserting styles since it would be visually covered by Chromatic (especially with the "All Variants" stories + pseudostates add on!). Is it helpful to also have these component test checks for styles?
Kinda related - In general, when would it be helpful for us to reach for component tests in Storybook? I'm thinking if there's functionality we want to test for that relies on browser behaviour! Curious to see what other think!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
100% agree with you. We should move this to "All variants" but the issue is that Link
(and Button
) still rely on Clickable
for setting pseudo states styles. As you know, I'd love to get us to a point where we don't use Clickable for those styles, and we let CSS handle that for us, but that's a tricky change that would be definitively nice to address.
Once we migrate to CSS pseudoclasses, I'm totally in favor of moving this to visual regression tests instead of interaction tests.
export const Controlled: StoryComponentType = () => { | ||
const [checkedOne, setCheckedOne] = React.useState(false); | ||
const [checkedTwo, setCheckedTwo] = React.useState(false); | ||
export const Controlled: StoryComponentType = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: This component doesn't have any big changes, just moved the code around to support CSF3.
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
npm Snapshot: NOT Published🤕 Oh noes!! We couldn't find any changesets in this PR (10b4b74). As a result, we did not publish an npm snapshot for you. |
Here's an example of how the new Test addon works locally: Screen.Recording.2024-11-28.at.3.37.08.PM.mov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very neat! We'll have to see if component tests can be used in Perseus.
@@ -9,52 +6,17 @@ const config: StorybookConfig = { | |||
"../__docs__/**/*.mdx", | |||
], | |||
addons: [ | |||
getAbsolutePath("@storybook/addon-essentials"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Storybook upgrades always want to drop this back in. I finally searched when it's needed. Apparently in Yarn PnP situations module resolution can fail without this. Good to know we can remove this in Perseus too then.
https://storybook.js.org/docs/faq#how-do-i-fix-module-resolution-in-special-environments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh I see, thanks for the context.
@@ -1,6 +1,6 @@ | |||
import * as React from "react"; | |||
import wonderBlocksTheme from "./wonder-blocks-theme"; | |||
|
|||
import {Decorator} from "@storybook/react"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally out of the blue and unrelated to this PR, but it looks like you don't have import/order
enabled in Wonder Blocks. I (not sure about others Perseus) really love how it automatically orders imports so I don't think about it... although, maybe you don't think about it either. :D
https://github.com/Khan/perseus/blob/main/.eslintrc.js#L220..L236
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great point! I'll add this rule in a separate PR. I'll probably wait until WB is in a more "quiet" state to apply the changes :)
@@ -332,7 +332,7 @@ export const ControlledMultilpleCombobox: Story = { | |||
], | |||
|
|||
play: async ({canvasElement}) => { | |||
const canvas = within(canvasElement); | |||
const canvas = within(canvasElement.ownerDocument.body); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, this differs from the examples for these play()
functions. :( They always show how you originally had it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh yeah, this is useful for when we are using portals. I should add a comment here stating that.
"@storybook/react": "^8.2.1", | ||
"@storybook/react-vite": "^8.2.1", | ||
"@storybook/test": "^8.2.1", | ||
"@storybook/addon-a11y": "^8.5.0-alpha.10", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-alpha.10
😱
Are you planning to wait till this is released before landing? 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hehe living on the edge! j/k
I can totally wait until it is officially released :). I mainly wanted to start this PR to be able to unlock their upcoming Accessibility addon (that uses this new testing tooling), so @marcysutton can try it out and give us some feedback, and also give feedback to the Storybook team as well, which she's planning to do :).
In the screen recording it looked like there were three errors, but when you clicked on each test individually it looked like they were all passing. Am I misinterpreting what I was seeing? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. You may want to wait until 8.5 is no longer alpha as Jeremy suggested.
], | ||
staticDirs: ["../static"], | ||
core: { | ||
disableTelemetry: true, | ||
}, | ||
framework: getAbsolutePath("@storybook/react-vite"), | ||
async viteFinal(config, {configType}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
y'all are switching to vitest?
], | ||
staticDirs: ["../static"], | ||
core: { | ||
disableTelemetry: true, | ||
}, | ||
framework: getAbsolutePath("@storybook/react-vite"), | ||
async viteFinal(config, {configType}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, it's just for running storybook tests.
<Tooltip | ||
content="This is a tooltip that auto-updates its position when the trigger element changes." | ||
opened={true} | ||
autoUpdate={true} | ||
> | ||
<View | ||
style={[ | ||
position && { | ||
position: "absolute", | ||
top: position.y, | ||
left: position.x, | ||
}, | ||
]} | ||
> | ||
Trigger element | ||
</View> | ||
</Tooltip> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was moved to be included after the buttons. See my previous comment here: https://github.com/Khan/wonder-blocks/pull/2373/files#r1862635270
); | ||
}; | ||
|
||
Controlled.play = async ({canvasElement}) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In previous versions of Storybook you had to manually navigate to each story and press "play" to run the tests, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right! and that made it hard to determine when we introduced a regression with these interaction tests. I also added the CI step to ensure that we don't introduce issues in the future.
NOTE: Storybook uses the test-runner for this, but it will be replaced by this new test addon.
The previous runner used Jest + Playwright, and they wanted to switch to Vitest specially because they can run these tests in browsers as well as in headless mode. This is a new vitest
feature that is also being refined in collaboration between the Storybook and vitest teams: https://vitest.dev/guide/browser/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More context here: https://storybook.js.org/blog/storybook-test-sneak-peek/
@kevinb-khan good question. I added the video to use as a visual reference when we see failures in the browser. You can see the failures being reported in headless mode in CI here: https://github.com/Khan/wonder-blocks/actions/runs/12057096671/job/33620982112 What you see in the individual views, those tests were passing, but were considered flaky as it seemed that the results between browser and CI diverged. I asked the Storybook team about that, and they found the issue which I fixed here: 93279fa The issues were not flaky per se, it was that I misconfigured the global decorators in preview.tsx. For more context, the Storybook folks helped me to find the cause here: https://discord.com/channels/486522875931656193/1301551207835504694/1311423632412774481 I joined their #sb-test-early-access channel in the Storybook Discord server to be able to experiment with this new feature in our repo and get early feedback from them. |
That's super cool that these tests run on CI in Chromatic. This is really powerful stuff. Thanks for setting this up. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for setting this up! Looking forward to the upcoming a11y tooling 😄
// compare different color formats, so hex was converted to RGB. | ||
await expect(computedStyle.outline).toBe("rgb(24, 101, 242) solid 1px"); | ||
// rgb(24, 101, 242) is the same as Color.blue | ||
await expect(link).toHaveStyle("outline: rgb(24, 101, 242) solid 1px"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do we think about tests that check for specific styles? I had started removing jest tests that were asserting styles since it would be visually covered by Chromatic (especially with the "All Variants" stories + pseudostates add on!). Is it helpful to also have these component test checks for styles?
Kinda related - In general, when would it be helpful for us to reach for component tests in Storybook? I'm thinking if there's functionality we want to test for that relies on browser behaviour! Curious to see what other think!
Summary:
Upgrading to v8.5.x
Upgrading Storybook to experiment with the new test addon.
We are doing this so we can test the new UI testing capabilities that work with
vitest
+Playwright
under the hood.It will also get us closer to test the new a11y tooling that will be available
in the next days.
NOTE: This new feature is still under development by the Storybook team, so we
are not yet ready to use it in production.
You can see more information about this new testing approach here:
Integrating the new testing approach in CI
Now that we have upgraded to the experimental version, we can take advantage of its new features.
This PR adds a new step to our GH workflows: Running Storybook component tests in CI.
Issue: XXX-XXXX
Test plan:
yarn storybook
and check if the new test addon is available.Also verify how the storybook tests failed here:
https://github.com/Khan/wonder-blocks/actions/runs/12057096671/job/33620982112
Now check that these tests were fixed here:
https://github.com/Khan/wonder-blocks/actions/runs/12074721182/job/33673231130#step:9:1024