test(workflow-execution-service): add CollaborationResource unit tests#6904
Conversation
Covers the session lifecycle, WIdRequest bookkeeping, CommandRequest and RestoreVersionRequest fan-out, heartbeat, and the non-DB locking branches against a mocked javax.websocket.Session. The two branches that reach WorkflowAccessResource.hasWriteAccess need SqlServer and stay uncovered.
Automated Reviewer SuggestionsBased on the
|
There was a problem hiding this comment.
✅ Ready to approve
The change is test-only and the added coverage matches the stated scope, with only a minor naming/shadowing nit noted.
Note: this review does not count toward required approvals for merging.
Pull request overview
Adds a ScalaTest/ScalaMock unit-test suite for CollaborationResource (the WebSocket collaboration endpoint in Amber’s workflow-execution service) to cover its in-memory session bookkeeping and message fan-out logic without involving any database/network dependencies.
Changes:
- Introduces
CollaborationResourceSpecwith tests for session lifecycle (myOnOpen/myOnClose) andWIdRequestbookkeeping (authenticated + anonymous). - Adds fan-out tests for
CommandRequestandRestoreVersionRequest, plus a direct-response test forHeartBeatRequest. - Covers non-DB locking branches (
TryLockRequestonDUMMY_WID, andAcquireLockRequesthandoff / regrant / null-sentinel behavior) while isolating JVM-wide mutable state viabeforeEach.
File summaries
| File | Description |
|---|---|
| amber/src/test/scala/org/apache/texera/web/resource/CollaborationResourceSpec.scala | Adds unit tests that exercise CollaborationResource’s session maps and message propagation behavior using mocked javax.websocket.Session. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Low
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 410 | 0.25 | 24,049/32,106/32,106 us | 🔴 +12.3% / 🔴 +93.5% |
| ⚪ | bs=100 sw=10 sl=64 | 827 | 0.505 | 119,317/138,363/138,363 us | ⚪ within ±5% / 🔴 +26.8% |
| 🔴 | bs=1000 sw=10 sl=64 | 933 | 0.569 | 1,064,474/1,213,736/1,213,736 us | 🔴 +10.7% / 🔴 +16.0% |
Baseline details
Latest main 4af3a85 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 410 tuples/sec | 441 tuples/sec | 754.55 tuples/sec | -7.0% | -45.7% |
| bs=10 sw=10 sl=64 | MB/s | 0.25 MB/s | 0.269 MB/s | 0.461 MB/s | -7.1% | -45.7% |
| bs=10 sw=10 sl=64 | p50 | 24,049 us | 21,409 us | 12,816 us | +12.3% | +87.7% |
| bs=10 sw=10 sl=64 | p95 | 32,106 us | 31,862 us | 16,594 us | +0.8% | +93.5% |
| bs=10 sw=10 sl=64 | p99 | 32,106 us | 31,862 us | 19,806 us | +0.8% | +62.1% |
| bs=100 sw=10 sl=64 | throughput | 827 tuples/sec | 845 tuples/sec | 969.38 tuples/sec | -2.1% | -14.7% |
| bs=100 sw=10 sl=64 | MB/s | 0.505 MB/s | 0.516 MB/s | 0.592 MB/s | -2.1% | -14.6% |
| bs=100 sw=10 sl=64 | p50 | 119,317 us | 117,644 us | 103,584 us | +1.4% | +15.2% |
| bs=100 sw=10 sl=64 | p95 | 138,363 us | 140,771 us | 109,097 us | -1.7% | +26.8% |
| bs=100 sw=10 sl=64 | p99 | 138,363 us | 140,771 us | 117,304 us | -1.7% | +18.0% |
| bs=1000 sw=10 sl=64 | throughput | 933 tuples/sec | 942 tuples/sec | 1,004 tuples/sec | -1.0% | -7.0% |
| bs=1000 sw=10 sl=64 | MB/s | 0.569 MB/s | 0.575 MB/s | 0.613 MB/s | -1.0% | -7.1% |
| bs=1000 sw=10 sl=64 | p50 | 1,064,474 us | 1,060,611 us | 1,002,357 us | +0.4% | +6.2% |
| bs=1000 sw=10 sl=64 | p95 | 1,213,736 us | 1,096,568 us | 1,046,463 us | +10.7% | +16.0% |
| bs=1000 sw=10 sl=64 | p99 | 1,213,736 us | 1,096,568 us | 1,073,661 us | +10.7% | +13.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,488.09,200,128000,410,0.250,24049.43,32106.34,32106.34
1,100,10,64,20,2417.72,2000,1280000,827,0.505,119317.27,138363.29,138363.29
2,1000,10,64,20,21444.97,20000,12800000,933,0.569,1064473.96,1213735.58,1213735.58
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6904 +/- ##
============================================
+ Coverage 78.39% 78.50% +0.10%
- Complexity 3723 3749 +26
============================================
Files 1161 1161
Lines 46066 46066
Branches 5107 5107
============================================
+ Hits 36112 36162 +50
+ Misses 8343 8278 -65
- Partials 1611 1626 +15
*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:
|
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Meng Wang <mengw15@uci.edu>
What changes were proposed in this PR?
CollaborationResourcehad no spec and sat at 0% — all 87 tracked lines unhit. It reads as websocket-bound but the only collaborator is thejavax.websocket.Sessioninterface, which mocks cleanly with the ScalaMock already on amber's test classpath, so the session bookkeeping and message fan-out are ordinary unit-testable logic.Adds
CollaborationResourceSpecwith 13 tests covering the session lifecycle (myOnOpen,myOnClose),WIdRequestbookkeeping on both the authenticated and anonymous paths,CommandRequestandRestoreVersionRequestfan-out,HeartBeatRequest, and the locking branches that do not touch the database. AmockSessionhelper returns aSessionwith a fixed id whose outgoing messages are collected into a buffer; requests are built by serializing the real request case classes, so the"type"discriminator cannot drift out of sync with the production@JsonTypeInfoconfig.Three behaviors are worth calling out because they are subtle rather than obvious:
WIdRequesttest pins theset.union(Set(senderSessId))line, which returns a fresh set instead of mutating and only works because the result is reassigned into the map.wIdLockHolderSessionIdMapstores anullsentinel for "no holder", which is a distinct state from an absent key. With the sentinel in place,AcquireLockRequestcannot resolve the holder session and rethrows — the spec pins that current behavior.The five bookkeeping maps live on the companion object and are therefore JVM-wide mutable state, so
beforeEachclears all five; that is what keeps the suite order-independent. There are no clocks, threads, temp files or network calls in the spec.Any related issues, documentation, discussions?
Closes #6900.
The read-only
TryLockRequestrejection and the lock hand-off insidemyOnCloseboth reachWorkflowAccessResource.hasWriteAccessand thereforeSqlServer; they are left uncovered rather than pullingMockTexeraDB(and an embedded Postgres process) in for two branches.How was this PR tested?
sbt "WorkflowExecutionService/testOnly *CollaborationResourceSpec"passes with 13 tests;Test/scalafmtCheckandTest/scalafix --checkare clean. For the failure path, the self-exclusion guard in theCommandRequestfan-out was temporarily removed from the production code, which reddened the fan-out test and exited non-zero, and the guard was then restored — so the assertions genuinely constrain the behavior.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (claude-opus-5)