fix(test): make section-sheet tests independent of the wall clock - #34
Open
kubiknyc wants to merge 1 commit into
Open
fix(test): make section-sheet tests independent of the wall clock#34kubiknyc wants to merge 1 commit into
kubiknyc wants to merge 1 commit into
Conversation
The eight *SectionSheet suites each run a single test that performs the suite's first render -- paying the one-time cost of lazily requiring the RN/Expo component stack -- and then waits a real 400ms debounce, both inside one jest budget. Warm, that costs ~2.6s against the 5s default; on a cold transform cache it exceeds even 30s. So the suites failed deterministically after --clearCache, in a fresh worktree, or whenever Windows cleaned %TEMP%\jest. It read as a flake only because cache state varied between runs. Replace waitFor with the fake-timer pattern already used by CrewWorkSheet.test.tsx and useSectionDraft.test.tsx: advance by the exported SECTION_DRAFT_DEBOUNCE_MS, flush the promise chain issueWrite kicks off, then assert. try/finally restores real timers. Also set testTimeout to 30000. That does not fix the sheets on its own, but it fixes RepositoryProvider.rekey.test.tsx cold and gives the remaining real-timer tests headroom over the 5s default the repo never chose. Verified cold (--clearCache) with coverage at 21 workers: 64/64 suites, 790/790 tests. npm run verify exits 0. Caveat: fake timers also fake jest's own timeout timer, so the 30s limit is not enforced inside these eight tests.
📝 WalkthroughWalkthroughThe PR updates eight section-sheet tests to use Jest fake timers for draft debounce handling. The tests flush asynchronous writes and restore real timers. Jest now allows tests to run for up to 30 seconds. ChangesSection draft test timing
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/components/report/DelaysSectionSheet.test.tsx (1)
22-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the manual two-tick microtask flush with
jest.advanceTimersByTimeAsync.All eight converted tests use the same pattern:
jest.advanceTimersByTime(SECTION_DRAFT_DEBOUNCE_MS)followed by exactly twoawait Promise.resolve()calls, with a comment stating this exists because "issueWrite chains through a Promise." This hard-codes an assumption about the promise-chain depth insideuseSectionDraft.ts. If that depth changes in the future (for example, an added.then()for optimistic updates or error handling), all eight tests can start failing for reasons unrelated to the behavior under test, and each failure requires re-deriving the correct tick count. Jest 29 (the version in use here) exposesjest.advanceTimersByTimeAsync(msToRun), which flushes scheduled promise callbacks between timer executions automatically, removing the need to guess the tick count. This does not affect correctness today (the PR states 790/790 tests pass), and the current pattern matches the documented repo precedent inCrewWorkSheet.test.tsxanduseSectionDraft.test.tsx, so this is optional rather than urgent.
src/components/report/DelaysSectionSheet.test.tsx#L22-L38: Replacejest.advanceTimersByTime(SECTION_DRAFT_DEBOUNCE_MS)plus the twoawait Promise.resolve()calls withawait jest.advanceTimersByTimeAsync(SECTION_DRAFT_DEBOUNCE_MS).src/components/report/DeliveriesSectionSheet.test.tsx#L21-L43: Apply the same replacement.src/components/report/EquipmentSectionSheet.test.tsx#L21-L39: Apply the same replacement.src/components/report/InspectionsSectionSheet.test.tsx#L21-L43: Apply the same replacement.src/components/report/RfisSectionSheet.test.tsx#L21-L38: Apply the same replacement.src/components/report/SafetySectionSheet.test.tsx#L25-L42: Apply the same replacement.src/components/report/VisitorsSectionSheet.test.tsx#L21-L38: Apply the same replacement.src/components/report/WeatherSectionSheet.test.tsx#L21-L43: Apply the same replacement.♻️ Proposed pattern change (applies to each site above)
- fireEvent.press(getByLabelText('Add delay')); - jest.advanceTimersByTime(SECTION_DRAFT_DEBOUNCE_MS); - // Flush the microtask chain the debounced callback kicks off (issueWrite - // chains through a Promise) — fake timers don't do this for us. - await Promise.resolve(); - await Promise.resolve(); + fireEvent.press(getByLabelText('Add delay')); + await jest.advanceTimersByTimeAsync(SECTION_DRAFT_DEBOUNCE_MS); expect(mockUpdateSection).toHaveBeenCalledWith('r1', 'delays', expect.anything(), false);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/report/DelaysSectionSheet.test.tsx` around lines 22 - 38, Replace the manual timer advancement and two Promise.resolve() calls in the test flow around DelaysSectionSheet, applying the same change at src/components/report/DelaysSectionSheet.test.tsx:22-38, src/components/report/DeliveriesSectionSheet.test.tsx:21-43, src/components/report/EquipmentSectionSheet.test.tsx:21-39, src/components/report/InspectionsSectionSheet.test.tsx:21-43, src/components/report/RfisSectionSheet.test.tsx:21-38, src/components/report/SafetySectionSheet.test.tsx:25-42, src/components/report/VisitorsSectionSheet.test.tsx:21-38, and src/components/report/WeatherSectionSheet.test.tsx:21-43. Use await jest.advanceTimersByTimeAsync(SECTION_DRAFT_DEBOUNCE_MS) and remove the obsolete microtask-flush comment and awaits.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/components/report/DelaysSectionSheet.test.tsx`:
- Around line 22-38: Replace the manual timer advancement and two
Promise.resolve() calls in the test flow around DelaysSectionSheet, applying the
same change at src/components/report/DelaysSectionSheet.test.tsx:22-38,
src/components/report/DeliveriesSectionSheet.test.tsx:21-43,
src/components/report/EquipmentSectionSheet.test.tsx:21-39,
src/components/report/InspectionsSectionSheet.test.tsx:21-43,
src/components/report/RfisSectionSheet.test.tsx:21-38,
src/components/report/SafetySectionSheet.test.tsx:25-42,
src/components/report/VisitorsSectionSheet.test.tsx:21-38, and
src/components/report/WeatherSectionSheet.test.tsx:21-43. Use await
jest.advanceTimersByTimeAsync(SECTION_DRAFT_DEBOUNCE_MS) and remove the obsolete
microtask-flush comment and awaits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f5406e8d-2836-4c49-a023-a7db6372f001
📒 Files selected for processing (9)
package.jsonsrc/components/report/DelaysSectionSheet.test.tsxsrc/components/report/DeliveriesSectionSheet.test.tsxsrc/components/report/EquipmentSectionSheet.test.tsxsrc/components/report/InspectionsSectionSheet.test.tsxsrc/components/report/RfisSectionSheet.test.tsxsrc/components/report/SafetySectionSheet.test.tsxsrc/components/report/VisitorsSectionSheet.test.tsxsrc/components/report/WeatherSectionSheet.test.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The eight *SectionSheet suites each run a single test that performs the
suite's first render -- paying the one-time cost of lazily requiring the
RN/Expo component stack -- and then waits a real 400ms debounce, both
inside one jest budget. Warm, that costs ~2.6s against the 5s default;
on a cold transform cache it exceeds even 30s. So the suites failed
deterministically after --clearCache, in a fresh worktree, or whenever
Windows cleaned %TEMP%\jest. It read as a flake only because cache state
varied between runs.
Replace waitFor with the fake-timer pattern already used by
CrewWorkSheet.test.tsx and useSectionDraft.test.tsx: advance by the
exported SECTION_DRAFT_DEBOUNCE_MS, flush the promise chain issueWrite
kicks off, then assert. try/finally restores real timers.
Also set testTimeout to 30000. That does not fix the sheets on its own,
but it fixes RepositoryProvider.rekey.test.tsx cold and gives the
remaining real-timer tests headroom over the 5s default the repo never
chose.
Verified cold (--clearCache) with coverage at 21 workers: 64/64 suites,
790/790 tests. npm run verify exits 0.
Caveat: fake timers also fake jest's own timeout timer, so the 30s limit
is not enforced inside these eight tests.
Summary by CodeRabbit
Tests
Chores