Rewrite the mount-based presentational tests with React Testing Library#314
Conversation
|
Claude finished @jonathangreen's task in 5m 2s —— View job Claude Code Review
SummaryThis is a clean, high-quality tranche of the enzyme→RTL migration. All 24 new suites follow the established I found no bug-level or otherwise actionable issues in the new suites — nothing to change. Nice work. |
Greptile SummaryThis PR rewrites 24 of 28 Enzyme mount-based test suites to React Testing Library, moving them from
Confidence Score: 5/5Pure test-code change with no production source modifications — safe to merge. Every changed file is a test file. The three deferred suites retain their Enzyme originals, the deleted LibraryStats suite is demonstrably redundant, and the deleted AdvancedSearchBuilder RTL suite has its only meaningful test subsumed by the new AdvancedSearchFilterInput suite. No production behaviour is at risk. No files require special attention. Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #314 +/- ##
==========================================
- Coverage 90.19% 90.14% -0.06%
==========================================
Files 205 205
Lines 7610 7610
Branches 1664 1664
==========================================
- Hits 6864 6860 -4
Misses 437 437
- Partials 309 313 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
jonathangreen
force-pushed
the
chore/rewrite-mount-based-tests-rtl
branch
2 times, most recently
from
a23adc7 to
1912007
Compare
jonathangreen
force-pushed
the
chore/rewrite-mount-based-tests-rtl
branch
from
1912007 to
b669d2c
Compare
jonathangreen
force-pushed
the
chore/rewrite-mount-based-tests-rtl
branch
from
b669d2c to
c567ffb
Compare
jonathangreen
changed the base branch from
chore/jest-remove-mocha
to
chore/formdata-shim-test-util
jonathangreen
force-pushed
the
chore/formdata-shim-test-util
branch
from
d45ae2c to
3739c04
Compare
jonathangreen
force-pushed
the
chore/rewrite-mount-based-tests-rtl
branch
2 times, most recently
from
2be4ee3 to
2085ccb
Compare
## Description Adds one shared FormData test shim at `tests/jest/testUtils/formDataShim.ts`, plus its spec test. The unit jsdom environment installs undici's `FormData`, which throws on `new FormData(formElement)`. The reusable-components `Form` builds FormData that way on submit, so any test that asserts a submitted payload needs a stand-in. Previously each migration branch carried its own inline copy; this consolidates them into one. The shim mirrors the browser's `new FormData(form)` **successful-controls** rules: - named, non-disabled controls only; - checkboxes/radios contribute only when checked; - submit/reset/button/image/file inputs excluded; - every selected option of a `<select multiple>` included. It also exposes the standard read/write/iterate surface (`get`/`getAll`/`has`/`append`/`set`/`delete`/`forEach`/`keys`/`values`/`entries`/iterator). `installFormDataShim()` swaps it in around a suite (`beforeAll`) and restores the real `FormData` afterward (`afterAll`). `formDataShim.test.ts` (8 cases) pins the successful-controls behavior — disabled omitted, checkbox only when checked, radio group, submit excluded, multi-select expansion, and the read/write API — so the fidelity can't silently regress. ## Motivation and Context Part of the mocha/chai/enzyme → jest/RTL migration. Extracting the shim into its own small, self-tested base PR lets #314, #315, and #316 depend on one implementation via `installFormDataShim()` instead of duplicating (and independently drifting) a local shim in each — while keeping this base stable so those stacked PRs don't churn. ## How Has This Been Tested? `npx jest --selectProjects=unit --testPathPattern formDataShim` — 8/8 green. ## Checklist: - [x] I have updated the documentation accordingly. - [x] All new and existing tests passed.
24 of the 28 mount-based enzyme suites: the AdvancedSearch filter widgets (rendered inside a real DndProvider — HTML5Backend works under jsdom), the small leaf components and buttons, the self-test and manage-patrons panels, the search/sidebar/services views, three of the tab containers, Footer, and the app bootstrap (index.tsx). The tab containers navigate via router.push rather than internal state, so "active tab" is asserted through the visible panel; where a tab panel fetches on mount and is incidental to a navigation test it is mocked to a marker. LibraryStats is deleted outright rather than ported — tests/jest/components/Stats.test.tsx already covers LibraryStats.tsx at 100% of lines and tests more (the query hook, caching, sysadmin gating). The bootstrap test keeps its structural shape: it spies on ReactDOM.render, constructs CirculationAdmin in each mode, and asserts the captured element tree contains SetupPage / a Router — the analog of the original enzyme find(), since what those roots render is covered by their own tests. Coverage rises to 94.48% (+24 lines over the baseline) with no previously- covered line lost; rendering real leaf children reaches branches the shallow and mount-with-stubbed-children tests did not. Deferred, with their enzyme originals retained: ConfigTabContainer and BookDetailsTabContainer (their full mounts are the sole cover for eleven connected panels' mapStateToProps/mapDispatchToProps wiring — that coverage is restored properly when those panels are migrated to render their connected exports in later waves) and AdvancedSearchBuilder (its one uncovered line is a react-dnd move callback reachable only through a real drop, grouped with the other drag-dependent coverage in the final wave). Mocking those panels now would have dropped 77 lines; the coverage gate caught it. 57 legacy suites remain.
jonathangreen
force-pushed
the
chore/rewrite-mount-based-tests-rtl
branch
from
2085ccb to
13c6a7f
Compare
tdilauro
left a comment
tdilauro
left a comment
There was a problem hiding this comment.
Looks good!
A few minor things, but no showstoppers.
| expect((options[0] as HTMLOptionElement).selected).toBe(true); | ||
|
|
||
| expect(options[1]).toHaveValue("or"); | ||
| expect(options[1]).toHaveTextContent( | ||
| "Any of these filters may be matched:" | ||
| ); | ||
| expect((options[1] as HTMLOptionElement).selected).toBe(false); |
There was a problem hiding this comment.
Minor - A couple of these (the true/false) assertions are unneeded because of the expect(select).toHaveValue("and"); at the end of the test:
| expect((options[0] as HTMLOptionElement).selected).toBe(true); | |
| expect(options[1]).toHaveValue("or"); | |
| expect(options[1]).toHaveTextContent( | |
| "Any of these filters may be matched:" | |
| ); | |
| expect((options[1] as HTMLOptionElement).selected).toBe(false); | |
| expect(options[1]).toHaveValue("or"); | |
| expect(options[1]).toHaveTextContent( | |
| "Any of these filters may be matched:" | |
| ); |
| expect( | ||
| screen.getByRole("button", { name: "Download" }) | ||
| ).toBeInTheDocument(); |
There was a problem hiding this comment.
Minor - This is duplicating the check for one of the buttons above, but it is the way I would prefer (over the approach above) to check for the buttons with Jest/RTL.
expect(screen.getByRole("button", { name: "Download" })).toBeInTheDocument();
expect(screen.getByRole("button", { name: "Close" })).toBeInTheDocument();
There was a problem hiding this comment.
expect(screen.getByRole("button", { name: "Close" })).toBeInTheDocument() doesn't work here, because the headers x dismiss button is also called Close. So using that causes getByRole to throw because it matched two elements.
I made some changes here though to clean up the test a little bit and scope the getByRole down to the footer.
| <div data-testid="Link" data-to={props.to} className={props.className}> | ||
| {props.children} | ||
| </div> |
There was a problem hiding this comment.
Minor - It would make more sense for this to be an <a> than a <div>.
| <div data-testid="Link" data-to={props.to} className={props.className}> | |
| {props.children} | |
| </div> | |
| <a data-testid="Link" data-to={props.to} className={props.className}> | |
| {props.children} | |
| </a> |
There was a problem hiding this comment.
I switched this to an <a>, but I also needed to add a href. Otherwise linting fails on jsx-a11y/anchor-is-valid.
- AdvancedSearchFilter: replace a masked not-selected assertion (readOnly
forced it true regardless of id) with a dedicated non-readOnly mismatch
test that actually exercises the query.id === selectedQueryId false branch
- AdvancedSearchBooleanFilter: drop the redundant option .selected
assertions; select.toHaveValue("and") already covers it
- CirculationEventsDownloadForm: query the buttons by role, scoping Close to
the modal footer to avoid the header dismiss button's colliding "Close" name
- CustomListsSidebar: render the mocked Link as an anchor (href for a11y)
3 participants
Description
Rewrites 24 of the 28 mount-based enzyme suites to React Testing Library: the AdvancedSearch filter widgets (rendered inside a real
DndProvider— HTML5Backend works under jsdom), the small leaf components and buttons, the self-test and manage-patrons panels, the search/sidebar/services views, three of the tab containers,Footer, and the app bootstrap (index.tsx).router.pushrather than internal state, so "active tab" is asserted through the visible panel; where a tab panel fetches on mount and is incidental to a navigation test, it's mocked to a marker.LibraryStatsis deleted outright rather than ported —tests/jest/components/Stats.test.tsxalready coversLibraryStats.tsxat 100% of lines and tests more (the query hook, caching, sysadmin gating).ReactDOM.render, constructsCirculationAdminin each mode, and asserts the captured element tree containsSetupPage/ aRouter— the analog of the original enzymefind(), since what those roots render is covered by their own tests.userEventusage follows theconst user = userEvent.setup()convention used acrosstests/jest/.Deferred, with their enzyme originals retained:
ConfigTabContainerandBookDetailsTabContainer(their full mounts are the sole cover for eleven connected panels'mapStateToProps/mapDispatchToPropswiring, restored properly when those panels migrate to render their connected exports) andAdvancedSearchBuilder(its one uncovered line is a react-dnd move callback reachable only through a real drop). Mocking those panels now would have dropped 77 lines; the coverage gate caught it.Motivation and Context
Part of the mocha/chai/enzyme → jest/RTL migration — the components enzyme actually mounted, the largest tranche. Retiring the last enzyme suites is what unblocks React 17+.
How Has This Been Tested?
npm test— full suite green. Coverage rises to 94.48% (+24 lines over the baseline) with no previously-covered line lost; rendering real leaf children reaches branches the shallow and mount-with-stubbed-children tests did not.Checklist: