RTL harness and React upgrade prep - #1564
Open
AhtishamShahid wants to merge 8 commits into
Open
Conversation
Zero imports in the codebase. The package does not exist for React 16+, so it would block the upgrade for no benefit. Verified: suite unchanged at 480 passing; production webpack build compiles (7 pre-existing Sass deprecation warnings, unrelated).
React 16's scheduler opens a MessageChannel at import time that keeps the event loop alive, so mocha never exits. Landing the flag ahead of the React bump keeps that change reviewable on its own. Watch mode is excluded. Verified: default and coverage paths both 480 passing, exit 0. Suite already exited cleanly before this change, so the flag is not masking a leak. shfmt/shellcheck not run - neither binary available locally and pre-commit is not a CI job; tabs verified preserved and bash -n is clean.
All three sites perform an imperative side effect on an MDC instance when the 'open' prop changes, so componentDidUpdate is the correct React 16+ replacement. getDerivedStateFromProps cannot be used -- it is static and must be pure. Verified: 480 passing, Drawer_test 12 passing, lint clean. UNVERIFIED - MANUAL CHECK OUTSTANDING BEFORE MERGE: Plan 1 Task 3 Step 8 (browser check of drawer / collection-edit dialog / more_vert menu) has NOT been done. No test in the suite covers these imperative MDC paths -- they are exercised only via .instance(), so a green suite does not prove this conversion correct. Do not merge without it.
RTL 12.1.5 peers react <18.0.0, so it runs on React 15. Pinned exactly -- v13+ requires React 18. Harness test proves RTL renders a React 15 class component and coexists with Enzyme in one mocha process. Extended beyond the plan's 2 static-render cases: act is undefined on react-dom 15, so the cases that actually matter are event-driven and async. Verified working -- fireEvent flushing setState, findBy* after an async setState, and cleanup() firing componentWillUnmount. Suite: 480 -> 485 passing.
Repo-wide no-op reformat. The tree had drifted from its own formatter -- the last repo-wide format was #398 and fmt:check was never added to CI, so 'npm run fmt' rewrote 36 files unrelated to any current change. Landing it once, isolated, so every later commit in the React 19 / RTL migration has a clean diff and 'npm run fmt' is a no-op. No behaviour change. Of 36 files, 25 differ only in whitespace; the other 11 differ only in prettier normalizations verified by hand -- single to double quotes, trailing-comma removal, import/expression reflowing, object key realignment. Verified: 485 passing, lint clean, production webpack build compiles with the same 7 pre-existing Sass warnings, and 'npm run fmt:check' now reports 139 files unchanged.
Drops the dead #integration_test_div branch (that element is never created anywhere in the repo) and the now-unused ReactDOM import. NOTE: the plan's stated bug does not exist. It claimed global_init.js wipes the DOM before RTL can unmount, so componentWillUnmount never fires. Probed directly -- observed order is test-body -> componentWillUnmount -> later hooks. RTL's cleanup holds a direct reference to its own container, so detaching it from document.body does not prevent unmountComponentAtNode. The change is still worth making for two weaker reasons: the innerHTML reset previously ran first, so componentWillUnmount fired against an already- detached node (Dialog/Drawer/Menu call MDC .destroy() there, VideoPlayer disposes its player), and calling cleanup() explicitly removes any dependence on mocha hook registration order. teardown_test.js is therefore a regression guard, not a reproduction -- it passed both before and after. Suite: 485 -> 487 passing, twice with identical counts.
All 32 Enzyme test files currently define their own local render helper wrapping <Provider>. One shared helper replaces them as files migrate. Suite: 487 -> 492 passing.
First conversion, and the reference pattern for the rest. Uses the shared renderWithProviders helper and keeps redux-asserts' listenForActions unchanged. Adds an explicit non-null check on .message -- Enzyme's find() returns a wrapper even for zero matches, so the old assertion could not tell 'wrong text' from 'element missing'. Verified the test can actually fail: injected two mutants into ErrorPage.js (corrupted title text, renamed the .message class) and both killed all 3 tests. A conversion that cannot fail is a deleted test with extra steps. 31 Enzyme test files remain. Suite: 492 passing.
AhtishamShahid
marked this pull request as ready for review
July 31, 2026 09:57
AhtishamShahid
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related issues
What
Phase E0 + R0 of the React 15 → 18 / Enzyme → RTL migration. Stands up React Testing Library
alongside Enzyme with a shared render helper and correct teardown, converts one pilot test file,
and clears the React-upgrade prerequisites that don't depend on Enzyme.
React stays on
^15.6.1and Enzyme stays installed. Nothing here upgrades React.Why this ordering
RTL 12 and Enzyme coexist in one mocha process, so the migration can proceed file-by-file with the
suite green throughout. This was verified directly rather than assumed: RTL 12.1.5 renders, fires
events, resolves async queries, and unmounts correctly on
react@15.6.1— despiteactbeingundefinedonreact-dom@15. RTL degrades gracefully instead of throwing.That matters because it means no unofficial Enzyme bridge adapter is needed at any point.
Enzyme is removed while still on React 15, then React moves in later phases.
Changes
bcfad9areact-addons-shallow-compare(zero imports; doesn't exist for React 16+)bf8e775--exitto the mocha runner (default/coverage/codecov; watch excluded)c023294componentWillReceiveProps→componentDidUpdateinDialog/Drawer/Menucbcdf77e80dfb6prettier-eslintreformat (isolated, no behaviour change)86b1e29cleanup()in global teardown; drop dead#integration_test_divbranch7fd9380renderWithProvidersRTL helperf8284e6ErrorPage_test.jsfrom Enzyme to RTL (pilot / reference pattern)New files under
static/js/testUtils/:renderWithProviders.jsplusharness_test.js,teardown_test.js,renderWithProviders_test.js.Verification
npm run lintclean;npm run fmt:checkcleanNODE_ENV=production npm run buildcompiles (7 pre-existing Sass warnings from@material0.33)js_test.shstderr allowlist (still 7)The pilot conversion was mutation-checked by hand. Two bugs were injected into
ErrorPage.js(corrupted title text, renamed
.messageclass) and each killed all 3 tests — confirming theconverted test detects breakage rather than merely passing.
The three lifecycle conversions were verified in a browser, since no test covers them (they're
reached only via
.instance(), the API being removed). Each of the drawer, Edit Collection dialog,and per-video
more_vertmenu was opened, closed, and reopened — the reopen being the casethat matters, as
componentDidUpdatedoesn't fire on mount and the prop comparison inverted. Allpassed, with zero console errors across a full cycle.
Reviewer notes
e80dfb6is the noisy one. The repo had drifted from its own formatter (last repo-wideformat was Reformat using eslint-config-mitodl #398;
fmt:checkisn't a CI gate), sonpm run fmtrewrote 36 unrelated files.Landed as its own commit so the other seven stay reviewable. Of the 36, 25 differ only in
whitespace; the other 11 were hand-checked as prettier normalizations (quote style, trailing
commas, import/expression reflow). Review it with whitespace hidden.
@testing-library/reactis pinned to exactly12.1.5with no caret — v13+ requires React 18.Please don't let a caret in until React reaches 18.
Out of scope / follow-ups
Two pre-existing environment bugs surfaced while running this. Neither is caused by these changes
and neither is fixed here:
Dockerfile-nodeships node 24.17.0 whilepackage.jsonpins 24.16.0. Yarn 1 hard-fails onthe mismatch, so the
watchcontainer crash-loops and every bundle 404s —docker-compose upcurrently yields a blank app. Workaround is
YARN_IGNORE_ENGINES=1in a local.env.docker-compose.ymlstill carries the obsoleteversion:attribute, warned on every command.Also noted for a future ticket:
global_init.jsappends a<script>tag at module load "to appeasevideojs-youtube", but the
afterEachwipesdocument.body.innerHTML— so it's gone after the firsttest. Not addressed here because fixing it changes behaviour for tests this PR doesn't touch.
Next
Recommend landing the mutation-testing baseline (mitodl/hq#12637) before the bulk conversions in
mitodl/hq#12638 — it's the only objective check that converted tests still assert anything.