Fix flaky usePendingConciergeResponse unmount test#87556
Fix flaky usePendingConciergeResponse unmount test#87556
Conversation
Use a 10s delay instead of 80ms in the unmount-cleanup test so the timer never races with real wall-clock time under CI load. The test only needs to prove clearTimeout prevents the action — a large pending timer that gets cleared is just as valid as a short one. Co-authored-by: Vit Horacek <mountiny@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. |
|
@thesahindia Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
@MelvinBot can you sync up with main |
…gConciergeResponseTest Co-authored-by: Vit Horacek <mountiny@users.noreply.github.com>
|
Synced with main — merged the latest changes from |
|
The To resolve: Re-run the failed |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb05c49540
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // And we wait long enough to confirm the action was not applied | ||
| await delay(150); | ||
| await waitForBatchedUpdates(); |
There was a problem hiding this comment.
This test no longer verifies that unmount cleanup actually cancels the timeout. displayAfter is set 10 seconds in the future, but the assertion runs after only 150ms, so the callback cannot execute even if clearTimeout is removed; the test would still pass and miss a real regression in usePendingConciergeResponse cleanup behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — the test was indeed not validating that clearTimeout actually prevents the callback. Fixed in b3f7e0f: the unmount test now uses jest.useFakeTimers() and advances time past the deadline with jest.advanceTimersByTime(), so removing clearTimeout from the hook cleanup would cause the test to fail.
|
The failing checks are not related to this PR's changes:
All code-related checks (ESLint, TypeScript, Prettier, tests 1–7, Storybook, builds) are passing. Test job 8 is still running. Action needed: Re-run the |
|
The failing checks are both unrelated to this PR's changes:
All code-related checks (tests, ESLint, typecheck, prettier, storybook, builds) passed successfully. |
…eout The test previously used a 10s real-timer delay but only waited 150ms after unmount, so it would pass even without clearTimeout. Now uses jest.useFakeTimers() to advance past the deadline, ensuring the test catches regressions in cleanup behavior. Co-authored-by: Vit Horacek <mountiny@users.noreply.github.com>
|
@codex review |
2 participants
Explanation of Change
The
usePendingConciergeResponseunmount test was flaky on CI because it used an 80ms real-timer delay (Date.now() + SHORT_DELAY). Under CI load, thewaitForBatchedUpdatesround-trips consume enough wall-clock time for the delay to expire beforeunmount()runs, causing the timer callback to fire and the assertion to fail.This PR changes the unmount test to use a 10s delay instead. Since this test only needs to prove that
clearTimeoutprevents the action on unmount, a large pending timer that gets cleared is just as valid as a short one — and it eliminates the race window entirely.Fixed Issues
$ #87440
Tests
npx jest tests/unit/hooks/usePendingConciergeResponse.test.tsOffline tests
N/A — test-only change, no runtime behavior affected.
QA Steps
[No QA] — This is a test-only fix for a flaky CI job. No product behavior is changed.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
N/A — test-only change
No UI changes.