[WB-1936.1] Re-enable Storybook CI tests + Storybook v10 #2887
Conversation
🦋 Changeset detectedLatest commit: 21261f0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
npm Snapshot: NOT Published🤕 Oh noes!! We couldn't find any changesets in this PR (ef65cd5). As a result, we did not publish an npm snapshot for you. |
|
Size Change: 0 B Total Size: 109 kB ℹ️ View Unchanged
|
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-qcidnnemwq.chromatic.com/ Chromatic results:
|
jandrade
changed the title
| # Exit with status 0 (OK) once the built version has been published to | ||
| # Chromatic. | ||
| exitOnceUploaded: true |
There was a problem hiding this comment.
note: Added this as we don't have to wait for the build process to finish. We already have the info we need at this point.
| # See: https://www.chromatic.com/docs/turbosnap | ||
| onlyChanged: true | ||
|
|
||
| # TODO(WB-1936): Re-enable this check once the issue with Chromatic |
There was a problem hiding this comment.
note: Moved this to chromatic-pr.yml as a separate job, that way we can isolate testing failures from the build step.
| # Install Playwright browsers so Vitest browser mode can run story tests | ||
| - name: Install playwright dependencies | ||
| run: pnpm exec playwright install chromium --with-deps | ||
|
|
||
| # Build the project | ||
| - name: Generate WB build artifacts | ||
| run: pnpm build | ||
|
|
||
| - name: Run Storybook tests | ||
| run: pnpm test:storybook | ||
| env: | ||
| SB_URL: '${{ needs.chromatic_review.outputs.storybookUrl }}' |
There was a problem hiding this comment.
note: This is what I moved from chromatic-build.yml. The other steps are requirements for this to succeed.
| }, | ||
| }, | ||
| docs: { | ||
| codePanel: true, |
There was a problem hiding this comment.
note: Learned about this and added it to the project: https://storybook.js.org/docs/writing-docs/code-panel
| if (enableRenderStateRootDecorator) { | ||
| return ( | ||
| <RenderStateRoot key={localTheme}> | ||
| <ThemeSwitcherContext.Provider value={theme}> | ||
| <ThemeSwitcher theme={theme}> | ||
| <Story /> | ||
| </ThemeSwitcher> | ||
| </ThemeSwitcherContext.Provider> | ||
| </RenderStateRoot> | ||
| ); | ||
| } |
There was a problem hiding this comment.
note: This is no longer used, so I got rid of it.
| args: { | ||
| locale: "en-US", | ||
| }, |
There was a problem hiding this comment.
note: I highly suspect this was the step that was causing the testing timeouts that led us to disable SB tests in CI months ago.
There was a problem hiding this comment.
Ohh interesting! Before we introduced the locale prop, we were using navigator.language to get the locale (which wasn't the same locale as the user's kaLocale). Was the SB test issue related to navigator.language?
There was a problem hiding this comment.
I think so.... I checked in the console and navigator.language was returning en, and Intl.DateTimeFormat expects the local to follow BCP 47 language format: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/DateTimeFormat/DateTimeFormat#locales
According to the docs, en should be valid, but for some reason, SB tests break b/c it expects the region part of the locale to be present: https://developer.mozilla.org/en-US/docs/Glossary/BCP_47_language_tag#bcp_47_syntax
My guess is that the headless browser triggered by Playwright fails to process the date correctly, and that's why we have to provide the both the language and region subtags.
e.g. en-US instead of en.
| field={ | ||
| <TextField id="foo" value="bar" onChange={() => {}} /> | ||
| } |
There was a problem hiding this comment.
note: Refactored this to remove some warnings in the console (adds noise to the test logs).
There was a problem hiding this comment.
note: Same reason.... added these keys to prevent warnings in the browser (you can see similar changes in other files).
There was a problem hiding this comment.
note: As part of figuring out the SB test issue, I decided to upgrade to v10, and as I noticed that the changes were minimal, I decided to go with it in this PR.
There was a problem hiding this comment.
note: Refactored this file to adopt the most up-to-date recommendations from the Storybook team.
https://storybook.js.org/docs/writing-tests/integrations/vitest-addon#example-configuration-files
jandrade
changed the title
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
beaesguerra
left a comment
beaesguerra
left a comment
There was a problem hiding this comment.
Looks good to me! Thanks for these fixes and upgrades! Left some non-blocking questions :)
| }, | ||
| }, | ||
| docs: { | ||
| codePanel: true, |
| args: { | ||
| locale: "en-US", | ||
| }, |
There was a problem hiding this comment.
Ohh interesting! Before we introduced the locale prop, we were using navigator.language to get the locale (which wasn't the same locale as the user's kaLocale). Was the SB test issue related to navigator.language?
| "../__docs__/**/*.mdx", | ||
| ], | ||
| addons: [ | ||
| getAbsolutePath("@storybook/addon-a11y"), |
There was a problem hiding this comment.
Ohh interesting, I thought the getAbsolutePath changes were auto-generated when we used storybook to migrate versions for us! (Looks like we ran pnpm dlx @storybook/latest upgrade in 4c03506 last time!)
Were you able to use the migration tool also to upgrade to Storybook 10? Or is it something we should do manually instead!
There was a problem hiding this comment.
Good question! I used the upgrade tool for it, but I then I got rid of the require part manually as we don't need it. (I still think running the upgrade script is the recommended solution!).
| <rect id="path-1" x="0" y="0" width="256" height="256" rx="8" /> | ||
| </defs> | ||
| <g stroke="none" stroke-width="1" fill="none" fill-rule="evenodd"> | ||
| <g stroke="none" strokeWidth="1" fill="none" fillRule="evenodd"> |
| import {View} from "@khanacademy/wonder-blocks-core"; | ||
| import Button from "@khanacademy/wonder-blocks-button"; | ||
|
|
||
| import {fireEvent} from "storybook/test"; |
There was a problem hiding this comment.
Ohh interesting! How do we know when we should import from @testing-library/react vs storybook/test?
There was a problem hiding this comment.
I think we should always use @tl/react in *.test.tsx files and use storybook/test in *.stories.tsx files that contain interaction tests. I'm not sure why this was working before tbh :)
| "remark-gfm": "^4.0.0", | ||
| "rollup": "^2.79.2", | ||
| "rollup-plugin-node-externals": "^8.0.0", | ||
| "storybook": "catalog:", |
There was a problem hiding this comment.
Should we remove storybook from pnpm-workspace.yaml? Or keep versions in that file and use catalog:? (I'm still not 100% sure when should define version numbers in the catalog!)
wonder-blocks/pnpm-workspace.yaml
Line 35 in 69a709d
There was a problem hiding this comment.
oh good call! I'll remove it from the catalog. I think for the storybook case, it is better to keep versions in package.json directly for the following reasons:
- The SB
upgrademigration tool makes changes directly inpackage.json(it doesn't support workspaces). - Unfortunately, pnpm doesn't have a way to update
pnpm-workspaceversions when runningpnpm addorpnpm up(at least in the pnpm version we use). It looks like a newer version kinda does that: https://pnpm.io/cli/add#--save-catalog, but I don't think the SB update tool supports it.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2887 +/- ##
============================
============================
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Summary:
Some time ago, we disabled Storybook CI tests because of an issue with
Chromatic. This PR re-enables the tests to ensure that the tests are run
when we publish a new build to Chromatic using the latest Storybook version (v10).
As part of the fix, I've moved the SB testing workflow to a separate GH job. By doing this, I finally identified the issue and made changes to fix this job.
Next PR will include enabling a11y automated tests via CI (and locally).
Issue: https://khanacademy.atlassian.net/browse/WB-1936
Test plan:
Verify that the Storybook CI tests are run when we publish a new Chromatic build
and that the tests pass.