Skip to content

Conversation

@sukvvon
Copy link
Contributor

@sukvvon sukvvon commented Sep 13, 2025

Related Bug Reports or Discussions

Fixes #

Summary

This PR migrates all tests from real timers to Vitest fake timers to improve test determinism and execution speed. The migration includes comprehensive updates across 33 test files, eliminates most React act warnings, and removes the @testing-library/user-event dependency in favor of fireEvent.

Key Changes

  1. Migrate to Vitest fake timers - Add beforeEach(vi.useFakeTimers) and afterEach(vi.useRealTimers)
  2. Replace userEvent with fireEvent - Use synchronous event simulation (userEvent uses real timers internally)
  3. Replace Promise resolver pattern with setTimeout delays - Convert manual promise resolution to timer-controlled delays
  4. Replace Testing Library async utilities - Switch from findByText/waitFor to getByText with explicit timer control
  5. Add loading state assertions - Verify Suspense loading states during async transitions
  6. Fix React act warnings - Wrap operations properly to eliminate console warnings

Why Replace userEvent with fireEvent?

  • userEvent uses real timers internally, conflicting with fake timers
  • fireEvent is synchronous and fully compatible with vi.useFakeTimers()
  • For state management testing, we need logic correctness, not interaction realism
  • Simpler, faster, and one less dependency

fireEvent Pattern (Simple Rule)

Use await act(() => fireEvent.click(...)) when immediately checking Suspense loading state.

Otherwise use plain fireEvent.click(...).

Examples

With Suspense (need act):

await act(() => fireEvent.click(screen.getByText('button')))
expect(screen.getByText('loading')).toBeInTheDocument() // ← checking loading right after
await act(() => vi.advanceTimersByTimeAsync(100))
expect(screen.getByText('result')).toBeInTheDocument()

Without Suspense (plain fireEvent):

fireEvent.click(screen.getByText('button')) // ← no loading check needed
expect(screen.getByText('count: 1')).toBeInTheDocument()

Pro tip: When in doubt, try plain fireEvent first. If the test fails or shows act warnings, wrap with await act().

Statistics

  • Files changed: 33 test files + package.json
  • Lines changed: +2,249 / -2,229 (net +20)
  • Dependencies removed: @testing-library/user-event

Check List

  • pnpm run fix for formatting and linting code and docs

@vercel
Copy link

vercel bot commented Sep 13, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
jotai Ready Ready Preview Comment Dec 1, 2025 8:39am

@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 13, 2025

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Sep 13, 2025

More templates

npm i https://pkg.pr.new/jotai@3147

commit: e513fe3

@github-actions
Copy link

github-actions bot commented Sep 13, 2025

LiveCodes Preview in LiveCodes

Latest commit: e513fe3
Last updated: Dec 1, 2025 8:39am (UTC)

Playground Link
React demo https://livecodes.io?x=id/6RNESTAFA

See documentations for usage instructions.

…ByText' with 'getByText', and add fake timers
…ByText' with 'getByText', and remove 'waitFor'
…t' with 'getByText', add fake timer, and remove 'waitFor'
@dai-shi
Copy link
Member

dai-shi commented Nov 30, 2025

In Jotai's case, it doesn't really matter. fire-event may be slightly better here for performance reasons since it's only testing the library.

Got it now. Thanks for your tip.

Copy link
Member

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

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

Finally, I reviewed all files. Hope I don't miss anything. If I do, hope you can cover it.

Comment on lines 36 to 46
const asyncAtom = atom(async () => 42)
const asyncAtom = atom(async () => {
await new Promise((resolve) => setTimeout(resolve, 10))
return 42
})
Copy link
Member

Choose a reason for hiding this comment

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

Could you revert the change in this PR?
I only find it in this file, but please check all other files too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dai-shi Reverted to immediately-resolving async atom with vi.advanceTimersByTimeAsync(0) in useAtomValue.test.tsx (9125c21)

Comment on lines 64 to 66
expect(screen.getByText('loading')).toBeInTheDocument()

expect(await screen.findByText('value: 42')).toBeInTheDocument()
await act(() => vi.advanceTimersByTimeAsync(10))
Copy link
Member

Choose a reason for hiding this comment

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

Though, this is a good addition to our tests. Let's do that after merging this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was fixed with 9125c21.

Let's do that after merging this PR

I agree with your opinion.

Comment on lines 266 to 267
// we need a small delay to reproduce the issue
await new Promise((r) => setTimeout(r, 10))
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't feel valid to remove this. It means to remove the entire spec. Would it be possible for you to try reverting #1481 temporary on your end locally and learn how to reproduce the issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dai-shi Restored the 10ms delay using fake timer: await act(() => vi.advanceTimersByTimeAsync(10)) (25e2ba6)

Comment on lines 790 to 789
await new Promise((r) => setTimeout(r))
await Promise.resolve()
Copy link
Member

Choose a reason for hiding this comment

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

This changes the behavior. Can you revert?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dai-shi Reverted to await new Promise((resolve) => setTimeout(resolve)) (363e28a)

get(countDerivedAtom)
set(countAtom, 2)
})

Copy link
Member

Choose a reason for hiding this comment

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

Could you remove changes that only add new lines? It makes the diff bigger.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dai-shi These blank lines improve readability by separating logical sections. I'd prefer to keep them if possible.

Copy link
Member

Choose a reason for hiding this comment

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

Well, you might have noticed that my preference is fewer lines. Anyway, it's just stylistic. I will change them later if I want to.

Comment on lines 148 to 162
await store.get(asyncAtom)

await vi.advanceTimersByTimeAsync(100)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if it's testing the same thing, but should be fine.

Copy link
Contributor Author

@sukvvon sukvvon Nov 30, 2025

Choose a reason for hiding this comment

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

It was fixed with f674996

Comment on lines 120 to 125
store.set(syncAtom, Promise.resolve(2))
store.set(
syncAtom,
new Promise<number>((resolve) => setTimeout(() => resolve(2), 100)),
)
Copy link
Member

Choose a reason for hiding this comment

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

I saw this kind of changes many times, but I wonder if we could simply do vi.advanceTimersByTimeAsync(0), instead of changing the atom definition. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted to Promise.resolve() with vi.advanceTimersByTimeAsync(0) in unwrap.test.ts and loadable.test.ts (f674996)

Copy link
Member

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

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

I'm not surprised if I missed anything, but let's merge this for now.
Thanks for your contribution!

get(countDerivedAtom)
set(countAtom, 2)
})

Copy link
Member

Choose a reason for hiding this comment

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

Well, you might have noticed that my preference is fewer lines. Anyway, it's just stylistic. I will change them later if I want to.

@dai-shi dai-shi merged commit 08d0a85 into pmndrs:main Dec 1, 2025
45 checks passed
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.

3 participants