Repository navigation
test(frontend): wait for async handlers instead of sleeping in unit tests - #1569
Conversation
…ests Replace the 123 fixed setTimeout sleeps (10/20/50/100ms, plus one 2100ms) that frontend unit tests used to let async handlers finish: - flushPromises() (new src/__tests__/flush-promises.ts) resolves on a zero-delay timer, which fires only after the microtask queue is empty, so a handler chain over mocked promises settles however many awaits it has. Used for the 122 sleeps in auth, auth-mfa-enroll, plans*, history-*, dashboard, savings-history and riexchange tests. - The riexchange "keeps the exchange ID on screen" test slept 2.1s to outlast the old 2s auto-close. It now runs every timer the exchange schedules under jest fake timers, so it also catches an auto-close at any longer delay, in about 50ms. - no-wallclock-sleeps.test.ts fails if a test under src/__tests__ awaits a non-zero setTimeout again (123 hits on main, 0 here). Test-only; no production frontend code changes. The Go sleeps from the issue are on another branch. Refs LeanerCloud#36 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 58 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE Blocking:
Non-blocking:
Attacked:
Body-stale:
Reviewer: rI1. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths. |
|
Local verification at |
|
Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 1 approving independent review(s) on |
Summary
Part of #36 (TEST-04), frontend half only. Frontend unit tests used 123 fixed
await new Promise(r => setTimeout(r, N))sleeps (N = 10/20/50/100ms, plus one 2100ms) to let async handlers finish. Each one wastes N ms per run and can still flake on a loaded CI runner. This PR swaps each sleep for a wait on the work itself. Only files underfrontend/src/__tests__change. No production code changes.Root cause and fix
The handlers under test (login, MFA, profile save, plan/purchase actions, history buttons, dashboard, savings history, RI exchange) await mocked API promises that are already settled. So the handler chain is pure microtasks, and a sleep was only a guess at how long that chain takes.
flushPromises()(newfrontend/src/__tests__/flush-promises.ts) resolves on a zero-delay timer. That timer fires only after the microtask queue is fully empty, including callbacks queued by other callbacks. So the whole chain settles however manyawaits it has, with no wall-clock window. This replaces 122 sleeps inauth,auth-mfa-enroll(including itstick()helper),plans,plans-ramp-label,plans-range-validation,plans-service-ids,history-retry-button,history-approve-button,history-cancel-button,history-approval-queue,dashboard,savings-historyandriexchange. The 264setTimeout(r, 0)flushes already in the suite work the same way.jest.runAllTimersAsync(), which runs every timer the exchange flow schedules. The test drops from about 2.1s to about 50ms and is stronger than before: it also catches an auto-close at any longer delay, which the 2.1s window could not.no-wallclock-sleeps.test.tsscanssrc/__tests__and fails on anynew Promise(r => setTimeout(r, X))(orsetTimeout(() => r(), X)) where X is not literal0. A non-zero constant likeDELAY_MSis flagged too. Its first test pins the matcher's behavior.Choices for negative assertions (
expect(api.x).not.toHaveBeenCalled()and similar): in every converted site the code that could break the assertion runs in the same mocked-promise chain. That chain is fully drained before the zero-delay timer fires, so the flush is at least as strict as the old 10-50ms sleep. None of the converted negative assertions depend on a real timer orrequestAnimationFramein the code under test. I checked everysetTimeout/requestAnimationFramein non-testfrontend/src: they are debounces of 300ms or more, 2-8s UI timers, or rAF layout and scroll work that no converted negative assertion looks at. Mutation probes below confirm this.I chose not to use
waitForfrom@testing-library/dom. It polls on a 50ms interval by default, so it would bring back most of the dead time, and it cannot express "did not happen".Refs #36
Verification
All unit/mock evidence (jest + jsdom, mocked API module), run locally on Linux:
Fail-before / pass-after:
origin/main7b86e38 (git archive) with only the guard added: fails, 123 offenders (auth-mfa-enroll.test.ts:107 sleeps 10ms, ...), matching thegrepcount of the issue's pattern. On this branch: passes, 0 offenders.setTimeout(() => closeModal(modal), 2000)after a successful exchange insrc/riexchange.ts(local only, reverted): the test fails (Expected: false, Received: true). With a 10000ms auto-close it also fails; the old 2.1s sleep would have passed that. It passes without the mutation.git statusclean afterwards):plans.tsrunPlannedPurchaseNow: on cancel, wait 20 microtask hops, then callapi.runPlannedPurchaseanyway. Result: "run action cancelled by user" fails.auth.tslogin pre-flight: after showing the validation error, wait 20 microtask hops, then callapi.login. Result: the 3 "... without calling api.login" tests fail.Timing on a shared, loaded VM, so it is noisy (three
--runInBandruns against one run on main): the riexchange receipt test went from 2146ms to 46-59ms. Summed test durations forauth.test.tswent from 2356ms to 912-1145ms. The other suites are dominated by real rendering work and the noise is bigger than the saved 10-50ms per site.Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree:
queue/platform@c00ef619= origin/main15dc8febplus these heads, merged in this order:Linux, Go 1.26.9,
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers. The full suite ran on644bfcaf, which differs from this tree only by a test-only rename on fix/issue-134-mfa-token (a password-like constant that git-secrets flagged):go build ./...,go vet ./...,go vet -tags integration ./...and the fullgo test -race -short -count=1 ./....--build-tags integration.gocyclo -over 10is silent on the changed non-test files.-tags integration -count=1):internal/api,internal/auth,internal/config,internal/database/postgres/migrations,internal/purchaseandinternal/serverall pass. Migration 000152 applies on top of main's 000151.scripts/setup-git-secrets.shpatterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged.scripts/test-git-secrets-allowlist.sh: 183 PASS, 0 FAIL..tftest.hclfiles were not run on this tree (CI runs them). Nothing was applied to any cloud.Out of scope
internal/scheduler,internal/api,internal/server). They are handled on a separate branch, and test: wait on goroutines and barriers instead of wall-clock sleeps in Go tests #1298 already did part of that work. That is why this PR saysRefs, not a closing keyword.setTimeout(r, 0)flushes and the few localflush()helpers (riexchange,archera-comparison,purchase-modal-*). These are not wall-clock sleeps, so I left them alone. They could be consolidated ontoflushPromises()later.Maintainer: please mirror the issue's triage labels onto this PR:
impact/internal,type/chore,triaged,priority/p3,severity/low,urgency/eventually,effort/m.🤖 Generated with Claude Code