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

Fix Test Warnings in VxAdmin #2801

Merged
merged 2 commits into from
Nov 30, 2022
Merged

Fix Test Warnings in VxAdmin #2801

merged 2 commits into from
Nov 30, 2022

Conversation

adghayes
Copy link
Collaborator

With the introduction of React Query, we are getting a lot test warnings about updates to unmounted components. This can make it hard to read the test output and can obscure more concerning warnings.

The cause seems to be that React Query is fetching and making state updates after tests have ended. RTL good practice would be to wait for changes on the screen before ending or continuing the test. But in some cases, there are no changes on the screen, because the data is simply refetching or it is fetching data for which we have a default value (so there is no change on screen). I'll comment to point out some of the different cases.

I don't necessarily intend to merge this PR, but I was curious to see what it took to sort out the warnings and to start a discussion around it.

@@ -494,7 +495,7 @@ test('printing ballots and printed ballots report', async () => {
userEvent.click(screen.getByRole('button', { name: 'Continue' }));

// Printing should now continue normally
await waitFor(() => getByText('Printing'));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In this printing modal case, we simply weren't waiting for the modal to close before moving on. Not a React Query issue.

Comment on lines 732 to 735
// useQuery's in AppRoot are refetching data, and there's no change to wait on
await advanceTimersAndPromises();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unclear to me what was being fetched and why, but warnings indicated it was one of the hooks in AppRoot.

Comment on lines 1094 to 1114
await screen.findByText('Currently tallying live ballots.');
// We're waiting on a query for isOfficialResults. It has a default value,
// so there is no change on the page to wait for before test ends.
// Await promises to avoid test warning.
await advanceTimersAndPromises();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Case where we have a default value and there's nothing we could wait on.

Comment on lines 1245 to 1264
// We're waiting on a query for the file mode (live vs. test).
// It has a default value, so there is no change on the page when loaded.
// Await promises to avoid test warning.
await advanceTimersAndPromises();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This can happen in the middle of a test as well, not just at the end.

Comment on lines +49 to +52
await act(async () => {
await sleep(1);
});
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In this case, advancePromises didn't do the trick. I actually had to sleep. Could probably do the same by using fake timers and using advanceTimersAndPromises

Comment on lines +16 to +19
await act(async () => {
await sleep(1);
});
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

First three tests in this file all had the same problem.

@adghayes adghayes marked this pull request as ready for review November 29, 2022 23:16
@adghayes adghayes requested a review from a team as a code owner November 29, 2022 23:16
@adghayes adghayes requested review from arsalansufi and removed request for a team November 29, 2022 23:16
@adghayes
Copy link
Collaborator Author

@kofi-q my understanding from our discussion is that, if we want to avoid warnings in the present, we may want to merge these little fixes. I added comments with TODO's that indicate to remove random waits after we complete #1660 and upgrade to React 18.

@kofi-q
Copy link
Contributor

kofi-q commented Nov 30, 2022

@kofi-q my understanding from our discussion is that, if we want to avoid warnings in the present, we may want to merge these little fixes. I added comments with TODO's that indicate to remove random waits after we complete #1660 and upgrade to React 18.

yeah, that SGTM! having felt the pain of sorting through the console spam recently, i'm in favour of merging 😅

Copy link
Contributor

@kofi-q kofi-q left a comment

Choose a reason for hiding this comment

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

thanks for digging into this!

@adghayes adghayes merged commit 3918b9b into main Nov 30, 2022
@adghayes adghayes deleted the drew/admin-test-warnings branch November 30, 2022 17:45
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