Add CI step for storybook test runner #935
Conversation
martha
changed the title
martha
changed the title
martha
changed the title
| onlyChanged: true | ||
| - name: Storybook test |
There was a problem hiding this comment.
@martha I am wondering about the choice to user storybook test runner on CI as opposed to running tests via Chromatic on CI. The storybook gives the example that you might want to "Use [test runner] locally and Chromatic on your CI". I would guess that a benefit of Chromatic would be if the tests fail, maybe you can see some more detailed output on chromatic as opposed to just test logs. (Judging from chromatic docs about PR workflow it seems pretty advanced).
Linking https://github.com/open-path/Green-River/issues/6687 for reference too
There was a problem hiding this comment.
Sorry, I didn't read all the way through your description 🙈
I believe, alternatively, it's possible to run tests in CI against a Chromatic instance (see this other recipe), but based on the existing "Run Chromatic" build step, it looks like we only want to run chromatic on push to a release branch and not on pull request.
That was set up mostly for the purpose of publishing the "release version" of the storybook. I don't have anything against using chromatic to run testing on PRs. (And I guess publishing, maybe that's the norm for chromatic usage).
There was a problem hiding this comment.
Sounds good, I updated the PR to run the tests against the Chromatic instance of Storybook.
We do still need the test runner to run the accessibility tests. Chromatic supports running interaction tests, but doesn't support a11y tests as far as I can tell. You can see this in the example build here. The test runner step shows a11y failures, but the Chromatic step succeeds. In the corresponding Chromatic build for that action, there are 51 "unreviewed changes" (bc I had temporarily turned on the "visual tests" setting) but there are no a11y-related failures or warnings.
.storybook/test-runner.ts
Outdated
| @@ -0,0 +1,26 @@ | |||
| // COMMENTED OUT for now until we want to actually turn on accessibility tests on CI | |||
There was a problem hiding this comment.
Do the tests pass for you locally? test-storybook passed for me (which I was surprised by, if it included accessibility tests?) but I'm not 100% confident that I resolved the merge conflicts correctly
There was a problem hiding this comment.
We can see the accessibility failures both locally and on CI by uncommenting the contents of this file. See this build, for example.
Do you want to actually turn this on now and make all our CI tests fail until we fix the a11y issues? I had assumed not yet...
But maybe it would be a good idea to turn it on and factor it out into a separate build step, that way when it fails we can immediately recognize the failure is expected, instead of clicking into the gh action log to make sure it's failing on the expected step?
There was a problem hiding this comment.
Huh okay I wasn't seeing them locally, but I'll try again. No I agree, lets not intentionally have CI fail.
Description
This PR adds a CI step for the Storybook test runner, which we can eventually use to run automated a11y tests. The a11y test config itself is commented out for now, though, since it causes the build to fail. Without that turned on, the test runner is just running each story and passing the test if it encounters no errors. See example build: https://github.com/greenriver/hmis-frontend/actions/runs/10947831708/job/30397456339?pr=935#step:10:1213
This PR adds new dependencies: concurrently, http-server, and wait-on. They are recommended by this recipe provided by Storybook for running the test runner in CI.I believe, alternatively, it's possible to run tests in CI against a Chromatic instance (see this other recipe), but based on the existing "Run Chromatic" build step, it looks like we only want to run chromatic on push to a release branch and not on pull request.Type of change
Checklist before requesting review