Skip to content

[test] Refactor Pickers tests to async user-event#22043

Open
LukasTy wants to merge 15 commits intomui:masterfrom
LukasTy:claude/trusting-benz
Open

[test] Refactor Pickers tests to async user-event#22043
LukasTy wants to merge 15 commits intomui:masterfrom
LukasTy:claude/trusting-benz

Conversation

@LukasTy
Copy link
Copy Markdown
Member

@LukasTy LukasTy commented Apr 9, 2026

Summary

Reapply the still-relevant parts of #14583 on top of the Vitest migration. The fake-clock removal is already in master; what remained was:

  • Converting Pickers tests to async and driving them with @testing-library/user-event instead of the sync fireEvent / fireUserEvent.
  • Refactoring the shared test helpers in test/utils/pickers/ to expose only async APIs.
  • Fixing a latent useFieldRootProps bug that prevented Ctrl+A select-all from working when the keyboard event was dispatched via user-event (because the handler checked the deprecated event.keyCode).

Infrastructure changes (test/utils/pickers/)

  • fields.tsx — removed the sync selectSection, pressKey, testFieldKeyPress, and testFieldChange helpers; the former *Async variants are now the canonical ones. pressKey maps navigation keys (ArrowUp, Backspace, Home, ...) to user-event's curly-brace syntax, treats ' ' as {Space}, and treats '' (the legacy "clear section" signal) as {Backspace}.
  • openPicker.ts — removed the sync variant; openPicker(user, params) is now async. A small clickTarget helper falls back to fireEvent.click when user-event refuses to interact with pointer-events: none so disabled / readOnly open tests still work.
  • viewHandlers.tstimeClockHandler, digitalClockHandler, and multiSectionDigitalClockHandler are async and receive user as the first argument. fireTouchChangedEvent calls stay (no user-event equivalent for multi-touch) but are wrapped in act(async () => ...).
  • assertions.tsexpectPickerChangeHandlerValue(type, value, expected) now takes the raw value instead of a sinon spy, matching the original PR's signature.
  • describeValue/*setNewValue callbacks receive { user, selectSection, pressKey, ... } and return Promise<TValue>. All four test*.tsx helpers (testPickerOpenCloseLifeCycle, testPickerActionBar, testShortcuts, testControlledUnControlled) migrated to await user.click / user.keyboard.
  • describePicker.tsx — two slot-forwarding tests migrated to user.click.

Test files (~65 files across x-date-pickers and x-date-pickers-pro)

All fireEvent.* / fireUserEvent.* / sync helper calls replaced with await user.* equivalents, tests made async. fireEvent.click is kept for a small number of places where the exact synthetic-event sequence matters:

  • Performance tests asserting render counts — user.click fires a fuller pointer sequence (pointerover, pointerdown, pointerup, ...) that triggers extra hover/focus renders.
  • Click-on-disabled-element tests — user.click refuses pointer-events: none, so the click is ignored before the assertion can run.
  • Drag-and-drop and touch helpers — no user-event equivalent for dragstart/drop or multi-touch.

Each such exception has an inline comment explaining why.

Component fix

useFieldRootProps.ts had a latent select-all bug: the handler was checking String.fromCharCode(event.keyCode) === 'A', but user-event v14 doesn't set the deprecated keyCode property. This made every Ctrl+A select-all test silently no-op when driven by user-event. Added event.key?.toUpperCase() === 'A' as the primary check, with the old keyCode path kept as a fallback, so Ctrl+A triggers select-all regardless of how the keyboard event was dispatched.

Why now

The original PR was parked waiting for the move away from mocha. That move is done (the repo runs on vitest now), and the flakiness introduced by sinon's fake clocks is gone. The async/user-event portion is the remaining value from the original PR: more realistic test interactions and no more sync fireEvent / fireUserEvent noise in Pickers tests.

Test plan

  • pnpm --filter "@mui/x-date-pickers*" run typescript
  • pnpm test:unit --project "x-date-pickers" --run — 3241 passed, 814 skipped
  • pnpm test:unit --project "x-date-pickers-pro" --run — 986 passed, 299 skipped, 2 todo
  • pnpm eslint
  • pnpm prettier
  • Grep to confirm zero remaining fireUserEvent, selectSectionAsync, pressKeyAsync, openPickerAsync, clock: 'fake' in Pickers sources

Draft because the diff is large (~81 files, +1.7k/-1.7k) and the exceptions above would benefit from a review pass. Happy to split into smaller commits if preferred.

🤖 Generated with Claude Code

Reapply the still-relevant parts of mui#14583 on top of the Vitest migration.
The fake-clock removal is already in master; what remained was converting
tests to async + `@testing-library/user-event`, refactoring the shared
helpers, and dropping the deprecated sync variants.

Infrastructure changes in `test/utils/pickers/`:

- `fields.tsx`: removed sync `selectSection`, `pressKey`,
  `testFieldKeyPress`, and `testFieldChange`; the former `*Async`
  variants are now the canonical ones. `pressKey` maps navigation keys
  (`ArrowUp`, `Backspace`, `Home`, ...) to user-event's curly-brace
  syntax, treats `' '` as `{Space}`, and treats `''` (the legacy
  "clear section" signal) as `{Backspace}`.
- `openPicker.ts`: removed sync variant; `openPicker(user, params)` is
  async. A small `clickTarget` helper falls back to `fireEvent.click`
  when user-event refuses to interact with `pointer-events: none` so
  `disabled` / `readOnly` open tests still work.
- `viewHandlers.ts`: `timeClockHandler`, `digitalClockHandler`, and
  `multiSectionDigitalClockHandler` are now async and receive `user`
  as the first argument. `fireTouchChangedEvent` calls stay (no
  user-event equivalent for multi-touch) but are wrapped in
  `act(async () => ...)`.
- `assertions.ts`: `expectPickerChangeHandlerValue(type, value, expected)`
  now takes the raw value instead of a sinon spy, matching the original
  PR's signature.
- `describeValue/*`: `setNewValue` callbacks receive `{ user,
  selectSection, pressKey, ... }` and return `Promise<TValue>`. All
  four `test*.tsx` helpers (`testPickerOpenCloseLifeCycle`,
  `testPickerActionBar`, `testShortcuts`, `testControlledUnControlled`)
  migrated to `await user.click` / `user.keyboard`.
- `describePicker.tsx`: two slot-forwarding tests migrated to
  `user.click`.

Test files (~65 files across x-date-pickers and x-date-pickers-pro):
all `fireEvent.*` / `fireUserEvent.*` / sync helper calls replaced
with `await user.*` equivalents, tests made `async`. `fireEvent.click`
kept for a small number of places where the exact synthetic-event
sequence matters:

- performance tests asserting render counts (user-event fires a fuller
  pointer sequence that triggers extra renders),
- click-on-disabled-element tests where `user.click` refuses
  `pointer-events: none`,
- drag-and-drop and touch helpers (no user-event equivalent).

`useFieldRootProps.ts` had a latent select-all bug: the handler was
checking `String.fromCharCode(event.keyCode) === 'A'`, but user-event
v14 does not set the deprecated `keyCode` property. Added
`event.key?.toUpperCase() === 'A'` as the primary check so Ctrl+A
triggers select-all regardless of how the keyboard event was
dispatched.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@mui-bot
Copy link
Copy Markdown

mui-bot commented Apr 9, 2026

Deploy preview: https://deploy-preview-22043--material-ui-x.netlify.app/

Bundle size report

Bundle Parsed size Gzip size
@mui/x-data-grid 0B(0.00%) 0B(0.00%)
@mui/x-data-grid-pro 0B(0.00%) 0B(0.00%)
@mui/x-data-grid-premium 0B(0.00%) 0B(0.00%)
@mui/x-charts 0B(0.00%) 0B(0.00%)
@mui/x-charts-pro 0B(0.00%) 0B(0.00%)
@mui/x-charts-premium 0B(0.00%) 0B(0.00%)
@mui/x-date-pickers 🔺+30B(+0.01%) 🔺+10B(+0.02%)
@mui/x-date-pickers-pro 🔺+30B(+0.01%) 🔺+7B(+0.01%)
@mui/x-tree-view 0B(0.00%) 0B(0.00%)
@mui/x-tree-view-pro 0B(0.00%) 0B(0.00%)

Details of bundle changes

Generated by 🚫 dangerJS against 0621db8

@LukasTy LukasTy added test scope: pickers Changes related to the date/time pickers. type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature. labels Apr 9, 2026
LukasTy and others added 7 commits April 9, 2026 13:16
Pure formatting pass on top of the async/user-event refactor — no test
behavior changes.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
LukasTy and others added 6 commits April 9, 2026 16:39
When the "Open previous view" or "Open next view" button is clicked and
the resulting state update disables that same button, focus stays on the
now-disabled element. That traps keyboard events on an unreachable target,
and in practice swallows the picker's Escape-to-dismiss handler — it was
only observable because the refactored MobileTimePicker test needed a
`user.click(dialog)` workaround to move focus off the disabled button
before pressing Escape.

Track the previous disabled state via a ref and, when a button
transitions from enabled to disabled while it still owns
`document.activeElement`, move focus to its still-enabled sibling (or
blur as a fallback when both sides are disabled). Refs are attached via
`useForkRef` so existing slot-level ref overrides keep working.

This fixes the underlying issue so the test-level workaround in
`describeValue.MobileTimePicker.test.tsx` is no longer needed, and it
improves accessibility in real browsers too: keyboard users that press
the arrow switcher land on a sensible focus target instead of relying
on the browser's implicit blur-on-disable behavior.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@LukasTy
Copy link
Copy Markdown
Member Author

LukasTy commented Apr 9, 2026

Claude Opus 4.6 review

Overall

Well-structured migration — 81 files, +1.6k/-1.6k lines, and the diff is largely mechanical. The approach is sound: consolidate on @testing-library/user-event for more realistic interactions, drop the old sync fireEvent/fireUserEvent wrappers, and remove the now-unnecessary *Async name suffixes.

Component Fix (useFieldRootProps.ts)

The event.key?.toUpperCase() === 'A' addition is correct and well-reasoned. event.keyCode is deprecated and user-event v14 doesn't set it. Keeping the String.fromCharCode(event.keyCode) fallback is fine for backward compatibility with real browser events. No concerns here.

Test Utilities

fields.tsx — Clean consolidation. The pressKey helper correctly maps navigation keys to user-event's {Key} syntax, handles ' '{Space}, and ''{Backspace}. The forEachfor...of migration in testFieldChange is necessary for sequential await and correct.

openPicker.ts — The clickTarget fallback pattern (catch → check message string → fireEvent.click) is pragmatic but fragile — it relies on user-event's error message containing "pointer-events: none". Consider a more defensive approach:

const clickTarget = async (user: MuiRenderResult['user'], target: Element) => {
  const style = getComputedStyle(target);
  if (style.pointerEvents === 'none') {
    fireEvent.click(target);
    return;
  }
  await user.click(target);
};

This avoids string-matching on error messages which could break on user-event version bumps.

viewHandlers.ts — Wrapping fireTouchChangedEvent in act(async () => ...) is correct since there's no user-event equivalent for multi-touch. The unused _ parameter in timeClockHandler.setViewValue is fine for interface conformance.

assertions.ts — Good change to decouple from sinon's spy API. Passing raw values makes the helper library-agnostic.

describePicker: onTouchStartonPointerDown (describePicker.tsx)

This is a behavioral test change, not just a migration. The tests previously verified onTouchStart forwarding on the DesktopPaper and Popper slots. Now they verify onPointerDown. While both work (MUI spreads props to <div>), the tests are now testing a different event. Note that DesktopDateRangePicker.test.tsx still tests onTouchStart using fireEvent.touchStart. Consider either:

  • Keeping onTouchStart + fireEvent.touchStart in describePicker too (consistency), or
  • Documenting why the generic test now covers a different event than the component-specific one.

Test Files — Pattern Observations

The migration pattern is consistent across all ~65 files:

  • it(...)async functions
  • view.selectSectionawait view.selectSection
  • view.pressKey(sectionIndex, key)await view.pressKey(key) (section index removed — relies on focus state)
  • fireEvent.click(...)await user.click(...)
  • expectPickerChangeHandlerValue(type, spy, expected)expectPickerChangeHandlerValue(type, spy.lastCall.firstArg, expected)
  • forEach loops with await correctly converted to for...of

The fireEvent exceptions are well-documented in inline comments and are legitimate (paste events, hidden input changes, touch handlers, render count tests, disabled element clicks).

Minor Nits

  1. eslint-disable no-await-in-loop — appears in several for...of loops that intentionally need sequential execution. Correct, but consider whether a project-level rule override for test files would be cleaner than scattered inline disables.

  2. The setNewValue call sites now pass user alongside selectSection and pressKey. The naming is clear, but the options object is getting wide — not a blocker for this PR though.

Verdict

Approve with one suggestion: replace the string-based error catch in clickTarget with a proactive getComputedStyle check. Everything else is clean, well-documented, and correctly migrated.

@LukasTy LukasTy marked this pull request as ready for review April 9, 2026 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

scope: pickers Changes related to the date/time pickers. test type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants