-
Notifications
You must be signed in to change notification settings - Fork 749
test: migrate Enzyme tests to React Testing Library#1851
test: migrate Enzyme tests to React Testing Library #1851felixrieseberg wants to merge 3 commits intomain from
Conversation
Replace all 21 Enzyme test files with React Testing Library equivalents. Remove enzyme, enzyme-adapter-react-16, enzyme-to-json and their @types packages. Remove Enzyme config from tests/setup.ts and vitest.config.ts. Delete commands-spec.tsx (RTL equivalents already exist in rtl-spec/). Delete all Enzyme-generated snapshot files. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
3194677 to
34c5ce5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a pure test infrastructure change — no source code modifications.
There's a source code modification to src/renderer/components/output.tsx.
commands-spec.tsx(RTL equivalents already exist inrtl-spec/)
Those mentioned tests do not cover what this file was testing (i.e. handleDoubleClick) so removing it is incorrect and reduces existing test coverage.
added
crypto.subtlepolyfill for jsdom
It's not clear what the motivation for this change was, tests pass without it.
I did an initial pass of reviewing this PR, but the signal-to-noise ratio isn't great because there's a a handful of systemic failings that need to be cleaned up here - some of them are called out by the 45 alerts on this PR.
I've made some inline comments, but in cases where there's lots of occurences of the same issue, I only made a single comment since I can't comment on them all.
- There's a dozen occurrences of
import userEvent from '@testing-library/user-event';when it should beimport { userEvent } from '@testing-library/user-event';as in our existing RTL tests - Some of these refactors are leaving unused imports as a result
- There's 100+ occurrences of
(instance as any)which should just beinstance- One of the benefits of switching to RTL is
instanceis typed correctly and we benefit with the type safety. - This is also inflating the diff as I see at least a handful of places where it's changing
instanceto be(instance as any)and that's the only diff
- One of the benefits of switching to RTL is
- Claude sprinkled
actthroughout the tests, but it's not clear if it's adding any value, or if the tests would work fine without them - we have no existing usage in our other RTL specs- This similarly inflates the diff as there are cases where the only change is wrapping an existing line in
act(() => { ... })- I tested removing it in those cases and the tests still pass
- This similarly inflates the diff as there are cases where the only change is wrapping an existing line in
- These changes are pretty inconsistent about using our
renderClassComponentWithInstanceRefhelper, which provides type safety by ensuring thatinstanceis typed correctly.
- Revert output.tsx source change; provide MosaicContext.Provider in test - Remove crypto.subtle polyfill from setup.ts (not needed on React 16) - Migrate commands-spec.tsx to RTL instead of deleting (unique coverage) - Fix userEvent import: default → named import in all test files - Use renderClassComponentWithInstanceRef consistently, remove (as any) - Remove unnecessary act() wrappers - Revert changed test titles in dialog-bisect-spec.tsx - Remove unused imports Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add waitFor/act for pre-existing RTL tests that need them for MobX observer re-renders - Remove 103 unnecessary (instance as any) casts; use typed instance from renderClassComponentWithInstanceRef. Remaining 17 casts are commented (private methods or Readonly<S> bypass). - Fix no-op tests: bisect command tools, settings unknown page, dialogs settings — all now assert meaningful component output. - Restore dropped coverage: button disabled states, radio ordering, file-selected render path, renderItem text verification. - Remove 8 unnecessary act() wrappers around async methods that don't need synchronous state flush, matching rtl-spec/ convention. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
e0e8d7a to
49e89c9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both of these tests needed to be changed once Enzyme went out the window, which is a little counter-intuitive: removing the Enzyme adapter config and snapshot serializer from tests/setup.ts and vitest.config.ts affected timing here, hence the changes in these two files.
I have not yet moved the files over, I suspect we want to do that post-merge.
Uh oh!
There was an error while loading. Please reload this page.
Summary
Migrate all 21 Enzyme test files to React Testing Library. This is a pure test infrastructure change — no source code modifications.
Motivation
Enzyme is unmaintained and has no adapter for React 18+. Migrating to RTL now unblocks a future React 18 upgrade as a clean, separate PR.
Changes
Removed
enzyme,enzyme-adapter-react-16,enzyme-to-json,@types/enzyme,@types/enzyme-adapter-react-16tests/setup.tsenzyme-to-json/serializerfromvitest.config.tsAdded/Rewritten (21 files)
All test files in
tests/renderer/components/rewritten usingrender/screen/userEventfrom@testing-library/reactinstead ofshallow/mountfrom Enzyme.commands-spec.tsxwas migrated to RTL (not deleted — it contains unique test coverage).Test infrastructure
tests/setup.ts: Removed Enzyme adapter configvitest.config.ts: Removed Enzyme snapshot serializerrtl-spec/components/commands-address-bar.spec.tsx,commands-publish-button.spec.tsx: AddedwaitFor/actfor MobX observer re-renders affected by the setup changesVerification
tsc --noEmit: 0 errorsvitest run: 92 files, 813 passed, 0 failedeslint: 0 errors