Skip to content

test(pyamber): cover the python worker entry point - #8050

Open
aglinxinyuan wants to merge 1 commit into
apache:mainfrom
aglinxinyuan:cov/pyamber-worker-entry
Open

test(pyamber): cover the python worker entry point#8050
aglinxinyuan wants to merge 1 commit into
apache:mainfrom
aglinxinyuan:cov/pyamber-worker-entry

Conversation

@aglinxinyuan

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

test_run_python_worker.py goes from 27 tests to 42, covering the worker entry point's configuration-failure paths.

Metric Before After
Codecov fully-covered lines today 32/39 39/39
Codecov fully-covered lines after #8040 31/39 39/39
Branch arms 9/12 12/12

texera_run_python_worker.py reaches 100% on both lines and branches. +7 fully-covered lines today, +8 and +3 arms after #8040 — line 155 is a partial that the current branch-less report already counts covered, because it executes at import.

The uncovered set was the argument-parsing failures (26–28), the loguru level configuration (35), and the missing-dependency and R_HOME paths (48, 51, 155–156). These are what run when the worker starts with a malformed or incomplete configuration, so a regression there surfaces as a worker that fails to start with an unhelpful message.

One correction to the assessment worth recording: line 143, the R_HOME guard, was already at 2/2 — it was not part of the gap.

Verification

Two reviewers returned nine findings and reported thirteen surviving mutants against the first draft. All thirteen were independently reproduced on a pristine tree before being fixed, and all thirteen now die.

39 mutations in the final table: 38 killed, 1 equivalent, 0 real survivors. The equivalent one is frozenset to set for EXPECTED_CONFIG_KEYS — reported rather than omitted.

The repair also confirmed something worth stating about the measurement itself: pytest's importlib import mode re-executes this module per test, and there is exactly one <class filename="texera_run_python_worker.py"> entry in every report, on both sides and in the full-suite run. So the fresh execs attribute to the same source path rather than splitting the coverage entry — checked rather than assumed.

Measured from amber/ with an identical single-spec test-path scope on both sides, both with and without --cov-branch (the no-branch figure measured directly, not derived), per-line hits and condition-coverage read out of coverage.xml rather than from the summary percentage.

Full pyamber suite: 5 failed, 1174 passed, 7 errors against main's 1159 passed. Failure sets sorted and diffed — identical by identity, 12 entries. ruff check and ruff format --check pass on CI's exact scope.

No production file is touched; git diff -- 'amber/src/main/*' is empty.

Any related issues, documentation, discussions?

Closes #8046

How was this PR tested?

cd amber && python -m pytest -m "not integration" -q src/test/python/test_run_python_worker.py
42 passed

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

Generated-by: Claude Code (Opus 5)

Copilot AI lite review requested due to automatic review settings August 27, 2026 11:46

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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@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:

  • Contributors with relevant context: @yangzhang75
    You can notify them by mentioning @yangzhang75 in a comment.

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.43%. Comparing base (b01b11f) to head (1b7d553).

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #8050      +/-   ##
============================================
+ Coverage     93.41%   93.43%   +0.01%     
  Complexity     4696     4696              
============================================
  Files          1179     1179              
  Lines         47713    47713              
  Branches       5314     5314              
============================================
+ Hits          44573    44580       +7     
+ Misses         1698     1691       -7     
  Partials       1442     1442              
Flag Coverage Δ *Carryforward flag
access-control-service 81.00% <ø> (ø) Carriedforward from b01b11f
agent-service 99.32% <ø> (ø) Carriedforward from b01b11f
amber 89.60% <ø> (ø) Carriedforward from b01b11f
computing-unit-managing-service 73.67% <ø> (ø) Carriedforward from b01b11f
config-service 86.73% <ø> (ø) Carriedforward from b01b11f
file-service 86.70% <ø> (ø) Carriedforward from b01b11f
frontend 95.89% <ø> (ø) Carriedforward from b01b11f
notebook-migration-service 79.31% <ø> (ø) Carriedforward from b01b11f
pyamber 97.93% <ø> (+0.15%) ⬆️
workflow-compiling-service 77.19% <ø> (ø) Carriedforward from b01b11f

*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.

@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Benchmark changes need a look

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

Compared against main b01b11f 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 381 0.232 25,710/36,666/36,666 us 🔴 +22.8% / 🔴 +164.9%
bs=100 sw=10 sl=64 812 0.496 119,088/167,242/167,242 us ⚪ within ±5% / 🔴 +78.5%
bs=1000 sw=10 sl=64 956 0.583 1,042,235/1,113,594/1,113,594 us ⚪ within ±5% / 🔴 +23.3%
Baseline details

Latest main b01b11f from same runner

config metric PR latest main 7d avg Δ latest Δ 7d
bs=10 sw=10 sl=64 throughput 381 tuples/sec 440 tuples/sec 894.86 tuples/sec -13.4% -57.4%
bs=10 sw=10 sl=64 MB/s 0.232 MB/s 0.268 MB/s 0.546 MB/s -13.4% -57.5%
bs=10 sw=10 sl=64 p50 25,710 us 20,936 us 11,139 us +22.8% +130.8%
bs=10 sw=10 sl=64 p95 36,666 us 32,305 us 13,843 us +13.5% +164.9%
bs=10 sw=10 sl=64 p99 36,666 us 32,305 us 16,839 us +13.5% +117.7%
bs=100 sw=10 sl=64 throughput 812 tuples/sec 832 tuples/sec 1,166 tuples/sec -2.4% -30.3%
bs=100 sw=10 sl=64 MB/s 0.496 MB/s 0.508 MB/s 0.711 MB/s -2.4% -30.3%
bs=100 sw=10 sl=64 p50 119,088 us 115,920 us 87,509 us +2.7% +36.1%
bs=100 sw=10 sl=64 p95 167,242 us 159,945 us 93,675 us +4.6% +78.5%
bs=100 sw=10 sl=64 p99 167,242 us 159,945 us 102,153 us +4.6% +63.7%
bs=1000 sw=10 sl=64 throughput 956 tuples/sec 971 tuples/sec 1,198 tuples/sec -1.5% -20.2%
bs=1000 sw=10 sl=64 MB/s 0.583 MB/s 0.593 MB/s 0.731 MB/s -1.7% -20.3%
bs=1000 sw=10 sl=64 p50 1,042,235 us 1,029,828 us 859,766 us +1.2% +21.2%
bs=1000 sw=10 sl=64 p95 1,113,594 us 1,094,755 us 902,897 us +1.7% +23.3%
bs=1000 sw=10 sl=64 p99 1,113,594 us 1,094,755 us 937,957 us +1.7% +18.7%
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,525.45,200,128000,381,0.232,25709.90,36665.53,36665.53
1,100,10,64,20,2462.41,2000,1280000,812,0.496,119088.18,167241.90,167241.90
2,1000,10,64,20,20924.20,20000,12800000,956,0.583,1042234.96,1113594.47,1113594.47

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

@mengw15 mengw15 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

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 the pyamber worker entry point

4 participants