Skip to content

test(pyamber): add unit tests for TimedBuffer - #6637

Merged
aglinxinyuan merged 2 commits into
apache:mainfrom
Ma77Ball:test/timed-buffer-py-spec
Jul 20, 2026
Merged

test(pyamber): add unit tests for TimedBuffer#6637
aglinxinyuan merged 2 commits into
apache:mainfrom
Ma77Ball:test/timed-buffer-py-spec

Conversation

@Ma77Ball

@Ma77Ball Ma77Ball commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

  • Add amber/src/test/python/core/util/buffer/test_timed_buffer.py, a new pytest spec for TimedBuffer, which previously had no dedicated unit tests.
  • Cover put buffering and ordering, and flush-on-demand (emit and clear, empty-buffer, and generator laziness).
  • Cover flush-on-size at, below, and above max_message_num.
  • Cover flush-on-time using a monkeypatched module clock (elapsed-interval flush, timer reset after flush, sub-threshold no-flush) with no real sleeps.

Any related issues, documentation, discussions?

Closes: #6636

How was this PR tested?

  • Run: cd amber && python -m pytest src/test/python/core/util/buffer/test_timed_buffer.py, expect all 13 tests passing.
  • Test-only change; no production code is modified.

Was this PR authored or co-authored using generative AI tooling?

Co-authored with Claude Opus 4.8 in compliance with ASF

@github-actions

Copy link
Copy Markdown
Contributor

Automated Reviewer Suggestions

Based on the git blame history of the changed files, we recommend the following reviewers:

  • No candidates found from git blame history.

@github-actions

github-actions Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

⚠️ Benchmark changes need a look

🟢 2 better · 🔴 3 worse · ⚪ 10 noise (<±5%) · 0 without baseline

Compared against main f14cd39 benchmarked on this same runner, so the delta is largely free of cross-runner hardware noise. The "7d avg" column still reflects the gh-pages dashboard. Treat <±5% as noise unless repeated.

Dashboard · Run

config throughput MB/s latency max Δ latest / 7d
🔴 bs=10 sw=10 sl=64 411 0.251 24,713/30,653/30,653 us 🔴 +30.9% / 🔴 +96.1%
bs=100 sw=10 sl=64 930 0.568 105,798/129,048/129,048 us ⚪ within ±5% / 🔴 +21.7%
🟢 bs=1000 sw=10 sl=64 1,076 0.657 932,901/982,236/982,236 us 🟢 -16.1% / 🟢 -6.3%
Baseline details

Latest main f14cd39 from same runner

config metric PR latest main 7d avg Δ latest Δ 7d
bs=10 sw=10 sl=64 throughput 411 tuples/sec 467 tuples/sec 767.23 tuples/sec -12.0% -46.4%
bs=10 sw=10 sl=64 MB/s 0.251 MB/s 0.285 MB/s 0.468 MB/s -11.9% -46.4%
bs=10 sw=10 sl=64 p50 24,713 us 18,882 us 12,705 us +30.9% +94.5%
bs=10 sw=10 sl=64 p95 30,653 us 31,420 us 15,633 us -2.4% +96.1%
bs=10 sw=10 sl=64 p99 30,653 us 31,420 us 20,214 us -2.4% +51.6%
bs=100 sw=10 sl=64 throughput 930 tuples/sec 962 tuples/sec 1,001 tuples/sec -3.3% -7.1%
bs=100 sw=10 sl=64 MB/s 0.568 MB/s 0.587 MB/s 0.611 MB/s -3.2% -7.1%
bs=100 sw=10 sl=64 p50 105,798 us 101,950 us 99,949 us +3.8% +5.9%
bs=100 sw=10 sl=64 p95 129,048 us 129,222 us 105,996 us -0.1% +21.7%
bs=100 sw=10 sl=64 p99 129,048 us 129,222 us 117,999 us -0.1% +9.4%
bs=1000 sw=10 sl=64 throughput 1,076 tuples/sec 1,062 tuples/sec 1,033 tuples/sec +1.3% +4.1%
bs=1000 sw=10 sl=64 MB/s 0.657 MB/s 0.648 MB/s 0.631 MB/s +1.4% +4.2%
bs=1000 sw=10 sl=64 p50 932,901 us 915,593 us 974,571 us +1.9% -4.3%
bs=1000 sw=10 sl=64 p95 982,236 us 1,171,368 us 1,016,467 us -16.1% -3.4%
bs=1000 sw=10 sl=64 p99 982,236 us 1,171,368 us 1,048,798 us -16.1% -6.3%
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,486.46,200,128000,411,0.251,24713.09,30653.24,30653.24
1,100,10,64,20,2149.62,2000,1280000,930,0.568,105798.34,129048.05,129048.05
2,1000,10,64,20,18588.86,20000,12800000,1076,0.657,932901.05,982236.34,982236.34

@codecov-commenter

codecov-commenter commented Jul 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.51%. Comparing base (f14cd39) to head (59504e9).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #6637   +/-   ##
=========================================
  Coverage     74.51%   74.51%           
  Complexity     3440     3440           
=========================================
  Files          1157     1157           
  Lines         45621    45621           
  Branches       5031     5031           
=========================================
  Hits          33996    33996           
  Misses         9973     9973           
  Partials       1652     1652           
Flag Coverage Δ *Carryforward flag
access-control-service 70.00% <ø> (ø) Carriedforward from 226337d
agent-service 76.76% <ø> (ø) Carriedforward from 226337d
amber 66.82% <ø> (ø) Carriedforward from 226337d
computing-unit-managing-service 18.00% <ø> (ø) Carriedforward from 226337d
config-service 66.66% <ø> (ø) Carriedforward from 226337d
file-service 66.80% <ø> (ø) Carriedforward from 226337d
frontend 78.29% <ø> (ø) Carriedforward from 226337d
notebook-migration-service 78.94% <ø> (ø) Carriedforward from 226337d
pyamber 91.83% <ø> (ø)
workflow-compiling-service 55.14% <ø> (ø) Carriedforward from 226337d

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@aglinxinyuan
aglinxinyuan requested a review from Copilot July 20, 2026 21:46

@aglinxinyuan aglinxinyuan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds dedicated pytest unit coverage for the Python worker TimedBuffer utility in Amber, aligning with issue #6636’s goal of deterministic, mocked-time tests for size- and time-based flushing.

Changes:

  • Add a new test_timed_buffer.py spec covering buffering semantics, flush-on-demand, flush-on-size, and flush-on-time with a monkeypatched clock.
  • Validate ordering guarantees and generator laziness (side effects occur on iteration, not generator creation).
  • Assert default configuration values for TimedBuffer.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread amber/src/test/python/core/util/buffer/test_timed_buffer.py Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Xinyuan Lin <xinyual3@uci.edu>
@aglinxinyuan
aglinxinyuan enabled auto-merge July 20, 2026 21:51
@aglinxinyuan
aglinxinyuan added this pull request to the merge queue Jul 20, 2026
Merged via the queue into apache:main with commit 7deaec2 Jul 20, 2026
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add unit test coverage for timed_buffer

4 participants