test(notify): anchor dedupe keys to today — CI is red on every PR because a date aged out - #207
Merged
Merged
Conversation
The 6 failures in notification_dedupe_test are a TIME BOMB, not a regression.
Nothing in the code changed: the calendar did.
The suite builds date-PREFIXED dedupe keys from a hardcoded `2026-07-23`, and
`FiredKeyStore` prunes dated flags older than `retentionDays` (14). While that
date was recent the keys stayed inside the window and the guard deduped
correctly. Once it aged past 14 days, every key was pruned the moment it was
written, so the second and third emit fired again:
same dedupeKey fires the OS notification exactly once
Expected: <1> Actual: <3>
main passed CI on 2026-08-04, when the date was 12 days old. It has been
failing since the window closed, on the same commit, with no code change and
nothing to point at.
Proved it directly before fixing: substituting today's date into the
unmodified file on origin/main turns all 15 tests green.
Fix is to anchor to `todayLabel()` (already imported here) plus a
`_dayLabelOffset` helper for the next-day case, so the keys sit inside the
retention window permanently -- which is the condition the dedupe guard is
actually specified against. No lib/ change; the guard itself was always correct.
Full suite on this branch: 1201 passing, 0 failing.
WHY THIS MATTERS BEYOND THE 6 TESTS: `flutter test --concurrency=1` is exactly
what test.yml runs, so from the moment that window closed EVERY open PR's CI
went red regardless of content. This unblocks them all.
Worth a follow-up: other suites hardcode dates too (ai_briefing_test,
day_nav_test, readiness_freeze_test, core_screens_test,
sleep_profile_policy_test, db_storage_hygiene_test). Most are not
retention-sensitive, but any test whose subject prunes or windows on age has
the same failure mode latent in it.
This was referenced Aug 6, 2026
Merged
Contributor
📝 WalkthroughWalkthroughThe notification dedupe tests now generate date labels and dedupe keys from today and tomorrow. Existing coverage remains for persistence, permissions, categories, quiet hours, concurrency, retention, and high-stress alerts. ChangesNotification dedupe test updates
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
This was referenced Aug 6, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@test/notification_dedupe_test.dart`:
- Around line 80-87: Update the test label initialization around _today,
_tomorrow, and _dayLabelOffset to capture a single DateTime value, derive both
offsets from that timestamp, and reuse the canonical todayLabel formatter from
lib/data/day_label.dart. Remove the manual formatting in _dayLabelOffset while
preserving the expected today and tomorrow labels.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 28b68b73-480d-4557-bccc-5a4655fd164b
📒 Files selected for processing (1)
test/notification_dedupe_test.dart
Comment on lines
+80
to
+87
| final String _tomorrow = _dayLabelOffset(1); | ||
|
|
||
| String _dayLabelOffset(int days) { | ||
| final d = DateTime.now().add(Duration(days: days)); | ||
| return '${d.year.toString().padLeft(4, '0')}-' | ||
| '${d.month.toString().padLeft(2, '0')}-' | ||
| '${d.day.toString().padLeft(2, '0')}'; | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Derive both labels from one timestamp and the canonical formatter.
_dayLabelOffset calls DateTime.now() independently from _today. If initialization crosses local midnight, _tomorrow can skip a calendar day. The manual formatter can also diverge from todayLabel in lib/data/day_label.dart at Line [24].
Capture one DateTime and derive both labels with todayLabel.
Proposed fix
-final String _today = todayLabel();
-final String _tomorrow = _dayLabelOffset(1);
+final _testNow = DateTime.now();
+final String _today = todayLabel(_testNow);
+final String _tomorrow = _dayLabelOffset(_testNow, 1);
-String _dayLabelOffset(int days) {
- final d = DateTime.now().add(Duration(days: days));
- return '${d.year.toString().padLeft(4, '0')}-'
- '${d.month.toString().padLeft(2, '0')}-'
- '${d.day.toString().padLeft(2, '0')}';
-}
+String _dayLabelOffset(DateTime base, int days) =>
+ todayLabel(DateTime(base.year, base.month, base.day + days));🤖 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 `@test/notification_dedupe_test.dart` around lines 80 - 87, Update the test
label initialization around _today, _tomorrow, and _dayLabelOffset to capture a
single DateTime value, derive both offsets from that timestamp, and reuse the
canonical todayLabel formatter from lib/data/day_label.dart. Remove the manual
formatting in _dayLabelOffset while preserving the expected today and tomorrow
labels.
abdulsaheel
added a commit
that referenced
this pull request
Aug 8, 2026
Four of the five findings were real. Each verified against the code before
changing anything.
1. VACUOUS GUARD TEST (Major) -- substrate_accel_absence_test only exercised
`accelPresentFraction`, never the gate it feeds at substrate.dart:574-600.
The coverage floor could have been deleted and every assertion still passed
-- the same vacuity I flagged in other people's tests earlier in this stack,
in my own. Now drives `calendarDays` (the real entry point) and asserts on
`PhysioDay.sleepSource`.
Two things worth recording about building it. The first positive control
FAILED: a "near-still" night was not detected as sleep at all, which would
have made the negative test pass for the wrong reason. Fixed by giving the
fixture a genuine shape -- constant gravity through the night, a 10 deg/s
ramp while awake (a ramp, not an alternation: the mask smooths z-angle with
a 5-second rolling MEDIAN, which erases a 1 Hz square wave entirely).
Second, mutation testing showed the headline all-zero case was STILL not
discriminating -- with the gate removed it passed anyway, because a record
immobile end to end is rejected downstream regardless. The discriminating
shape is the realistic one: evening has gravity, the night's records do not.
Coverage lands below the floor, and those zeros would otherwise form a clean
multi-hour "immobile" block to anchor on. Removing the gate now fails 2
tests; both are pinned, with the non-discriminating one kept and labelled.
2. STALE SERIAL (Minor, real) -- `context.select` selected `a.device`, but
`select` compares with `==`, `DeviceState` declares no `==`/`hashCode`, and
`BleEngine` mutates `state.serial` IN PLACE. The selector returned the same
reference before and after, so no rebuild fired and the row could sit on a
stale serial indefinitely. Now selects the serial STRING the row renders,
which is the only value this sheet reads from `device`/`paired`.
3. CHIP OVERFLOW (Minor) -- two intrinsically-sized, non-flex chips in a Row,
the second carrying "WHOOP 5 (experimental)". At large text scales or on a
narrow device their combined width exceeds the Expanded column. Wrap
degrades to a second line instead.
4. TEST COVERAGE (Minor) -- the R10-lite case asserted only ABSENCE from
`decoded_onehz`, which would equally pass if the record were dropped
outright; retention in `samples` is the other half of that contract. Added,
plus a case protecting the unparseable-hex fallback in
`_decodeOneHzSample`.
NOT CHANGED, deliberately:
* `gen5DeepBuffersEnabled` is unwired in production -- verified true (zero
overrides in app_state or background_sync), but it is a deliberate
default-OFF opt-in for v20/v21/v26 buffers that are archived, not
interpreted. Wiring a settings toggle for a feature nothing consumes would
be premature.
* The protocol PIN. CodeRabbit's `pubspec.yaml` comment finding was real and
is fixed -- the file claimed this branch was "intentionally WHOOP-4-only"
while pinning the multiband commit twelve lines below. But the pin itself
is left alone: protocol#16 has landed, so the SHA is now reachable from
protocol main and SHOULD be repointed there (the current pin is a
PR-branch head on the branch this PR deletes on merge -- the exact
orphaning the same paragraph warns against). That belongs with the
main-merge, not this pass; recorded as a FOLLOW-UP in the file.
12 tests added; the gate tests are mutation-verified. analyze clean.
Suite 1085 passing / 6 failing -- the 6 are the notification_dedupe time bombs
this branch still carries because it predates #207 on main (19 hardcoded dates,
no `todayLabel()`). They clear when main is merged in.
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.
This is currently blocking CI on every open PR. Nothing is wrong with any of them, and nothing is wrong with
lib/— the calendar advanced.What happened
notification_dedupe_testbuilds date-prefixed dedupe keys from a hardcoded2026-07-23.FiredKeyStoreprunes dated flags older thanretentionDays(14). While that date was recent the keys stayed inside the window and the guard deduped correctly. Once it aged past 14 days, every key was pruned the instant it was written, so the second and third emit fired again:mainpassed CI on 2026-08-04, when that date was 12 days old. It has been failing ever since the window closed — on the same commit, with no code change and nothing to point at.Proof before fix
Substituting today's date into the unmodified file on
origin/mainturns all 15 tests green. That isolates the cause completely: the guard was never broken.The fix
Anchor to
todayLabel()(already imported in this file) plus a small_dayLabelOffsethelper for the next-day case, so the keys sit inside the retention window permanently — which is the condition the dedupe guard is actually specified against.No
lib/change. Full suite on this branch: 1201 passing, 0 failing.Why it matters beyond these 6 tests
flutter test --concurrency=1is exactly whattest.ymlruns. From the moment that window closed, every open PR's CI went red regardless of content. Merging this unblocks all of them.Follow-up worth considering
Other suites hardcode dates too —
ai_briefing_test,day_nav_test,readiness_freeze_test,core_screens_test,sleep_profile_policy_test,db_storage_hygiene_test. Most aren't retention-sensitive, but any test whose subject prunes or windows on age carries the same failure mode latent. A lint or a sharedtodayLabel()-based fixture helper would close the class.Also note: fork PRs are currently sitting at
action_required— they need "Approve and run workflows" before any of them can show a green check.Summary by CodeRabbit