test(pyamber): close the last storage, manager and handler gaps - #8337
Merged
aglinxinyuan merged 1 commit intoSep 2, 2026
Merged
Conversation
Contributor
Automated Reviewer SuggestionsBased on the
|
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The changes are isolated to tests, align with the stated coverage goals, and the new assertions correctly match the exercised production behaviors.
Pull request overview
This PR extends the Amber/pyamber Python unit test suite to cover previously unexecuted lines and untaken branch arms in storage, manager, and handler utilities—closing the remaining coverage gaps without modifying production code.
Changes:
- Added targeted tests that exercise previously uncovered “no-op” / idempotent branches (e.g., base handlers, base receive, repeated close).
- Strengthened
DocumentFactorytests to pin storage-key sanitization behavior and verify correct namespace/key propagation to the Iceberg layer. - Introduced a new test module for
ActorCommandHandlerbase behavior.
File summaries
| File | Description |
|---|---|
| amber/src/test/python/pytexera/udf/examples/test_echo_operator.py | Adds an explicit test covering EchoOperator.on_finish’s override behavior (single None emission). |
| amber/src/test/python/pytexera/storage/test_large_binary_input_stream.py | Adds an idempotency test ensuring LargeBinaryInputStream.close() does not double-close the underlying stream. |
| amber/src/test/python/core/util/stoppable/test_stoppable_queue_blocking_thread.py | Adds coverage for the base receive() no-op behavior and verifies queue draining still works via run(). |
| amber/src/test/python/core/storage/test_document_factory.py | Pins sanitize_uri_path() behavior and expands routing assertions to include storage key + overwrite semantics + distinct NotImplementedError sites. |
| amber/src/test/python/core/architecture/managers/test_state_manager.py | Adds a test ensuring get_current_state() tracks the latest transition (not the version counter). |
| amber/src/test/python/core/architecture/handlers/actorcommand/test_actor_handler_base.py | New test module validating ActorCommandHandler base __call__ is an inert no-op and cmd stays unset. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8337 +/- ##
============================================
+ Coverage 94.05% 94.07% +0.01%
Complexity 4812 4812
============================================
Files 1196 1196
Lines 48749 48749
Branches 5884 5884
============================================
+ Hits 45852 45860 +8
+ Misses 1437 1431 -6
+ Partials 1460 1458 -2
*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 | 395 | 0.241 | 25,062/36,325/36,325 us | 🟢 -9.0% / 🔴 +128.4% |
| 🔴 | bs=100 sw=10 sl=64 | 803 | 0.49 | 126,804/150,812/150,812 us | 🔴 +12.0% / 🔴 +36.3% |
| 🟢 | bs=1000 sw=10 sl=64 | 968 | 0.591 | 1,039,824/1,118,371/1,118,371 us | 🟢 -9.4% / 🔴 +6.1% |
Baseline details
Latest main fe76eec from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 395 tuples/sec | 374 tuples/sec | 750.65 tuples/sec | +5.6% | -47.4% |
| bs=10 sw=10 sl=64 | MB/s | 0.241 MB/s | 0.228 MB/s | 0.458 MB/s | +5.7% | -47.4% |
| bs=10 sw=10 sl=64 | p50 | 25,062 us | 27,528 us | 13,049 us | -9.0% | +92.1% |
| bs=10 sw=10 sl=64 | p95 | 36,325 us | 34,374 us | 15,902 us | +5.7% | +128.4% |
| bs=10 sw=10 sl=64 | p99 | 36,325 us | 34,374 us | 19,184 us | +5.7% | +89.4% |
| bs=100 sw=10 sl=64 | throughput | 803 tuples/sec | 857 tuples/sec | 960.3 tuples/sec | -6.3% | -16.4% |
| bs=100 sw=10 sl=64 | MB/s | 0.49 MB/s | 0.523 MB/s | 0.586 MB/s | -6.3% | -16.4% |
| bs=100 sw=10 sl=64 | p50 | 126,804 us | 113,249 us | 104,199 us | +12.0% | +21.7% |
| bs=100 sw=10 sl=64 | p95 | 150,812 us | 168,208 us | 110,645 us | -10.3% | +36.3% |
| bs=100 sw=10 sl=64 | p99 | 150,812 us | 168,208 us | 120,888 us | -10.3% | +24.8% |
| bs=1000 sw=10 sl=64 | throughput | 968 tuples/sec | 990 tuples/sec | 993.27 tuples/sec | -2.2% | -2.5% |
| bs=1000 sw=10 sl=64 | MB/s | 0.591 MB/s | 0.604 MB/s | 0.606 MB/s | -2.2% | -2.5% |
| bs=1000 sw=10 sl=64 | p50 | 1,039,824 us | 1,007,196 us | 1,012,191 us | +3.2% | +2.7% |
| bs=1000 sw=10 sl=64 | p95 | 1,118,371 us | 1,233,904 us | 1,054,057 us | -9.4% | +6.1% |
| bs=1000 sw=10 sl=64 | p99 | 1,118,371 us | 1,233,904 us | 1,081,313 us | -9.4% | +3.4% |
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,505.99,200,128000,395,0.241,25062.24,36325.13,36325.13
1,100,10,64,20,2490.05,2000,1280000,803,0.490,126804.13,150811.93,150811.93
2,1000,10,64,20,20658.79,20000,12800000,968,0.591,1039823.56,1118370.78,1118370.78
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The changes are confined to tests, and the added assertions align with the current production implementations without introducing brittle or incorrect expectations.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
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?
Five existing pyamber test modules extended and one added. +8 fully-covered lines — all six files finish at 100% statement and 100% branch coverage.
core/storage/document_factory.pypytexera/storage/large_binary_input_stream.pycore/architecture/managers/state_manager.pycore/util/stoppable/stoppable_queue_blocking_thread.pypytexera/udf/examples/echo_operator.pycore/architecture/handlers/actorcommand/actor_handler_base.pyBundle total 196/204 → 204/204. Six of the eight gained lines were never executed; the other two are lines whose second branch arm was never taken.
test_actor_handler_base.pyis new — that module had no test at all.No file here clears a meaningful bar alone; the bundle total is the point, and it is stated that way rather than dressed up per file.
Verification
23 mutants, zero survivors. Each applied one at a time against the repaired tests, and each verified at full non-integration-suite scope by comparing the exact set of failing test ids against the baseline run rather than just the count. 19 of the 23 are killed uniquely by their credited test, proven by re-running the same mutant with only that test deselected and confirming the suite returns to the byte-identical baseline.
Measured by running the exact CI command from
amber/twice at identical whole-suite scope — no-kor-zfilter either time, so no filter-attribution risk — once with the tracked specs restored fromHEADand the new test directory moved out of the tree, once with the bundle in place. Bothcoverage.xmlfiles were parsed programmatically.No
logger.info/debugbody is claimed anywhere: every newly covered line is a plain statement or branch arm that executes identically under CI'sWARNlog level.Eight reviewer findings, all repaired — including one standalone test that was deleted because measurement showed it contributed zero:
actor_handler_base.py's baseline missed only line 30, so line 25 was already covered and the test pinning it added nothing.Full pyamber suite:
5 failed, 1295 passed, 7 errors, with theFAILED/ERRORset identical by identity to main's known 12 entries — no regression.ruff checkandruff format --checkpass on CI's exact scope. The new test file carries the Apache licence header.No production file is touched.
Any related issues, documentation, discussions?
Closes #8335
How was this PR tested?
The 5 failures and 7 errors are the pre-existing Iceberg/Windows set, identical to
main; on CI, where the Iceberg catalog is available, they do not occur.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 5)