Repository navigation
test(api): make notification load event delivery deterministic - #2127
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Controlled handler workload api/src/unraid-api/graph/resolvers/notifications/notifications.load.spec.ts, .limetech/ai-review-markers/fix-notification-load-event-count-057e17c1ef19.json |
The test closes the watcher, records synthetic events, and invokes the notification handler directly. It adds a repeated-change case and updates event and warning expectations. A final-stage AI review marker was added. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Other
Possibly related PRs
- unraid/api#2123: Changes notification count behavior that this load test asserts.
Merge Risk: 
f2ad0
-
The controlled workload checks that repeated changes do not publish additions. No merge-blocking issue is identified.
Pre-merge checks: 
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: making notification load event delivery deterministic in API tests. Docstring Coverage No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Docstring Coverage
-
Explanation
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files. (1 skipped: 1 unsupported.)
-
Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- 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.
A rabbit counts each change in flight
Two hops for changes repeated right
The watcher rests; the handler runs
With recorded events, the work is done
And carrots mark the test tonight
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2127 +/- ##
==========================================
- Coverage 53.97% 53.95% -0.03%
==========================================
Files 1046 1046
Lines 73610 73610
Branches 8833 8831 -2
==========================================
- Hits 39730 39714 -16
- Misses 33752 33768 +16
Partials 128 128 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
🔄 PR Merged - Plugin Redirected to StagingThis PR has been merged and the preview plugin has been updated to redirect to the staging version. For users testing this PR:
Staging URL: Thank you for testing! 🚀 |
Summary
Make the notification load tests deterministic by delivering fixed event batches to the real service handler after fixture writes complete.
Why This Exists
Work intent: remove the filesystem-event cardinality assumption that intermittently blocks API CI while preserving notification correctness and read-cost coverage. Release PR #2122 failed Test API because 100 writes produced 157 change events. The test waited for exactly 100 events and could never complete once the count exceeded that value. Closing the watcher at a transient exact count could also discard later deliveries.
Resolution
Close the real watcher after initialization, then deliver the workload through the existing
handleNotificationEventimplementation. Keep real fixture files, parsing, index updates, subscriptions, warning scans, and all existing read-cost and final-state assertions. Add repeated-change workloads at each history/burst size to verify duplicate changes preserve exact counts and do not publish additions.Reviewer Considerations
This separates the fixed workload used to measure handler costs from OS scheduling. The existing
notifications.overview.spec.tstests continue to cover real watcher delivery, legacy writes, subscriptions, bursts, hydration, and directory changes. The broadcast tests cover events arriving during scans. Production notification behavior and timeout limits are unchanged.Behavior Changes
CI no longer requires one filesystem event per logical file write. No customer-facing or API behavior changes.
Implementation Summary
One test file changes. The load matrix grows from 12 to 15 cases, including duplicate change delivery. No production modules or dependencies change.
Verification
pnpm --filter @unraid/api test src/unraid-api/graph/resolvers/notifications/notifications.load.spec.ts src/unraid-api/graph/resolvers/notifications/notifications.overview.spec.ts src/unraid-api/graph/resolvers/notifications/notifications.broadcast.spec.ts: 45 passed.pnpm --filter @unraid/api run lint: passed.pnpm --filter @unraid/api run type-check: passed.Risk
The load test now measures an explicit event batch rather than OS timing. Real watcher and in-flight broadcast integration coverage remains in the adjacent suites. Full Linux CI is pending.
Summary by CodeRabbit