test(pyamber): cover the main loop's control, console and end-channel paths - #8042
Merged
Merged
Conversation
Contributor
Automated Reviewer SuggestionsBased on the
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8042 +/- ##
============================================
+ Coverage 93.41% 93.44% +0.02%
Complexity 4696 4696
============================================
Files 1179 1179
Lines 47713 47713
Branches 5314 5314
============================================
+ Hits 44573 44587 +14
+ Misses 1698 1684 -14
Partials 1442 1442
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 419 | 0.256 | 22,480/32,614/32,614 us | 🔴 +22.1% / 🔴 +132.8% |
| ⚪ | bs=100 sw=10 sl=64 | 958 | 0.584 | 100,183/139,559/139,559 us | ⚪ within ±5% / 🔴 +46.8% |
| 🔴 | bs=1000 sw=10 sl=64 | 1,133 | 0.691 | 877,076/969,080/969,080 us | 🔴 +6.9% / 🔴 +5.6% |
Baseline details
Latest main b01b11f from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 419 tuples/sec | 477 tuples/sec | 893.99 tuples/sec | -12.2% | -53.1% |
| bs=10 sw=10 sl=64 | MB/s | 0.256 MB/s | 0.291 MB/s | 0.546 MB/s | -12.0% | -53.1% |
| bs=10 sw=10 sl=64 | p50 | 22,480 us | 18,410 us | 11,176 us | +22.1% | +101.1% |
| bs=10 sw=10 sl=64 | p95 | 32,614 us | 31,634 us | 14,009 us | +3.1% | +132.8% |
| bs=10 sw=10 sl=64 | p99 | 32,614 us | 31,634 us | 16,898 us | +3.1% | +93.0% |
| bs=100 sw=10 sl=64 | throughput | 958 tuples/sec | 973 tuples/sec | 1,157 tuples/sec | -1.5% | -17.2% |
| bs=100 sw=10 sl=64 | MB/s | 0.584 MB/s | 0.594 MB/s | 0.706 MB/s | -1.7% | -17.3% |
| bs=100 sw=10 sl=64 | p50 | 100,183 us | 99,602 us | 88,763 us | +0.6% | +12.9% |
| bs=100 sw=10 sl=64 | p95 | 139,559 us | 145,900 us | 95,096 us | -4.3% | +46.8% |
| bs=100 sw=10 sl=64 | p99 | 139,559 us | 145,900 us | 102,854 us | -4.3% | +35.7% |
| bs=1000 sw=10 sl=64 | throughput | 1,133 tuples/sec | 1,157 tuples/sec | 1,187 tuples/sec | -2.1% | -4.5% |
| bs=1000 sw=10 sl=64 | MB/s | 0.691 MB/s | 0.706 MB/s | 0.724 MB/s | -2.1% | -4.6% |
| bs=1000 sw=10 sl=64 | p50 | 877,076 us | 858,220 us | 874,968 us | +2.2% | +0.2% |
| bs=1000 sw=10 sl=64 | p95 | 969,080 us | 906,790 us | 917,815 us | +6.9% | +5.6% |
| bs=1000 sw=10 sl=64 | p99 | 969,080 us | 906,790 us | 949,868 us | +6.9% | +2.0% |
Raw CSV
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,477.59,200,128000,419,0.256,22480.41,32614.08,32614.08
1,100,10,64,20,2088.47,2000,1280000,958,0.584,100183.05,139559.15,139559.15
2,1000,10,64,20,17654.14,20000,12800000,1133,0.691,877075.71,969079.63,969079.63
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Xinyuan Lin <xinyual3@uci.edu>
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
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.
What changes were proposed in this PR?
test_main_loop.pygoes from 34 tests to 44, covering the console-message RPC path, the end-channel completion rules, the per-element exception backstop, and the ECM/loop-counter forwarding paths.+14 fully-covered lines move on Codecov today; the full +21 only shows up once #8040 lands. Lines 176, 397, 658, 661, 706, 714 and 837 are line-hit with one arm untaken, so today's branch-less report already scores them covered — closing their second arm is real and mutation-proven but invisible until branch coverage is on. I would rather state both figures than quote the larger one and have it not materialise.
The
[tool.coverage.run] branch = truesetting is deliberately not added here; it belongs to #8040 and adding it would conflict.Newly fully covered: 176, 177, 397, 400, 658, 661, 662, 706, 714, 715, 718, 719, 725, 813, 814, 817, 837, 838, 839, 849, 850. Nothing lost. Line 795 remains uncovered and four lines (349, 650, 740, 794) remain branch-partial.
What the reviewers found
Two adversarial reviewers reported eight surviving mutants against the first draft. Every one was reproduced before being fixed — seven gave a clean
43 passedon the delivered spec and one produced no summary at all. Eight of the claimed lines were covered vacuously. The ones worth naming:output_queue.get()instead of failing — a hang, not an assertion failure, which is the worst way for a test to be wrong.EndChannelbroadcast at all.assert switched == [True]claimed a statement order it cannot observe.emitted_counter == 0assertion was a degenerate literal, and areset_calls == []assertion was call-graph-guaranteed to hold.All eight now die. 33 mutations applied one at a time, 32 killed, 1 judged equivalent (a flush/pause order exchange, with the reasoning recorded).
Four of the 32 kills are honest about their conditions: the line-706, line-661, line-837 negations and the 393–394 order swap die only when the run is scoped to a single test, because they make pre-existing thread-driven tests block, and pytest-timeout's Windows
threadmethod then kills the session before the killing test runs. They are stated as scoped kills rather than credited as clean whole-file kills.One further mutant was discarded rather than reported as surviving: negating line 661 makes the pre-existing
test_main_loop_thread_can_process_messagesblock forever, yielding no per-test signal. The same statement is covered by areturn→passmutant that dies exclusively against a new test, so nothing is left unproven.A defect found and deliberately not pinned
_process_data_element's per-element backstop (813–814) isexcept Exception as err: logger.exception(err)— it logs and continues without callingcontext.report_exception. A runtime failure on one element therefore never reaches the coordinator, and the workflow can report SUCCESS on a short result.The new test asserts only that iteration continues and that nothing propagates. It does not assert that nothing is reported, because that half is arguably a silent-wrong-results bug and pinning it would cement it. The test carries a comment saying so; anyone strengthening it should keep that boundary.
Verification
Measured with the full CI-shaped suite from
amber/,--cov-branchpassed explicitly on both sides (this branch is onmainand so lacks #8040's config),LOGURU_LEVEL=WARNING, per-linehitsandcondition-coverageread out ofcoverage.xmlby script rather than from the summary percentage. The before-state was obtained by copying the original spec back from a scratch snapshot — never viagit restore— with the production tree verified pristine throughout.No regression. Full pyamber suite:
5 failed, 1169 passed, 7 errorsagainst a baseline of5 failed, 1159 passed, 7 errors. TheFAILED/ERRORnode-id sets were extracted from both runs, sorted and diffed: identical by identity, 12 entries (7test_iceberg_documenterrors, 2 iceberg failures,test_tuple::test_hash, 2test_expression_evaluatorrepr-formatting failures). +10 passing. The scoped spec passes44 passedon five consecutive runs.ruff checkandruff format --checkpass on CI's exact scope (src/main/python src/test/python).git diff --numstatshows666 0— 666 added, zero deleted.A flake seen once and reported rather than buried: one no-branch after-run came back 282/285 with lines 324–325 at zero hits; three consecutive re-runs all gave 284/285 with both covered. Those two lines are in
_check_and_process_control's while body, reached from a daemon thread, so a scheduling-dependent miss is plausible. The published figure is the reproducible one.On overlap with open PRs
This file is not untouched work. #7624 (open) and #7388 (draft) both modify
main_loop.pyandtest_main_loop.py. I checked both diffs: none of the 21 lines here falls inside either PR's edited hunks, and the new tests are appended at the end ofTestMainLoop, away from the test-file hunks. The collision risk is a routine rebase rather than a semantic conflict — but it is not zero, and the earlier claim that this target was free was wrong.Deliberately not included
Line 795 and the four remaining partials (349, 650, 740, 794) are left. Reaching 813/814 already required monkeypatching a
MainLoopprivate — established practice in this file but white-box, and flagged as the one such test in the bundle.No production file is touched;
git diff -- 'amber/src/main/*'is empty, checked after every mutation revert and as the final action.Any related issues, documentation, discussions?
Closes #8041
How was this PR tested?
amber/junit.xml,amber/coverage.xmlandamber/.coverageare regenerated by every run and are not committed.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 5)