docs: correct FE1 and agent guidance on Storybook and Vitest roles - #29016
Conversation
8610882 to
41b0835
Compare
Storybook in this repository is visual regression only. The storybook CI job runs pixel-storybook, which loads each story, runs its play function, waits for the DOM to settle, and screenshots it. Vitest is where behavior and interaction assertions belong. The old FE1 inverted this: it demanded interaction coverage in play functions and called RTL interaction tests a violation. Update FE1, its one-line summary, the repository guardrail, and the frontend-review checklist so agents stop producing assertion-heavy play functions.
41b0835 to
74d21c3
Compare
| item that must not change. | ||
| ## FE1: Vitest covers behavior, Storybook covers visual states | ||
|
|
||
| Every frontend change needs a Vitest/RTL test for the behavior, and/or |
There was a problem hiding this comment.
I think demanding "every change be tested" it part of the problem. agents love to interpret that as "a new test must be written" even if coverage is already present. they get desperate looking for ways to expand test coverage and write bullshit negative-value tests as a result.
long way of saying: we need to loosen this language. it's too strict sounding as is.
also wth is rtl?
There was a problem hiding this comment.
Agreed. Has the agent loosen the language a bit
|
|
||
| Every frontend change needs a Vitest/RTL test for the behavior, and/or | ||
| a Storybook story for every visual state the change can render. CI runs | ||
| Pixel, our internal visual regression tool for Storybook, over the build: |
There was a problem hiding this comment.
| Pixel, our internal visual regression tool for Storybook, over the build: | |
| Pixel for visual regression testing using Storybook: |
There was a problem hiding this comment.
Had the agent apply this
| check: what the DOM renders, shows, or hides. The test is the behavior | ||
| check: what the component did. | ||
|
|
||
| - Write the behavior test in a `.test.tsx` file using RTL and `userEvent`. |
There was a problem hiding this comment.
oh, react testing library? I don't like this acronym. it's not common enough to be obvious.
There was a problem hiding this comment.
Fair, to be fair I momentarily read it as right to left 🤣
| check: what the DOM renders, shows, or hides. The test is the behavior | ||
| check: what the component did. | ||
|
|
||
| - Write the behavior test in a `.test.tsx` file using RTL and `userEvent`. |
There was a problem hiding this comment.
| - Write the behavior test in a `.test.tsx` file using RTL and `userEvent`. | |
| - Behavior tests should use Vitest, React Testing Library and `userEvent`. |
I think this is another case of language that is too suggestive of "you must write more tests". it's too prescriptive. saying "write the test" implies that a test must be written.
|
|
||
| - Write the behavior test in a `.test.tsx` file using RTL and `userEvent`. | ||
| Drive the real interaction, then assert the non-visual outcome: the | ||
| callback payload, the API request, the attribute value, the state change. |
There was a problem hiding this comment.
I think tests that assert attribute presence are negative-value and should be avoided.
| you interact with, not as the assertion. An outcome assertion such as | ||
| `expect(el).toBeVisible()` or `toBeInTheDocument()` reports what the DOM | ||
| renders, which is exactly what the story's screenshot already captures. |
There was a problem hiding this comment.
whereas this actually should be said louder, and more strictly. do not assert .toBeInTheDocument(). hard stop.
| - A story's `play` function exists only for state setup the screenshot needs: | ||
| open the menu, toggle the switch, type the text, focus the trigger. Do not | ||
| put assertions in a play function. Pixel fails the capture only if the | ||
| setup throws. |
There was a problem hiding this comment.
and call out more clearly that the alternative here is to do it in Vitest instead if it's actually valuable.
| ## Frontend contract | ||
|
|
||
| - **FE1:** UI behavior changes ship with Storybook stories whose `play` function exercises the real interaction. Vitest or RTL is for pure logic only. | ||
| - **FE1:** UI behavior changes ship with a Vitest/RTL test for the behavior and a Storybook story for each visual state. Pixel screenshots every story in CI, so a story's `play` function may drive the component into the state being captured (open the menu, type text) but must not assert behavior. Tests drive the interaction and assert the non-visual outcome (callback, request, attribute, state); assertions on what the DOM renders, such as visibility or presence, are the screenshot's job. |
There was a problem hiding this comment.
RTL again. just say the full name here.
|
@aslilac my agent has made the changes, I've checked them and I think it covers everything you've asked for. Lemme know if I missed anything or anything else we should cover. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e336f463ec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - Use semantic queries (`getByRole`, `getByLabelText`) to locate the elements | ||
| you interact with, not as the assertion. Do not assert `.toBeInTheDocument()`, | ||
| `.toBeVisible()`, DOM geometry, or attribute presence. Hard stop: they | ||
| report what the DOM renders, which is exactly what the story's screenshot | ||
| already captures. |
There was a problem hiding this comment.
When the contract is semantic DOM state, such as the absence of an incorrect tablist role or the presence of an accessible error message, there may be no callback or request to assert, and a pixel screenshot cannot detect the accessibility regression. The hard ban would reject tests such as site/src/components/Tabs/Tabs.test.tsx:31-46, removing the only check for semantics that remain visually identical. Limit the prohibition to assertions that merely duplicate visual coverage, while allowing rendered or semantic assertions when they are themselves the behavior under test.
AGENTS.md reference: AGENTS.md:L31-L31
Useful? React with 👍 / 👎.
| new Vitest test, and the new visual states by Storybook stories. Behavior | ||
| tests should use Vitest, React Testing Library and `userEvent`. A `play` | ||
| function may only drive state setup the screenshot needs (open the menu, | ||
| type the text); assertions in a `play` function are a FAIL, as is behavior |
There was a problem hiding this comment.
"assertions are a fail" what does that mean? it makes it sound like pixel is supposed to enforce that you don't use assertions, but that's not how it works
There was a problem hiding this comment.
oh, this is in the review skill?
| `toBeInTheDocument`, geometry, attribute presence) is a FAIL, hard stop: | ||
| the story's screenshot already covers it. Flag tests written only to | ||
| expand coverage when equivalent coverage already exists. Flag stories marked | ||
| `parameters.pixel.exclude: true` that have no equivalent test, since an |
There was a problem hiding this comment.
and require an explanation for excluded stories. why does the story exist if it's not worth snapshotting?
|
Merging now because I'm still getting PRs loaded with sloppy tests @aslilac . If there is anything else to fix I'll do a follow-up PR |
3 participants
Storybook in this repository is visual regression only. The storybook CI job runs pixel-storybook, which loads each story, runs its play function, waits for the DOM to settle, and screenshots it.
FE1 stated the opposite: it demanded interaction coverage in play functions and called RTL interaction tests a violation, so agents kept producing assertion-heavy play functions that duplicate what Pixel already captures.
The corrected contract, in every document that restates it:
playfunction must not assert at all (it only drives state setup the screenshot needs), and a Vitest test must not usetoBeVisible/toBeInTheDocument/geometry as its outcome assertion.userEvent, uses semantic queries to locate the element it interacts with, and asserts the non-visual outcome: the callback payload, the API request, the attribute value, the state change.Files updated:
.claude/docs/FRONTEND_PATTERNS.md: FE1 rewritten with both incorrect examples (assertions in a play function, and visibility as a test's outcome) plus a correct example assertingonValueChange's payload.site/AGENTS.md: FE1 one-line summary and the Testing section.AGENTS.md: the frontend repository guardrail..claude/skills/frontend-review/SKILL.md: the FE1 checklist now flags visibility/presence outcome assertions in tests as a FAIL, alongside assertions in play functions.Follow-ups in this stack remove the assertion-only play functions and duplicate stories that the old guidance produced.
Generated by Coder Agents on behalf of @DanielleMaywood.