Skip to content

Conversation

@einouqo
Copy link
Contributor

@einouqo einouqo commented Jan 12, 2026

Some tests currently pass only because the assertions inside the pending ⁠setTimeout callbacks are never actually run.

bug.mp4

From my understanding, assertions scheduled by setTimeout while the the timer is faked with vitest aren't actually executed until one of the vi.advanceTimer* functions are called, and everything that isn't executed by the end of the test is dropped (this is why assertion doesn't actually check anything in the fixed tests)

The following test demonstrates the issue. It should fail because ⁠immediateCallback is never passed to the hook and cannot possibly be called, yet the test currently passes:

it('should call immediateCallback before the first frame', () => {
const immediateCallback = vi.fn()
renderHook(() =>
useRafLoop(callback, {
immediate: true,
immediateCallback: true,
}),
)
setTimeout(() => {
expect(immediateCallback).toHaveBeenCalled()
})
})

it('should call immediateCallback before the first frame', () => {
  const immediateCallback = vi.fn() // This mock is never passed to the hook

  renderHook(() =>
    useRafLoop(callback, { // This takes `callback` as the first argument, not `immediateCallback`
      immediate: true,
      immediateCallback: true, 
    }),
  )

  setTimeout(() => {
    // This assertion is never reached because timers aren't advanced
    expect(immediateCallback).toHaveBeenCalled() 
  })
})

What was changed

  • slight test redesign for every useRaf* hook, ensuring assertion
  • revised tests, reduced redundancy
  • dependency update - bumped vitest from v2 to v3 (necessary for requestAnimationFrame logic testing without hacky mocks), while keeping the vite dependency untouched

Copilot AI review requested due to automatic review settings January 12, 2026 00:59
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a critical testing issue where assertions inside setTimeout callbacks were never executed because fake timers weren't being advanced. The fix involves upgrading vitest from v2 to v3 (which provides better requestAnimationFrame support) and refactoring all useRaf* hook tests to use vi.advanceTimersToNextFrame() instead of relying on unevaluated setTimeout callbacks.

Changes:

  • Upgraded vitest from v2.1.9 to v3.2.4 and @vitest/coverage-v8 accordingly
  • Migrated vitest configuration from workspace-based to projects-based config
  • Refactored tests for useRafState, useRafLoop, and useRafFn to properly execute assertions using vi.advanceTimersToNextFrame()

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
vitest.workspace.ts Deleted workspace configuration file
vitest.config.ts Created new projects-based configuration
package.json Updated vitest and @vitest/coverage-v8 to v3.2.4
pnpm-lock.yaml Updated lockfile with vitest v3 dependencies
use-raf-state/index.test.ts Replaced setTimeout assertions with vi.advanceTimersToNextFrame()
use-raf-loop/index.test.ts Fixed immediateCallback test bug and updated timer advancement
use-raf-fn/index.test.ts Simplified tests and fixed timer advancement
Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

expect(callback).toHaveBeenCalled()
})

it('should call immediateCallback before the first frame', () => {
Copy link

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

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

The test name 'should call immediateCallback before the first frame' is misleading because immediateCallback: true is a configuration option that tells the hook to call the main callback immediately, not a separate callback. Consider renaming this test to something like 'should call callback immediately when immediateCallback option is true' to better reflect what is being tested.

Suggested change
it('should call immediateCallback before the first frame', () => {
it('should call callback immediately when immediateCallback option is true', () => {

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if maintainers find the name better, it can be renamed

Copy link
Member

@vikiboss vikiboss 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 the PR, LGTM.

@vikiboss vikiboss merged commit e1259b6 into sheinsight:main Jan 12, 2026
1 check 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.

2 participants