Skip to content

test(amber): cover the DB-backed TryLockRequest branches in CollaborationResource - #7254

Merged
aglinxinyuan merged 2 commits into
apache:mainfrom
mengw15:chore/7228-collaboration-lock-test
Aug 3, 2026
Merged

test(amber): cover the DB-backed TryLockRequest branches in CollaborationResource#7254
aglinxinyuan merged 2 commits into
apache:mainfrom
mengw15:chore/7228-collaboration-lock-test

Conversation

@mengw15

@mengw15 mengw15 commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

Extends the existing CollaborationResourceSpec to cover the TryLockRequest
branches that consult the database
(amber/src/main/scala/org/apache/texera/web/resource/CollaborationResource.scala).
The prior spec (#6904) left these uncovered because they reach
WorkflowAccessResource.hasWriteAccess, which needs SqlServer. No production
code was changed.

The suite now also mixes in MockTexeraDB: for these three tests a user, a
workflow and one workflow_user_access row are seeded so
checkIsReadOnlyhasWriteAccess reads a real privilege. The session is still
driven through myOnMsg with the existing mocked javax.websocket.Session, and
outgoing events are asserted from the captured send buffer. +3 tests:

  • read-only user (workflow_user_access = READ) → LockRejectedEvent +
    WorkflowAccessEvent(workflowReadonly=true), and the holder slot is set to the
    null sentinel.
  • writable user (WRITE) with no current holder → WorkflowAccessEvent(readonly=false)
    • LockGrantedEvent, and the holder becomes the sender.
  • writable user when a different session already holds the lock →
    WorkflowAccessEvent(readonly=false) + LockRejectedEvent, holder unchanged.

Any related issues, documentation, discussions?

Closes #7228

How was this PR tested?

Extended unit tests, run locally against embedded Postgres (MockTexeraDB). All
pass, and the failure path was verified by breaking an assertion to confirm the
suite goes red:

sbt "WorkflowExecutionService/testOnly *CollaborationResourceSpec"
# Tests: succeeded 16, failed 0
sbt "WorkflowExecutionService/Test/scalafmtCheck"       # clean
sbt "WorkflowExecutionService/Test/scalafix --check"    # clean

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

Generated-by: Claude Code (Opus 4.8 [1M context])

@github-actions

github-actions Bot commented Aug 2, 2026

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.

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

Extends the Amber websocket collaboration endpoint test suite (CollaborationResourceSpec) to exercise the TryLockRequest branches that call into WorkflowAccessResource.hasWriteAccess by backing those paths with MockTexeraDB (embedded Postgres) and seeding the minimal user, workflow, and workflow_user_access rows needed per test. No production code changes are included.

Changes:

  • Mixes MockTexeraDB into CollaborationResourceSpec and adds DB lifecycle + helper methods to seed/cleanup access rows.
  • Adds a helper to register an authenticated session on a seeded workflow (lockingSession).
  • Adds 3 new TryLockRequest tests covering read-only rejection, writable grant, and writable rejection when another session holds the lock.

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

@github-actions

github-actions Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

⚠️ Benchmark changes need a look

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

Compared against main 436b37e 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 433 0.264 22,163/29,918/29,918 us 🟢 -12.2% / 🔴 +94.4%
🟢 bs=100 sw=10 sl=64 926 0.565 103,993/127,262/127,262 us 🟢 -28.4% / 🔴 +20.2%
🔴 bs=1000 sw=10 sl=64 1,080 0.659 921,017/1,035,316/1,035,316 us 🔴 +8.1% / 🟢 -5.7%
Baseline details

Latest main 436b37e from same runner

config metric PR latest main 7d avg Δ latest Δ 7d
bs=10 sw=10 sl=64 throughput 433 tuples/sec 454 tuples/sec 790.88 tuples/sec -4.6% -45.3%
bs=10 sw=10 sl=64 MB/s 0.264 MB/s 0.277 MB/s 0.483 MB/s -4.7% -45.3%
bs=10 sw=10 sl=64 p50 22,163 us 21,363 us 12,348 us +3.7% +79.5%
bs=10 sw=10 sl=64 p95 29,918 us 34,076 us 15,390 us -12.2% +94.4%
bs=10 sw=10 sl=64 p99 29,918 us 34,076 us 18,935 us -12.2% +58.0%
bs=100 sw=10 sl=64 throughput 926 tuples/sec 926 tuples/sec 1,009 tuples/sec 0.0% -8.2%
bs=100 sw=10 sl=64 MB/s 0.565 MB/s 0.565 MB/s 0.616 MB/s 0.0% -8.2%
bs=100 sw=10 sl=64 p50 103,993 us 101,884 us 99,753 us +2.1% +4.3%
bs=100 sw=10 sl=64 p95 127,262 us 177,668 us 105,895 us -28.4% +20.2%
bs=100 sw=10 sl=64 p99 127,262 us 177,668 us 113,263 us -28.4% +12.4%
bs=1000 sw=10 sl=64 throughput 1,080 tuples/sec 1,103 tuples/sec 1,037 tuples/sec -2.1% +4.1%
bs=1000 sw=10 sl=64 MB/s 0.659 MB/s 0.673 MB/s 0.633 MB/s -2.1% +4.1%
bs=1000 sw=10 sl=64 p50 921,017 us 900,993 us 976,194 us +2.2% -5.7%
bs=1000 sw=10 sl=64 p95 1,035,316 us 957,597 us 1,016,411 us +8.1% +1.9%
bs=1000 sw=10 sl=64 p99 1,035,316 us 957,597 us 1,045,212 us +8.1% -0.9%
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,462.26,200,128000,433,0.264,22162.52,29918.24,29918.24
1,100,10,64,20,2160.30,2000,1280000,926,0.565,103992.75,127262.17,127262.17
2,1000,10,64,20,18523.30,20000,12800000,1080,0.659,921017.26,1035316.26,1035316.26

@codecov-commenter

codecov-commenter commented Aug 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.67%. Comparing base (436b37e) to head (2c86c40).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #7254      +/-   ##
============================================
+ Coverage     82.64%   82.67%   +0.02%     
- Complexity     4080     4084       +4     
============================================
  Files          1162     1162              
  Lines         46282    46282              
  Branches       5160     5160              
============================================
+ Hits          38250    38263      +13     
+ Misses         6341     6324      -17     
- Partials       1691     1695       +4     
Flag Coverage Δ *Carryforward flag
access-control-service 70.00% <ø> (ø) Carriedforward from 8efd3ea
agent-service 83.65% <ø> (ø) Carriedforward from 8efd3ea
amber 79.37% <ø> (+0.07%) ⬆️
computing-unit-managing-service 43.60% <ø> (ø) Carriedforward from 8efd3ea
config-service 65.97% <ø> (ø) Carriedforward from 8efd3ea
file-service 66.80% <ø> (ø) Carriedforward from 8efd3ea
frontend 83.55% <ø> (ø) Carriedforward from 8efd3ea
notebook-migration-service 78.89% <ø> (ø) Carriedforward from 8efd3ea
pyamber 97.36% <ø> (ø) Carriedforward from 8efd3ea
workflow-compiling-service 26.31% <ø> (ø) Carriedforward from 8efd3ea

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

@mengw15
mengw15 requested a review from aglinxinyuan August 2, 2026 14:16
… DB-backed lock tests

The header still claimed the spec touches no database and left the
hasWriteAccess branches uncovered; the new TryLockRequest tests now cover them
via MockTexeraDB, so update the comment accordingly.

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

@aglinxinyuan
aglinxinyuan added this pull request to the merge queue Aug 3, 2026
Merged via the queue into apache:main with commit 2538d6a Aug 3, 2026
21 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.

extend CollaborationResource unit test coverage (collaboration lock handling)

4 participants