Repository navigation
CORE-2720: Replace five snapshot specs with targeted tests - #167
RoyEJohnson wants to merge 2 commits into
Conversation
Checkbox, TreeCheckbox, Tabs, NavBar and Tree were tested by snapshots that mostly recorded someone else's output: react-aria's handler props and generated ids, styled-components class hashes, a 400-character data URI repeated per entry, and document.body dumps. What they pinned of this repo's own behaviour was a handful of class names and prop-driven custom properties, buried in 1,300 lines that changed whenever a dependency or a default did. The replacements assert that behaviour directly: - Checkbox and TreeCheckbox: weight, size, variant colours, the disabled and mixed states, and labelProps. TreeCheckbox also selects its row when used as the selection checkbox in a Tree, which `slot="selection"` exists for. - Tabs: the root class for every variant and size, the named tab list, and switching tabs. - NavBar: the wrapper and portal slot, default and custom heights, maxWidth, the landmark's label, and the logo with and without a link. - Tree: the treegrid, the level variable the stylesheet indents by, and expanding a row through a chevron named after its row. Each assertion was checked against a deliberately broken component. They bind only values that depend on props, so they hold whether the static theme defaults are set inline or in the stylesheet. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The targeted tests cover the relevant component-owned behavior while removing dependency-sensitive snapshots.
Review effort: Balanced
Findings: None
What changed in this PR
Replaces five brittle snapshots with focused behavioral and styling tests.
Changes:
- Adds targeted Checkbox, TreeCheckbox, Tree, Tabs, and NavBar assertions.
- Covers accessibility, interaction, variants, sizing, and portal behavior.
- Removes obsolete snapshots.
| File | Description |
|---|---|
src/components/Checkbox/Checkbox.spec.tsx |
Adds focused checkbox tests. |
src/components/Checkbox/__snapshots__/Checkbox.spec.tsx.snap |
Removes checkbox snapshots. |
src/components/NavBar.spec.tsx |
Adds portal, sizing, landmark, and logo tests. |
src/components/Tabs.spec.tsx |
Adds variant, selection, and interaction tests. |
src/components/Tree/Tree.spec.tsx |
Adds tree structure and expansion tests. |
src/components/Tree/TreeCheckbox.spec.tsx |
Adds styling, state, and row-selection tests. |
src/components/Tree/__snapshots__/Tree.spec.tsx.snap |
Removes tree snapshots. |
src/components/Tree/__snapshots__/TreeCheckbox.spec.tsx.snap |
Removes TreeCheckbox snapshots. |
src/components/__snapshots__/NavBar.spec.tsx.snap |
Removes NavBar snapshots. |
src/components/__snapshots__/Tabs.spec.tsx.snap |
Removes Tabs snapshots. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
jivey
left a comment
There was a problem hiding this comment.
I don't usually mind snapshot tests because they occasionally catch unintended markup changes or props making it to the DOM. Is the issue that they are they changing a lot? I'd think they'd only change on a RAC upgrade.
|
(FWIW I found them helpful when reviewing https://github.com/openstax/ui-components/pull/166/changes right after this one) |
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
It is mostly the volume of updates that happen when things change, and not only RAC upgrades. Some recent PRs rewrote multiple snapshot files and had nothing to do with what actually changed. Snapshots are noisy that way: you have to figure out which, if any, of the reported changes matter. Targeted tests are clearer about what is broken. |
Jira: CORE-2720
Tests only; no production code changes. This came out of reviewing #166, which changes these snapshots.
Why
Five specs were tested by snapshots that mostly record someone else's output:
document.bodydumps; the logo's SVG pathsWhat they pinned of this repo's own behaviour was a handful of class names and prop-driven custom properties, buried in 1,300 lines that changed whenever a dependency or a default did.
What replaces them
labelProps. TreeCheckbox also gets the mixed state and a test that it selects its row when used as the selection checkbox in a Tree, which is whatslot="selection"is for.maxWidth, the landmark's label, and the logo with and without a link.--tree-item-levelvariable the stylesheet indents by, and expanding a row. The chevron's accessible name is asserted too ("expand/collapse 1"); react-aria adds the row's label, so every chevron is distinct.The variant tests compare against
checkboxVariantsrather than repeating hex values, so they check the wiring and leave the colours to the theme tests.What is no longer pinned
The snapshots also recorded react-aria's internal structure (ids, handler props, data attributes) and the logo's SVG paths. Those are asserted now only where this repo depends on them: roles, names, levels, selection and expansion.
Verification
size, keeping the checkmark when disabled, droppinglabelProps, swapping the mobile and desktop heights, never linking the logo, dropping the chevron's label or slot, and not passingpropsthrough to the selection checkbox. Every one fails the new tests.Interactions with other PRs
#166 and #163 are merged. Their changes to the five snapshots are resolved by deleting the snapshots, and #163's indeterminate tests are kept alongside the new Checkbox tests.
Not in this PR
ButtonBar and Radio (each under 50 lines) and Button and ManageCookies (small, readable, and beside many behavioural tests) keep their snapshots.
Radio.spec.tsx's "calls onFocus handler" test asserts nothing, so it can never fail; that is worth a follow-up.🤖 Generated with Claude Code