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

Upgrade user-event to v14 #1605

Merged
merged 20 commits into from
Jun 14, 2022
Merged

Upgrade user-event to v14 #1605

merged 20 commits into from
Jun 14, 2022

Conversation

robinmetral
Copy link
Contributor

@robinmetral robinmetral commented Jun 8, 2022

Purpose

Upgrade to @testing-library/user-event v14.

Approach and changes

  • upgrade to @testing-library/user-event v14
    • all user-event methods now return promises and need to be awaited
    • some keyboard() method params were renamed, e.g {esc} 👉 {Escape}; {space} 👉 { }
    • when using Jest fake timers, I had to configure user-event using { delay: null }. Issue ref
  • migrate from the deprecated @testing-library/react-hooks to renderHook from @testing-library/react. Issue ref
  • addressed an axe error surfaced by an improved accessibility test (likely wasn't an issue, but a bad practice)
  • other minor fixes (TS, act() warnings...)

Definition of done

  • Development completed
  • Reviewers assigned
  • Unit and integration tests
  • Meets minimum browser support
  • Meets accessibility requirements

@changeset-bot
Copy link

changeset-bot bot commented Jun 8, 2022

🦋 Changeset detected

Latest commit: f155d81

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sumup/circuit-ui Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link

vercel bot commented Jun 8, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
oss-circuit-ui ✅ Ready (Inspect) Visit Preview Jun 13, 2022 at 2:55PM (UTC)

Robin Métral added 15 commits June 13, 2022 11:40
- removed the mocked document.createRange. This isn't necessary anymore. See https://github.com/testing-library/user-event/issues/902\#issuecomment-1092886633
- awaited userEvent calls, removed unnecessary act blocks
- fixed type errors around icon types
- note: there are still act warnings for some tests in the specs, even some not involving any state updates (style snapshots). Probably points to unwanted rerenders. Added a fixme comment.
- exposed the renderHook method from @testing-library/react, meant to replace the onoe from @testing-library/react-hooks, which isn't compatible with React 18. For migration purposes, I'm naming it newRenderHook--I'll remove the legacy method and rename the new one after all specs have been migrated
- awaited userEvent calls and removed unnecessary act blocks
The live region wrapping toasts should not be a ul, because uls son't allow the status role (likely because it overrides list semantics anyways. Switched for divs.

Found through a better axe test
- await userEvent method calls
- added a new render helper to render a component tree including the ToastProvider, to reduce test boilerplate when testing business logic
- improved the accesibility test to cover an actual document with a toast instead of the toast UI in isolation
- awaited userEvent methods and removed unnecessary act blocks
- migrated useSidePanel to newRenderHook and removed unnecessary actHook blocks
- added ariaHideApp=false to default test props in SidePanelContext to silence react-modal warning about the undefined app element
- set up userEvent with delay=null in SidePanelContext to address this issue: testing-library/user-event#833
- TODO: two tests using userEvent.keyboard() are still failing. Marked as todo.
- TODO: there are two act() warnings left (likely from before) when running `yarn test sidepanel` (all SidePanel-related specs)
This also adds comments on accessibility tests that inconsistently trigger axe warnings. Will be investigated and addressed separately.
Checkbox, CurrencyInput, Selector.

Awaited userEvent method calls
Awaited userEvent method calls
- awaited userEvent method calls
- set up userEvent with delay=null in ModalContext spec (same as in SidePanelContext)
Hamburger, Pagination, PageList, UtilityLinks, useClickOutside.

Awaited userEvent method calls.

Does not include the migration to new renderHook for useClickOutside, will be handled separately.
Same as in the ModalProvider and SidePanelProvider
- awaited method calls
- migrated {space} to { }
- TODO: migrate to the new renderHook, will be addressed separately
- await method calls
- fix some TS errors
Robin Métral added 3 commits June 13, 2022 12:08
- replace imports
- replace removed waitForNextUpdate by a more explicit waitFor
- remove tests covering error logic. Error testing was removed in the renderHook port to RTL and we're not covering this in other components, so I think it's fine to remove.

See testing-library/react-testing-library#991 for more context
See testing-library/react-testing-library#991 for context.

- replaced import of renderHook to newRenderHook (will be renamed in a follow-up commit)
- replaced actHook by act
- (edge cases were handled in earlier commits)
Not sure why these specs passed locally before
@codecov
Copy link

codecov bot commented Jun 13, 2022

Codecov Report

Merging #1605 (f155d81) into main (df21962) will increase coverage by 0.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1605      +/-   ##
==========================================
+ Coverage   92.69%   92.77%   +0.07%     
==========================================
  Files         194      194              
  Lines        4026     4026              
  Branches     1258     1258              
==========================================
+ Hits         3732     3735       +3     
+ Misses        275      272       -3     
  Partials       19       19              
Impacted Files Coverage Δ
packages/circuit-ui/util/test-utils.tsx 100.00% <ø> (ø)
...ircuit-ui/components/ToastContext/ToastContext.tsx 92.30% <100.00%> (+2.56%) ⬆️
...ckages/circuit-ui/components/Step/hooks/useStep.ts 97.75% <0.00%> (-2.25%) ⬇️
...mponents/NotificationInline/NotificationInline.tsx 96.36% <0.00%> (+3.63%) ⬆️
...components/NotificationToast/NotificationToast.tsx 93.61% <0.00%> (+4.25%) ⬆️

@robinmetral robinmetral added ♿ accessibility Improves usability 📦 dependencies Pull requests that update a dependency file labels Jun 13, 2022
@robinmetral robinmetral marked this pull request as ready for review June 13, 2022 11:37
@robinmetral robinmetral requested a review from a team as a code owner June 13, 2022 11:37
@robinmetral robinmetral requested review from amelako and connor-baer and removed request for a team and amelako June 13, 2022 11:37
@sumup-oss sumup-oss deleted a comment from sumup-clark bot Jun 13, 2022
Copy link
Member

@connor-baer connor-baer left a comment

Choose a reason for hiding this comment

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

This looks like it was tedious. Thank you for taking care of it! I like the reduction of boilerplate code (no more act, yay!)

I think you can avoid most of the explicit types/typecasting by adding as const to the props objects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
♿ accessibility Improves usability 🗂 circuit-ui 🗂 cna-template 📦 dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants