Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RW-7610][risk=no] Unmount enzyme React components after each test case #5961

Merged
merged 2 commits into from
Nov 30, 2021

Conversation

calbach
Copy link
Contributor

@calbach calbach commented Nov 30, 2021

Turns out there is no automated teardown of enzyme provided by our test harness after each test case, per enzymejs/enzyme#911

The solution described above caused some complications, as using a common DOM root caused an additional unmount on every mount() call within the same test case. It's possible that fixing the issues that arise there would be a more pure solution, but it would likely require retrofitting a bunch of our components.

Specifically, it would result in this error when attempting to reattach the component:

  console.error                                                                                                                                                                                 
    Warning: Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronou
s tasks in a useEffect cleanup function.                                                                                                                                                        
        in Unknown (at with-error-modal.tsx:68)                                                                                                                                                 
        in ProfileErrorWrapper (at with-error-modal.tsx:45)                                                                                                                                     
        in ErrorModalWrapper (at data-access-requirements.spec.tsx:32)                                                                                                                          
        in Router (created by MemoryRouter)                                                                                                                                                     
        in MemoryRouter (created by WrapperComponent)                                                                                                                                           
        in WrapperComponent           

This change results in a large decrease in test latency for me locally and specifically reduced data-access-requirements test case runtime significantly, which were seemingly flaking due to large callback cruft build-up. From a few data points on circle appears to reduce test time by about half:

~2m50s -> ~1m20s

@calbach calbach marked this pull request as ready for review November 30, 2021 00:12
Copy link
Collaborator

@jmthibault79 jmthibault79 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome. Until now, I don't think my local UI test suite has fully passed since Angular. (Individual tests were fine, which makes sense)

@@ -8,13 +8,13 @@ require('mutationobserver-shim');
*/
const enzyme = require("enzyme");
const Adapter = require("enzyme-adapter-react-16");
const reactDOM = require("react-dom");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this appears to be unused

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no change when I comment this out

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, thanks. This was leftover from the previous approach

@calbach
Copy link
Contributor Author

calbach commented Nov 30, 2021

I've filed https://precisionmedicineinitiative.atlassian.net/browse/RW-7617 to track the ongoing Puppeteer notebook failures. These are completely unrelated to this PR, which only affects UI unit test code and has no bearing on the UI code itself, or the e2e tests.

@calbach
Copy link
Contributor Author

calbach commented Nov 30, 2021

@calbach calbach merged commit 3a9c171 into master Nov 30, 2021
@calbach calbach deleted the ch/ui-test-deflake branch November 30, 2021 23:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants