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

Support React 18 #613

Merged
merged 12 commits into from Sep 22, 2023
Merged

Support React 18 #613

merged 12 commits into from Sep 22, 2023

Conversation

chawes13
Copy link
Contributor

Resolves #568

Author Checklist

  • Add unit test(s)
  • Update version in package.json (see the versioning guidelines)
  • Update documentation (if necessary)
  • Add story to storybook (if necessary)
  • Assign dev reviewer

Comment on lines 518 to 520
await act(async () => {
await expect(upload(fileData, file)).rejects.toThrow()
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

React 18 enforces act more strictly. I couldn't figure out a way to get rid of the act warning without using it directly

Copy link
Contributor

Choose a reason for hiding this comment

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

The inner function doesn't have to be async. This works for me:

await act(() => expect(upload(fileData, file)).rejects.toThrow())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mostly just a syntax thing. You can either return the promise directly or await it in the body. I took your suggestion

Comment on lines 541 to 543
await act(async () => {
await user.click(screen.getByText('Upload'))
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know why waiting for upload-success works without showing an act console error, but simply waiting for upload-failure does not work.

Let me know if you have any insights here!

Copy link
Contributor

Choose a reason for hiding this comment

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

It probably doesn't help much, but I don't see any console errors removing the act wrapper here. Test passes without warnings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly act warnings are my kryptonite. I can't believe this works now 😭. Thank you 🙏

@chawes13
Copy link
Contributor Author

@josiasds I got started on this. There's still one test that is logging an act error to the console ("DateInput updates the value on change"). The stacktrace is complaining about r.setBlur from React date-picker. I could not figure out how to address it 😭. Open to your suggestions here!

@chawes13
Copy link
Contributor Author

Open issue related to this: testing-library/react-testing-library#1231

@chawes13 chawes13 marked this pull request as ready for review September 14, 2023 17:55
Copy link
Contributor

@josiasds josiasds left a comment

Choose a reason for hiding this comment

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

Looks good to me!

test/forms/inputs/date-input.test.js Outdated Show resolved Hide resolved

expect(input).toHaveValue('02/02/2023')
await user.click(input)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you need user.click here, as user.type already focuses on the element. The test passes without it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it open the popover menu though? I thought in doing testing earlier that it didn't open, but honestly I was trying a whole lot of things so maybe I was mistaken. I remember that my intent was to try and mimic the user's experience as much as possible

Comment on lines 541 to 543
await act(async () => {
await user.click(screen.getByText('Upload'))
})
Copy link
Contributor

Choose a reason for hiding this comment

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

It probably doesn't help much, but I don't see any console errors removing the act wrapper here. Test passes without warnings.

Comment on lines 518 to 520
await act(async () => {
await expect(upload(fileData, file)).rejects.toThrow()
})
Copy link
Contributor

Choose a reason for hiding this comment

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

The inner function doesn't have to be async. This works for me:

await act(() => expect(upload(fileData, file)).rejects.toThrow())

@chawes13 chawes13 merged commit 3567ea2 into main Sep 22, 2023
2 checks passed
@chawes13 chawes13 deleted the chore/support-react-18 branch September 22, 2023 13:21
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.

Add support for React 18
2 participants