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

Check Storybook build on CI for PRs #68466

Merged
merged 5 commits into from
Jan 7, 2025
Merged

Check Storybook build on CI for PRs #68466

Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4c12233
Check Storybook build on CI for PRs
mirka Jan 2, 2025
fdf8121
Test breakage
mirka Jan 2, 2025
e3ed320
Fix 😇
mirka Jan 2, 2025
9233cc9
Revert "Test breakage"
mirka Jan 6, 2025
2d73623
Also run from forked repos
mirka Jan 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions .github/workflows/storybook-check.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
name: Check Storybook build

on: pull_request

# Cancels all previous workflow runs for pull requests that have not completed.
concurrency:
# The concurrency group contains the workflow name and the branch name for pull requests
# or the commit hash for any other events.
group: ${{ github.workflow }}-${{ github.event_name == 'pull_request' && github.head_ref || github.sha }}
cancel-in-progress: true

jobs:
check:
runs-on: ubuntu-latest
if: ${{ github.repository == 'WordPress/gutenberg' }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if: ${{ github.repository == 'WordPress/gutenberg' }}
if: ${{ github.repository == 'WordPress/gutenberg' || github.event_name == 'pull_request' }}

Not a blocker: Should we also run this CI on PRs submitted to forked repos? Some other CIs seem to do that. I think it's ok to go either way.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, I think I removed this part unintentionally 🙈


steps:
- name: Checkout
uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
with:
show-progress: ${{ runner.debug == '1' && 'true' || 'false' }}

- name: Setup Node.js and install dependencies
uses: ./.github/setup-node

- name: Build Storybook
run: npm run storybook:build
Copy link
Member

Choose a reason for hiding this comment

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

🤔 does it make sense to run the regular build command here though? We don't really need the built assets, right?

There's a --smoke-test feature we could use for this purpose:

https://storybook.js.org/docs/api/cli-options#:~:text=my%2Dssl%2Dkey-,%2D%2Dsmoke%2Dtest,-Exit%20after%20successful

Furthermore, for such purposes, we should likely utilize --ci, which skips any interactive prompts:

https://storybook.js.org/docs/api/cli-options#:~:text=dev%20%2D%2Dsmoke%2Dtest-,%2D%2Dci,-CI%20mode%20(skip

I'd suggest we introduce a new smoke test command and we use it here.
Its package.json script entry would look roughly like this (untested):

"storybook:smoke-test": "storybook dev -c ./storybook --ci --smoke-test"

WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

I saw this in the original PR but leaned toward build for a couple reasons:

  • What we most want to prevent is the public Storybook workflow failing on trunk. There is a chance that the dev build succeeds but the prod build doesn't.
  • At least when running time comparisons locally, there is no substantial wall-clock difference between dev --ci --smoke-test and build, even though the total CPU time is clearly different (apparently build is better parallelized). The only significant difference was in the failing case, where dev --ci --smoke-test was actually 1 minute slower in wall-clock time due to some weird hanging between the error being logged and the process exiting.
  • Using dev requires a new test command (and possibly an additional pre command) in package.json.

So all in all it seems to me like using build is simpler and good enough, but no strong opinion. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

There is a chance that the dev build succeeds but the prod build doesn't.

Could you elaborate on that? Under what circumstances would it happen?

What's more important is that it's surprising that the smoke test is slower! In my tests against v7 (in another repo, though), that wasn't the case. If the regular build command is faster, I don't see a good reason not to go with it 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a chance that the dev build succeeds but the prod build doesn't.

Could you elaborate on that? Under what circumstances would it happen?

Nothing serious, I'm just thinking of code paths in the build process that involve process.env.NODE_ENV conditionals.

Loading