Repository navigation
Conversation
The "reports server metrics scoped past the warm-up" test ran a closed-loop load for 400ms of real time with a 120ms warm-up. Server metrics are only reported once the runner takes its baseline snapshot, which happens lazily on the first send scheduled past the warm-up. If both closed-loop workers were still busy with warm-up deliveries when the deadline passed (a cold target, CI scheduling), no measured send was ever dispatched, the baseline was never taken, and server was null. The success-rate assertion could not catch this, because an empty measured window reports a success rate of 1. The test now uses constant open-loop arrivals at 10/s over 400ms under a fake clock, which schedules exactly four sends: two inside the warm-up and two measured. Open-loop scheduling does not depend on completions, so the measured sends, and with them the baseline, always happen however slow the deliveries are. It also asserts that exactly two measured requests were counted client-side and that all four deliveries reached the inbox listener, so a vacuous pass is no longer possible. The fake clock used by the multi-recipient tests is now a shared createFakeClock() helper. Fixes fedify-dev#1219 Assisted-by: Claude Code:claude-opus-5-5 Assisted-by: Codex:gpt-6-astra
dahlia
added
component/cli
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (1)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe inbox benchmark tests now use a shared fake clock. The warm-up test uses scheduled arrivals and checks measured deliveries, listener deliveries, and server metrics. ChangesInbox benchmark tests
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Other Merge Risk: ⚪ Minimal · up to The supplied evidence identifies no issue that needs to be fixed before merging. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 🚀 New features to boost your workflow:
|
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.
Fixes #1219. Follow-up to #1220.
Why it could flake
The runner takes its baseline snapshot on the first send scheduled after the warm-up, and reports server metrics only if that snapshot exists. With two closed-loop workers, a 120ms warm-up, and a 400ms real-clock window, warm-up deliveries could keep both workers busy until the window closed. No measured send would start, so
servercame back null. The success-rate assertion still passed, because an empty measured window reports 1.How it's fixed
The test now uses constant open-loop arrivals at 10/s over 400ms under a fake clock. Sends at 0 and 100ms fall inside the warm-up; sends at 200 and 300ms are measured. Open-loop arrivals are scheduled up front rather than after earlier sends finish, so the measured sends always happen, at worst a little late, and the baseline with them.
Two new assertions require exactly two requests counted client-side and all four deliveries reaching the inbox listener.
The fake clock from #1220 is now a shared
createFakeClock()helper in inbox.test.ts.Since
ServerMetricsexposes only percentiles, the test can't verify that server metrics exclude warm-up verifications. stats-client.test.ts covers the subtraction.