Skip to content

test: synchronize async effects while preserving negative assertions - #160

Merged
elonehoo merged 2 commits into
mainfrom
test/155-deterministic-async
Oct 21, 2026
Merged

elonehoo merged 2 commits into
mainfrom
test/155-deterministic-async

Conversation

@elonehoo

@elonehoo elonehoo commented Oct 21, 2026 •

Copy link
Copy Markdown
Member

Summary

Fixes #155.

  • Replace FloatingPanel's guessed 10/40ms delays with waits for the actual callback conditions; verify controlled state, callback uniqueness and stale-listener silence in a separate bounded observation window after positive callback completion.
  • Wait for NumberInput's input/caret reconciliation animation frame rather than guessing 35ms. The pinned core schedules both in a single raf callback.
  • Preserve the existing 35ms post-unmount observation window in React and Vue. Do not replace negative assertions with immediately passing waitFor.
  • Add a delayed unwanted callback negative control. Removing the observation wait causes this test to fail.

No production behavior, skipped-test cleanup, Node policy, or release changes.

Verification

  • 332 React/Vue migration tests passed in happy-dom.
  • 8 focused unit tests passed.
  • Negative control mutation: removing the observation wait produced the expected failure; restored patch passes.
  • React and Vue type checks passed.
  • Full lint: 0 errors (667 existing warnings).
  • Independent static review: no blocking findings.
  • Local Chromium could not start because the matching executable was unavailable. Remote full browser suites subsequently passed on Node 22.22.3 and 24.
  • All seven GitHub Actions workflows passed on final head 6721e714dd136062c282588b8d7b35b03f428134, including full CI/type checks, docs validation/build, all four Storybooks, packed consumers, lint, and browser accessibility smoke. Full CI evidence.
  • Both automatic Vercel checks succeeded; no manual deployment was performed.
  • Post-Ready review suggestions addressed in the second commit. Final helper mutation control was rerun (red without the wait, green after restoring it); 8 focused unit tests pass. Awaiting refreshed automated review of the final head.

View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.


Note

Low Risk
Test-only timing and assertion refactors; no production code paths change.

Overview
Introduces a shared assertAfterObservationWindow helper (35ms) so tests can prove no late callbacks fire after unmount or controlled vetoes—something vi.waitFor cannot do because it would pass on initial silence.

React and Vue controllable-migration dialog lifecycle tests use the helper instead of inline setTimeout(35). FloatingPanel unit tests drop guessed 10/40ms sleeps: positive work waits on vi.waitFor for real callback/state conditions, then the observation window guards against extra onOpenChange echoes. NumberInput caret tests wait one requestAnimationFrame (documented as matching core reconciliation) instead of a fixed 35ms delay.

Adds a small unit test for the observation-window behavior (including a negative control that fails if the wait is removed).

Reviewed by Cursor Bugbot for commit 6721e71. Bugbot is set up for automated code reviews on this repo. Configure here.

@vercel

vercel Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

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

Project Deployment Actions Updated
destyler-ui Ready Ready Preview Oct 21, 2026 10:10am UTC
destyler-ui-dev Ready Ready Preview Oct 21, 2026 10:10am UTC

@elonehoo
elonehoo marked this pull request as ready for review October 21, 2026 09:54
@elonehoo

Copy link
Copy Markdown
Member Author

🤖 Review(中文)

概要:本 PR 只改测试:把 FloatingPanel 和 NumberInput 测试里靠猜的 10/40/35ms 延时,换成等待真实的回调条件或 requestAnimationFrame;同时把卸载后的“回调不应再触发”断言统一收进 assertAfterObservationWindow(保留 35ms 观察窗口),并加了一个反向对照测试。改动和描述一致,不涉及生产代码。

结论:整体方向很好,可以合并;下面几点是小建议,不阻塞。

建议

  1. FloatingPanel 测试里还有“立即执行的否定断言”(packages/vue/src/components/floating-panel/test/floating-panel-geometry-props.unit.test.ts)

    • vi.waitFor(() => expect(cb)...({ open: true })) 之后马上 expect(state()).toBe('closed');close-trigger 之后马上 expect(state()).toBe('open');replaced listeners stay fresh 里 expect(first).not.toHaveBeenCalled()。
    • 这些本质上都是“不应发生”的断言。原来的 10ms 延时还能给晚到的错误状态切换一点机会,现在回调一触发就检查,如果受控状态在稍后才被错误改掉,测试会照样通过。这和本 PR 自己在 observation-window.ts 里写的原则相矛盾。
    • 建议这几处也用 assertAfterObservationWindow(...),或至少再 await nextTick() 一次后再断言。
  2. waitFor + “恰好一次”断言:vi.waitFor(() => expect(cb).toHaveBeenCalledExactlyOnceWith(...)) 第一次满足就返回,之后多出来的调用抓不到。对“只调用一次”很关键的地方(例如受控模式下的 onOpenChange),可以在后面补一个 assertAfterObservationWindow(() => expect(cb).toHaveBeenCalledOnce())。

  3. utils/test/observation-window.ts

    • 35ms 是写死的魔法数。建议导出成常量(如 OBSERVATION_WINDOW_MS),或加一个可选参数,方便以后调整,也让反向对照测试里的 20ms 和它的关系更清楚(例如写成 OBSERVATION_WINDOW_MS / 2)。
    • 在注释里注明:这个 helper 依赖真实计时器,在 vi.useFakeTimers() 下会卡住。
  4. NumberInput 的 rAF 等待(packages/vue/src/test/controllable-migration.test.ts):现在依赖“core 在同一个 rAF 回调里完成两步,而且排在测试的 rAF 之前”。注释写得很清楚,赞。如果以后 core 改成嵌套 rAF,这里会悄悄变脆弱;可以考虑用 vi.waitFor(() => assertTextSelection(...)),或者在注释里写明对应的 core 版本/位置。

  5. 测试位置:observation-window.unit.test.ts 测的是根目录 utils/test 下的共享工具,却放在 packages/vue 里。请确认这是有意的(例如只有 Vue 的 unit 项目会跑它);如果 React 也用这个工具,放在共享位置或在两边都能跑会更直观。

做得好的地方

  • 反向对照测试很好地证明了观察窗口是必要的。
  • 把重复的 setTimeout(35) 收成一个带说明的 helper,可读性更好。
  • 验证记录完整(CI、类型检查、浏览器套件)。

@elonehoo

Copy link
Copy Markdown
Member Author

🤖 Review (English)

Summary: Test-only change. It replaces guessed 10/40/35ms sleeps in the FloatingPanel and NumberInput tests with waits on the real callback conditions or a requestAnimationFrame, moves the post-unmount "callback must not fire" checks into a shared assertAfterObservationWindow helper (keeping the 35ms window), and adds a negative-control test. The diff matches the description and touches no production code.

Verdict: Good direction, fine to merge. The points below are minor and non-blocking.

Suggestions

  1. Immediate negative assertions remain in the FloatingPanel tests (packages/vue/src/components/floating-panel/test/floating-panel-geometry-props.unit.test.ts)

    • Right after vi.waitFor(() => expect(cb)...({ open: true })) the test checks expect(state()).toBe('closed'); right after the close-trigger it checks expect(state()).toBe('open'); and replaced listeners stay fresh checks expect(first).not.toHaveBeenCalled().
    • These are all "this must not happen" assertions. The old 10ms delay gave a late, wrong state flip a chance to show up; now the check runs as soon as the callback fires, so a controlled state that gets wrongly changed a bit later would still pass. That contradicts the rule this PR itself writes down in observation-window.ts.
    • Consider wrapping these in assertAfterObservationWindow(...), or at least doing one more await nextTick() before asserting.
  2. waitFor with "exactly once" assertions: vi.waitFor(() => expect(cb).toHaveBeenCalledExactlyOnceWith(...)) returns on the first success, so an extra call that arrives later goes unnoticed. Where "only once" matters (for example onOpenChange in controlled mode), add a follow-up assertAfterObservationWindow(() => expect(cb).toHaveBeenCalledOnce()).

  3. utils/test/observation-window.ts

    • 35ms is a hard-coded magic number. Export it as a constant (for example OBSERVATION_WINDOW_MS) or accept an optional parameter, so it is easy to tune and the 20ms timer in the negative-control test can be expressed relative to it (for example OBSERVATION_WINDOW_MS / 2).
    • Note in the comment that the helper relies on real timers and will hang under vi.useFakeTimers().
  4. NumberInput rAF wait (packages/vue/src/test/controllable-migration.test.ts): this now relies on core doing both steps in a single rAF callback that is queued before the test's rAF. The comment explains it well. If core ever moves to a nested rAF, this becomes quietly fragile; consider vi.waitFor(() => assertTextSelection(...)), or reference the relevant core version or location in the comment.

  5. Test location: observation-window.unit.test.ts tests a shared helper in the root utils/test, but it lives under packages/vue. Please confirm that is intentional (for example, only the Vue unit project picks it up). Since React uses the helper too, a shared location or running it from both sides would be clearer.

What's good

  • The negative-control test clearly proves the observation window is needed.
  • Folding the repeated setTimeout(35) into one documented helper improves readability.
  • The verification notes are thorough (CI, type checks, browser suites).

@elonehoo

Copy link
Copy Markdown
Member Author

🤖 レビュー(日本語)

概要:テストのみの変更です。FloatingPanel と NumberInput のテストで推測に頼っていた 10/40/35ms の待機を、実際のコールバック条件や requestAnimationFrame を待つ形に置き換えています。また、アンマウント後の「コールバックが呼ばれないこと」の検証を共通ヘルパー assertAfterObservationWindow(35ms の観測ウィンドウを維持)にまとめ、ネガティブコントロールのテストを追加しています。差分は説明と一致しており、本番コードには触れていません。

結論:方向性は良く、マージして問題ありません。以下は軽微な提案で、ブロッカーではありません。

提案

  1. FloatingPanel のテストに「即時の否定アサーション」が残っています(packages/vue/src/components/floating-panel/test/floating-panel-geometry-props.unit.test.ts)

    • vi.waitFor(() => expect(cb)...({ open: true })) の直後に expect(state()).toBe('closed')、close-trigger の直後に expect(state()).toBe('open')、replaced listeners stay fresh では expect(first).not.toHaveBeenCalled() を確認しています。
    • これらはどれも「起きてはいけない」ことの検証です。以前の 10ms 待機は遅れて起きる誤った状態変化を捉える余地がありましたが、今はコールバック発火直後にチェックするため、制御状態が少し後に誤って変わっても通ってしまいます。これは本 PR が observation-window.ts に書いた原則と矛盾します。
    • これらも assertAfterObservationWindow(...) で包むか、少なくとももう一度 await nextTick() してからアサートすることを検討してください。
  2. waitFor と「ちょうど 1 回」のアサーション:vi.waitFor(() => expect(cb).toHaveBeenCalledExactlyOnceWith(...)) は最初に成功した時点で返るため、後から来た余分な呼び出しは検出できません。「1 回だけ」が重要な箇所(例:制御モードの onOpenChange)では、続けて assertAfterObservationWindow(() => expect(cb).toHaveBeenCalledOnce()) を追加するとよいです。

  3. utils/test/observation-window.ts

    • 35ms はハードコードされたマジックナンバーです。定数(例:OBSERVATION_WINDOW_MS)としてエクスポートするか、任意の引数を受け取れるようにすると調整しやすく、ネガティブコントロールの 20ms タイマーもそれに対する相対値(例:OBSERVATION_WINDOW_MS / 2)で表せます。
    • このヘルパーは実タイマーに依存し、vi.useFakeTimers() 下ではハングすることをコメントに明記するとよいです。
  4. NumberInput の rAF 待機(packages/vue/src/test/controllable-migration.test.ts):core が 1 つの rAF コールバック内で両方の処理を行い、それがテスト側の rAF より先にキューされていることに依存しています。コメントの説明は分かりやすいです。将来 core がネストした rAF に変わると静かに壊れやすくなるため、vi.waitFor(() => assertTextSelection(...)) を使うか、該当する core のバージョンや場所をコメントに記載することを検討してください。

  5. テストの配置:observation-window.unit.test.ts はルートの utils/test にある共通ヘルパーをテストしていますが、packages/vue 配下に置かれています。意図的か(例:Vue の unit プロジェクトだけが拾う)確認してください。React もこのヘルパーを使うので、共通の場所に置くか両方から実行できるようにすると分かりやすいです。

良い点

  • ネガティブコントロールのテストで、観測ウィンドウが必要であることを明確に示せています。
  • 繰り返されていた setTimeout(35) を説明付きのヘルパー 1 つにまとめ、可読性が上がっています。
  • 検証記録(CI、型チェック、ブラウザスイート)が丁寧です。

@elonehoo
elonehoo marked this pull request as draft October 21, 2026 10:02

Copy link
Copy Markdown
Member Author

Addressed the timing-coverage suggestions in 6721e71:

  • FloatingPanel now separates positive readiness from a subsequent negative observation window for controlled state, callback uniqueness, and stale listeners. Anchor callback uniqueness is also rechecked after that window.
  • The shared duration is named, the delayed-callback control uses half that duration, and the helper documents its real-timer requirement.
  • NumberInput's frame barrier identifies the inspected core version. A plain waitFor on already-correct text/caret would risk accepting the pre-reconciliation DOM, so the scheduler barrier remains.
  • The helper test intentionally lives in the existing vue-unit happy-dom project; this is documented without adding a new test runner or duplicate framework tests.

Independent static re-review found no blocking issue. Kept draft while the new head's full CI reruns; the previous head's green checks are not being treated as validation of this follow-up.

@elonehoo
elonehoo marked this pull request as ready for review October 21, 2026 10:23
@elonehoo
elonehoo merged commit 879f2b4 into main Oct 21, 2026
14 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.

Make short timer waits deterministic while preserving negative assertions

1 participant