You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Wait for the outbox to drop the last batch before a test checks it is empty - #546
A test of the cloud save queue failed once on main (run, attempt 1) and passed on rerun. This makes the test wait for what it then checks, so main stops failing at random.
What was wrong
reconnectAndSend in test/offline_page_changes_test.dart sent until nothing was queued, then returned. A change the server has accepted stays listed until the outbox has removed it from disk, and on a slow machine that removal outlasted the helper's fixed number of turns. The test then found one change (move-3-first) still listed and failed at expect(queue().pending, isEmpty).
No change was stuck. In 92 early returns observed under load, each had one in-flight entry, nothing queued and all three server batches done, and each cleared by itself with no further send.
The change
After the send loop, the helper waits up to five seconds for in-flight changes to leave the outbox. It does not throw when they stay, so a test that expects held work still reaches its own assertions. No app code changes.
Seen running
On Linux, four copies of the file at concurrency 4, pinned to one CPU beside a busy loop:
Result
Before, the one test, 252 runs
78 failed, all at this assertion
After, the whole file, 252 runs (10,332 tests)
0 failed
Not checked: Windows under the same load.
ci: test
Summary by CodeRabbit
Tests
Added coverage for waiting for in-flight page changes to clear from the queue during reconnection.
Confidence Score: 5/5
This test-only change appears safe to merge.
Summary
reconnectAndSend now waits for in-flight work after each batch, before checking whether more work is queued.
Offline page tests wait for in-flight changes to leave the outbox.
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 3b012939-11e8-4d26-b0ad-90f3919f7b0d
Commits
Reviewing files that changed from the base of the PR and between 1cf2db7 and 368b63c.
Files selected for processing: 1
test/offline_page_changes_test.dart
Walkthrough
The reconnect test helper now polls for in-flight page changes after its flush-and-settle loop. It stops when the queue clears or five seconds elapse, with one-millisecond waits between checks.
Changes: Offline page changes
Layer / File(s)
Summary
Wait for in-flight changes test/offline_page_changes_test.dart
After the flush loop, reconnectAndSend polls until in-flight changes leave the queue or five seconds elapse. Each wait is one millisecond.
SunkenInTime/icarus#526: Adds race tests that hold durable outbox removal while an answer is stored. Both PRs address the interval between server acceptance and durable outbox removal.
Merge Risk: Minimal · previous review 1cf2d
This change makes an offline page-change test helper wait longer for in-flight changes to clear, which should reduce intermittent test failures. It has no production impact. The remaining concern about multi-batch tests is unconfirmed, so merge-readiness risk is minimal.
Pre-merge checks: 5
Check name
Status
Explanation
Description Check
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
The title clearly describes the main change: waiting for the final outbox batch to be removed before asserting that the outbox is empty.
Docstring Coverage
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
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 eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 0 files. (1 skipped: 1 unsupported.)
Finishing Touches
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.
The reason will be displayed to describe this comment to others. Learn more.
Additional comments: 1
test/offline_page_changes_test.dart-60-60: 1
60-60: Major · Stability & Availability · Heavy lift
Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.
Wait across batch transitions.
When a batch completes with more work queued, inFlightByEntityKey can become empty before the next batch is claimed. The helper can then return before that work reaches the server, so multi-batch tests can still fail under load. The queue schedules the next flush without awaiting it. (raw.githubusercontent.com)
Continue draining runnable work within the deadline, while allowing held work to reach its assertions.
Review info
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 083d458d-b3c2-47da-afab-cf8bcbce886c
Commits
Reviewing files that changed from the base of the PR and between 141c9e5 and 1cf2db7.
Files selected for processing: 1
test/offline_page_changes_test.dart
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
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
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.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
A test of the cloud save queue failed once on main (run, attempt 1) and passed on rerun. This makes the test wait for what it then checks, so main stops failing at random.
What was wrong
reconnectAndSendintest/offline_page_changes_test.dartsent until nothing was queued, then returned. A change the server has accepted stays listed until the outbox has removed it from disk, and on a slow machine that removal outlasted the helper's fixed number of turns. The test then found one change (move-3-first) still listed and failed atexpect(queue().pending, isEmpty).No change was stuck. In 92 early returns observed under load, each had one in-flight entry, nothing queued and all three server batches done, and each cleared by itself with no further send.
The change
After the send loop, the helper waits up to five seconds for in-flight changes to leave the outbox. It does not throw when they stay, so a test that expects held work still reaches its own assertions. No app code changes.
Seen running
On Linux, four copies of the file at concurrency 4, pinned to one CPU beside a busy loop:
Not checked: Windows under the same load.
ci: test
Summary by CodeRabbit
This test-only change appears safe to merge.Summary
reconnectAndSendnow waits for in-flight work after each batch, before checking whether more work is queued.Reviews (2) · Last reviewed commit: "Look again for queued work after each ba..." · Reviewed by Greptile