Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates unit tests from Enzyme/Karma to React Testing Library/Jest, modernizing the test infrastructure and removing deprecated testing dependencies.
Changes:
- Removed Enzyme, Karma, Mocha dependencies and related configuration files
- Migrated all test files to use React Testing Library and Jest matchers
- Updated test utilities and mocks to work with Jest instead of rewire/Enzyme
- Modified some application code to improve testability (extracted helpers, made dependencies mockable)
Reviewed changes
Copilot reviewed 89 out of 121 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Removed Karma/Mocha/Enzyme dependencies, added Jest/RTL dependencies |
| jest.setup.js | Added global test helpers (chai, sinon) and polyfills for Jest environment |
| test/unit/**/*.test.js | Migrated all test files from Enzyme to React Testing Library |
| karma.conf.js, loadtests.js | Deleted Karma configuration files |
| app/redux/actions/async.js | Exported _win reference for test mocking |
| app/pages/patientdata/patientdata.js | Extracted computeIsInitialProcessing helper for testability |
| .eslintrc | Added Jest-specific linting rules for test files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
✨ ✨ ✨ |
henry-tp
left a comment
There was a problem hiding this comment.
My review so far. My review has been focused trying to reduce changes to the frontend components to accommodate our tests.
I'm continuing to work on the review, but wanted to get these comments out to you.
|
|
||
| let win = window; | ||
| // Exported as a mutable reference to allow location to be swapped in tests | ||
| export const _win = { location: window.location }; |
There was a problem hiding this comment.
Suggestion, non-blocking:
See https://github.com/tidepool-org/blip/pull/1884/changes#r2875422771
| ], | ||
| }, | ||
| maxWorkers: '50%', | ||
| workerIdleMemoryLimit: '512MB', |
|
✅ Approved conditional on Clint's approval. My suggestions on edits to React Component files are non-blocking for this PR. |
clintonium-119
left a comment
There was a problem hiding this comment.
On the whole, it's great. I do have a few comments that should probably be addressed before merging, but this really sets us up well :)
| }); | ||
| expect(timeInRangeFilterCount()).to.be.null; | ||
| expect(resetAllFiltersButton()).to.be.null; | ||
| }, 60000); |
There was a problem hiding this comment.
60 seconds timeout seems excessing 🫨
There was a problem hiding this comment.
I do wish these timeout bumps weren't necessary, but here's a Travis run for this test file. You can see all the individual tests and how long they each took. It's not pretty.
PASS test/unit/pages/ClinicPatients.test.js (377.579 s)
ClinicPatients
on mount
✓ should not fetch patients for clinic if already in progress (884 ms)
✓ should fetch patients for clinic (429 ms)
✓ should fetch patients for clinic if previously errored (400 ms)
patients hidden
✓ should render a button that toggles patients to be visible (190 ms)
no patients
✓ should render an empty table (270 ms)
✓ should open a modal for adding a new patient (3300 ms)
✓ should prevent adding a new patient with an invalid birthday (2477 ms)
✓ should prevent adding a new patient without an MRN if required by the clinic (2006 ms)
✓ should prevent adding a new patient with an invalid MRN (2980 ms)
✓ should prevent adding a new patient with an existing MRN (2905 ms)
Filter Reset Bar
✓ should hide the Filter Reset Bar (246 ms)
has patients but none matching filter criteria
Filter Reset Bar
✓ should hide the Filter Reset Bar (224 ms)
when Reset Filters button is clicked
✓ should show the No Results text (201 ms)
✓ should remove the active filters from localStorage (260 ms)
when Clear Search button is clicked
✓ should clear the search input text in Redux (383 ms)
has patients
showNames
✓ should show a row of data for each person (335 ms)
✓ should trigger a call to trackMetric (331 ms)
✓ should not have instructions displayed (350 ms)
show names clicked
✓ should render a list of patients (329 ms)
✓ should allow searching patients (547 ms)
✓ should link to a patient data view when patient name is clicked (324 ms)
✓ should link to a patient data view when patient birthday is clicked (321 ms)
✓ should display menu when "More" icon is clicked (345 ms)
✓ should open a modal for patient editing when edit link is clicked (2397 ms)
✓ should disable email editing for non-custodial patients (2013 ms)
✓ should open a modal for managing data connections when data connection menu option is clicked (638 ms)
✓ should remove a patient (485 ms)
tier0100 clinic
✓ should show the standard table columns (685 ms)
✓ should refetch patients with updated sort parameter when name or birthday headers are clicked (1509 ms)
showSummaryDashboard flag is true
✓ should show the summary dashboard instead of the standard patient table (2329 ms)
patient limit is reached
✓ should disable the add patient button (1100 ms)
✓ should show a popover with a link to the plans url if add patient button hovered (1138 ms)
tier0300 clinic
✓ should show and format patient data appropriately based on availablity (2758 ms)
✓ should refetch patients with updated sort parameter when sortable column headers are clicked (26183 ms)
✓ should allow refreshing the patient list and maintain (2594 ms)
✓ should show the time since the last patient data fetch (2542 ms)
✓ should allow filtering by last upload (7314 ms)
✓ should allow filtering by cgm use (6216 ms)
✓ should allow filtering by bg range targets that DO NOT meet selected criteria (15157 ms)
✓ should track how many filters are active (21929 ms)
✓ should reset all active filters at once (18272 ms)
✓ should clear pending filter edits when time in range filter dialog closed (10121 ms)
✓ should send an upload reminder to a fully claimed patient account (3736 ms)
summary period filtering
✓ should show the Filter Reset Bar (4772 ms)
✓ should allow filtering by summary period (9831 ms)
✓ should not show the GMI if selected period is less than 14 days (20260 ms)
persisted filter state
✓ should set the last upload filter on load based on the stored filters (4998 ms)
✓ should set the patient tag filters on load based on the stored filters (6056 ms)
✓ should set the time in range filters on load based on the stored filters (6236 ms)
✓ should fetch the initial patient based on the stored filters (4972 ms)
persisted sort state
✓ should set the table sort UI based on the the sort params from localStorage (5271 ms)
✓ should use the stored sort parameters when fetching the initial patient list (5251 ms)
mmol/L preferredBgUnits
✓ should show the bgm average glucose in mmol/L units (5843 ms)
✓ should show the bg range filters in mmol/L units (6289 ms)
managing patient tags
✓ should allow adding tags to a patient (4272 ms)
✓ Opens the Edit Patient modal when trying to add tags to patient not meeting MRN requirements (10655 ms)
Accessing TIDE dashboard
showTideDashboard flag is true
✓ should render the TIDE Dashboard CTA (2720 ms)
✓ should disable the TIDE Dashboard CTA if clinic has no patient tags defined (5039 ms)
✓ should not render the TIDE Dashboard CTA if clinic tier < tier0300 (4208 ms)
✓ should open a modal to configure the dashboard, and redirect when configured (12415 ms)
✓ should redirect right away to the dashboard if a valid configuration exists in localStorage (4817 ms)
✓ should open the config modal if an invalid configuration exists in localStorage (7835 ms)
showTideDashboard flag is false
✓ should not show the TIDE Dashboard CTA, even if clinic tier >= tier0300 (2825 ms)
Managing patient last reviewed dates
showSummaryDashboardLastReviewed flag is true
✓ should render the Last Reviewed column (3068 ms)
✓ should allow setting last reviewed date (3015 ms)
✓ should allow undoing last reviewed date (3120 ms)
✓ should refetch patients with updated sort parameter when Last Reviewed header is clicked (7435 ms)
✓ should not render the Last Reviewed column if showSummarData flag is false (3461 ms)
showSummaryDashboardLastReviewed flag is false
✓ should not show the Last Reviewed column, even if clinic tier >= tier0300 (2907 ms)
Generating RPM report
showRpmReport flag is true
✓ should render the RPM Report CTA (4302 ms)
✓ should not render the RPM Report CTA if clinic tier < tier0300 (4342 ms)
✓ should open a patient count limit modal if current filtered count is > 1000 (10686 ms)
✓ should open a modal to configure the report, and generate when configured (33558 ms)
✓ should call `exportRpmReport` with fetched report data (10584 ms)
showRpmReport flag is false
✓ should not show the TIDE Dashboard CTA, even if clinic tier >= tier0300 (2921 ms)
non-admin clinician
✓ should not render the remove button (722 ms)
| expect(savedConfig.period).to.equal('30d'); | ||
| expect(savedConfig.tags).to.be.an('array'); | ||
| expect(savedConfig.tags.length).to.be.above(0); | ||
| }, 15000); |
There was a problem hiding this comment.
15 second timeout can hopefully be reduced
clintonium-119
left a comment
There was a problem hiding this comment.
For some reason it didn't show my review as submitted, so re-submitting
Merges duplicate state checks in ClinicDetails and removes unused utility function in PatientData. Adjusts test timeouts and assertions to match updated data values. Sets global dev mode flag for testing environment.
|
@clintonium-119 Could I get a re-review for the changes/updates since the last review? (mostly a merge) |
WEB-4458 Enzyme to RTL migration
This pull request removes Karma and Enzyme from the test setup, fully migrating the project to Jest and React Testing Library. It updates configuration files, dependencies, and test utilities to support this change, and also includes improvements to test reliability and code quality. The most important changes are grouped below:
Testing Framework Migration:
karma.conf.js, Karma plugins, and related scripts frompackage.json. [1] F46d1020L46R7, [2]loadtests.jssetup file and removed Enzyme and its adapter from dependencies, replacing Enzyme usage with React Testing Library in test files. [1] [2] [3]package.jsonto use only Jest, simplifying test commands and removing concurrent/karma-specific scripts. (F46d1020L46R7)Test Environment and Utilities Updates:
jest.setup.jsto provide missing globals and mocks for improved test reliability, includingchai,sinon, browser APIs, and worker mocks. Also setglobal.__DEV__totruefor development-like test conditions.Dependency and Configuration Improvements:
@testing-library/jest-domand added@types/jestto dependencies. Added a resolution forcheerioto ensure compatibility. [1] [2]Code Quality and Robustness Enhancements:
Miscellaneous Updates:
This migration simplifies the testing setup, improves reliability, and aligns with modern React testing practices.