Skip to content

test(frontend): wait for async handlers instead of sleeping in unit tests - #1569

Merged
IAmJSD merged 2 commits into
LeanerCloud:mainfrom
IAmJSD:fix/issue-36-fe-waits
Oct 28, 2026
Merged

IAmJSD merged 2 commits into
LeanerCloud:mainfrom
IAmJSD:fix/issue-36-fe-waits

Conversation

@IAmJSD

@IAmJSD IAmJSD commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator

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 under frontend/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() (new frontend/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 many awaits it has, with no wall-clock window. This replaces 122 sleeps in auth, auth-mfa-enroll (including its tick() 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-history and riexchange. The 264 setTimeout(r, 0) flushes already in the suite work the same way.
  • riexchange "keeps the exchange ID on screen until the user closes the modal" slept 2.1s to outlast the old 2s auto-close (fix(frontend): announce RI exchange results and keep the exchange ID #1375). It now runs under jest fake timers and calls 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.
  • Guard: no-wallclock-sleeps.test.ts scans src/__tests__ and fails on any new Promise(r => setTimeout(r, X)) (or setTimeout(() => r(), X)) where X is not literal 0. A non-zero constant like DELAY_MS is 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 or requestAnimationFrame in the code under test. I checked every setTimeout/requestAnimationFrame in non-test frontend/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 waitFor from @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:

cd frontend && npm ci --ignore-scripts
npx jest <each touched file>                         # 13 suites, 752 passed / 1 skipped (same as main)
npx jest                                              # 175 suites, 4595 passed / 1 skipped
npx jest --runInBand                                  # 175 suites, 4595 passed / 1 skipped (no ordering issues)
npx jest --runInBand <touched files + guard> (x3)     # 754 passed, 0 failed each run
npx tsc --noEmit -p .                                 # clean
npx eslint <touched files>                            # clean

Fail-before / pass-after:

  • Guard test on a copy of origin/main 7b86e38 (git archive) with only the guard added: fails, 123 offenders (auth-mfa-enroll.test.ts:107 sleeps 10ms, ...), matching the grep count of the issue's pattern. On this branch: passes, 0 offenders.
  • riexchange receipt test, mutation probe: I put back setTimeout(() => closeModal(modal), 2000) after a successful exchange in src/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.
  • Negative assertions still catch late violations (local mutations, reverted, git status clean afterwards):
    • plans.ts runPlannedPurchaseNow: on cancel, wait 20 microtask hops, then call api.runPlannedPurchase anyway. Result: "run action cancelled by user" fails.
    • auth.ts login pre-flight: after showing the validation error, wait 20 microtask hops, then call api.login. Result: the 3 "... without calling api.login" tests fail.

Timing on a shared, loaded VM, so it is noisy (three --runInBand runs against one run on main): the riexchange receipt test went from 2146ms to 46-59ms. Summed test durations for auth.test.ts went 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/main 15dc8feb plus these heads, merged in this order:

Branch Tested head
fix/issue-712-fargate 2df9723
fix/issue-41-ri-exchange-wiring bb548fe
fix/issue-349-ci-hygiene 360ae6c
fix/issue-756-next-collection 2cb1b2f
fix/issue-36-go-clock aaea757
fix/issue-36-fe-waits 7feb01d
fix/issue-35-purchase-asserts fbafeb0
fix/issue-134-mfa-token 33da813

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 on 644bfcaf, 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):

  • Passed: go build ./..., go vet ./..., go vet -tags integration ./... and the full go test -race -short -count=1 ./....
  • Static checks: golangci-lint v2.10.1 found 0 issues on every changed package, with and without --build-tags integration. gocyclo -over 10 is silent on the changed non-test files.
  • Integration tests, every changed package that has them, one package at a time (-tags integration -count=1): internal/api, internal/auth, internal/config, internal/database/postgres/migrations, internal/purchase and internal/server all pass. Migration 000152 applies on top of main's 000151.
  • Frontend: the full jest suite passes on this tree (175 suites, 4595 tests, 1 skipped).
  • Secrets: git-secrets, using the repo's scripts/setup-git-secrets.sh patterns 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.
  • Terraform .tftest.hcl files were not run on this tree (CI runs them). Nothing was applied to any cloud.

Out of scope

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

IAmJSD and others added 2 commits October 27, 2026 23:53
…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>
@IAmJSD IAmJSD added impact/internal Team-internal only type/chore Maintenance / non-user-visible triaged Item has been triaged priority/p3 Polish / idea / may never ship severity/low Minor harm urgency/eventually No deadline effort/m Days labels Oct 28, 2026
@coderabbitai

coderabbitai Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You'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.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration
Commits

Reviewing files that changed from the base of the PR and between 15dc8fe and 7feb01d.

Files selected for processing: 15
  • frontend/src/__tests__/auth-mfa-enroll.test.ts
  • frontend/src/__tests__/auth.test.ts
  • frontend/src/__tests__/dashboard.test.ts
  • frontend/src/__tests__/flush-promises.ts
  • frontend/src/__tests__/history-approval-queue.test.ts
  • frontend/src/__tests__/history-approve-button.test.ts
  • frontend/src/__tests__/history-cancel-button.test.ts
  • frontend/src/__tests__/history-retry-button.test.ts
  • frontend/src/__tests__/no-wallclock-sleeps.test.ts
  • frontend/src/__tests__/plans-ramp-label.test.ts
  • frontend/src/__tests__/plans-range-validation.test.ts
  • frontend/src/__tests__/plans-service-ids.test.ts
  • frontend/src/__tests__/plans.test.ts
  • frontend/src/__tests__/riexchange.test.ts
  • frontend/src/__tests__/savings-history.test.ts
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@IAmJSD

IAmJSD commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator Author

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 7feb01d7529969c56519233c8875716aaa91e560

Blocking:

  • none

Non-blocking:

  • flushPromises() only drains the microtask queue, plus zero-delay timers that were already queued before it. That is weaker than the old 10-50ms sleep in one way: a short real timer in the code under test (under 50ms) used to fire inside the sleep window and now won't. For positive assertions that would show up as a test failure, so it's safe. For negative assertions it could hide a late violation. I checked every setTimeout/requestAnimationFrame in non-test frontend/src. The only sub-50ms timers are archera.ts:183 and recommendations.ts:2924 (both 0), and neither file's suites were converted. The rest are 200ms or more. The PR's claim holds today. A one-line note in flush-promises.ts that negative assertions on timer-driven code need fake timers would help future users.
  • The guard (no-wallclock-sleeps.test.ts) only catches the new Promise(r => setTimeout(r, N)) shape. A sleep spelled await sleep(50) through a local helper, or jest.advanceTimersByTime misuse, gets past it. That's fine for a regression guard, so I'm only noting it.

Attacked:

  • Confirmed the change is test-only: 15 files, all under frontend/src/__tests__. 123 sleeps were removed and 125 flushPromises() calls added; the two extra replace the deleted tick() helper's call sites. flush-promises.ts is not picked up as a suite (testMatch is **/__tests__/**/*.test.ts).
  • Ran npm ci --ignore-scripts in a scratch worktree, then npx jest on the 14 touched suites (754 passed, 1 skipped), npx tsc --noEmit -p . (clean) and npx eslint on the changed files (clean).
  • Fail-before:
    • Guard: restored main's versions of the 13 converted suites and kept the guard. It fails with exactly 123 offenders (auth-mfa-enroll.test.ts:107 sleeps 10ms, ...).
    • riexchange receipt test: added a modal auto-close setTimeout after the success at src/riexchange.ts:1654. The test fails (Expected: false, Received: true) at 2000ms and also at 10000ms, which the old 2.1s window could not catch.
    • All mutations reverted; git status clean.
  • The riexchange test restores real timers in finally, and runAllTimersAsync terminates: no setInterval on that path, and the test passes.

Body-stale:

  • none

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.

@IAmJSD

IAmJSD commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator Author

Local verification at 7feb01d7529969c56519233c8875716aaa91e560 merged onto main (15dc8feb), together with #1565, #1566, #1567, #1569 in that order (combined tree 189311ee, Linux, Go 1.26.10): go build ./..., go vet with and without -tags integration, the full go test -race -short -count=1 ./..., and go test -race -tags integration -count=1 on every changed package, against Docker Postgres. All pass. CI Success and pre-commit are green on this head, with 1 approving independent review(s) on it. Nothing was run against any cloud.

@IAmJSD
IAmJSD marked this pull request as ready for review October 28, 2026 04:47
@IAmJSD
IAmJSD requested a review from cristim as a code owner October 28, 2026 04:47
@IAmJSD
IAmJSD merged commit 7618082 into LeanerCloud:main Oct 28, 2026
42 checks passed
@IAmJSD

IAmJSD commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator Author

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 7feb01d7, green CI, and the combined-tree verification above. Squash-merged at exactly 7feb01d7529969c56519233c8875716aaa91e560.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/m Days impact/internal Team-internal only priority/p3 Polish / idea / may never ship severity/low Minor harm triaged Item has been triaged type/chore Maintenance / non-user-visible urgency/eventually No deadline

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant