Skip to content

test(amber): cover websocket session state - #7157

Open
aglinxinyuan wants to merge 2 commits into
apache:mainfrom
aglinxinyuan:test/session-state
Open

test(amber): cover websocket session state#7157
aglinxinyuan wants to merge 2 commits into
apache:mainfrom
aglinxinyuan:test/session-state

Conversation

@aglinxinyuan

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

Adds focused unit coverage for websocket event delivery, workflow-service subscription replacement and cleanup, session-map removal, and computing-unit access state.

Any related issues, documentation, discussions?

Closes #7153

How was this PR tested?

  • WorkflowExecutionService/testOnly org.apache.texera.web.SessionStateSpec — 3 succeeded.
  • WorkflowExecutionService/Test/scalafmtCheck and WorkflowExecutionService/Test/scalafix --check.

Mutation proof:

Production mutation Result
Replaced the serialized websocket payload with an empty string 1 test failed
Stopped retaining the subscribed workflow service 1 test failed
Stopped removing a session from the session-state map 1 test failed

Each mutation was reverted before the final green test run.

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

Generated-by: Codex (GPT-5)

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

@aglinxinyuan
aglinxinyuan marked this pull request as ready for review July 31, 2026 04:16
@aglinxinyuan
aglinxinyuan requested a review from mengw15 July 31, 2026 04:16
@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Benchmark changes need a look

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

Compared against main 45b7b20 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 385 0.235 25,514/35,984/35,984 us 🔴 +6.1% / 🔴 +127.9%
🟢 bs=100 sw=10 sl=64 780 0.476 127,587/149,078/149,078 us 🟢 -10.7% / 🔴 +35.2%
bs=1000 sw=10 sl=64 898 0.548 1,107,728/1,158,281/1,158,281 us ⚪ within ±5% / 🔴 -10.3%
Baseline details

Latest main 45b7b20 from same runner

config metric PR latest main 7d avg Δ latest Δ 7d
bs=10 sw=10 sl=64 throughput 385 tuples/sec 384 tuples/sec 771.62 tuples/sec +0.3% -50.1%
bs=10 sw=10 sl=64 MB/s 0.235 MB/s 0.234 MB/s 0.471 MB/s +0.4% -50.1%
bs=10 sw=10 sl=64 p50 25,514 us 24,040 us 12,626 us +6.1% +102.1%
bs=10 sw=10 sl=64 p95 35,984 us 37,136 us 15,786 us -3.1% +127.9%
bs=10 sw=10 sl=64 p99 35,984 us 37,136 us 19,344 us -3.1% +86.0%
bs=100 sw=10 sl=64 throughput 780 tuples/sec 777 tuples/sec 971.56 tuples/sec +0.4% -19.7%
bs=100 sw=10 sl=64 MB/s 0.476 MB/s 0.474 MB/s 0.593 MB/s +0.4% -19.7%
bs=100 sw=10 sl=64 p50 127,587 us 127,582 us 103,463 us +0.0% +23.3%
bs=100 sw=10 sl=64 p95 149,078 us 166,876 us 110,296 us -10.7% +35.2%
bs=100 sw=10 sl=64 p99 149,078 us 166,876 us 118,690 us -10.7% +25.6%
bs=1000 sw=10 sl=64 throughput 898 tuples/sec 910 tuples/sec 1,001 tuples/sec -1.3% -10.3%
bs=1000 sw=10 sl=64 MB/s 0.548 MB/s 0.555 MB/s 0.611 MB/s -1.3% -10.3%
bs=1000 sw=10 sl=64 p50 1,107,728 us 1,097,216 us 1,008,988 us +1.0% +9.8%
bs=1000 sw=10 sl=64 p95 1,158,281 us 1,143,127 us 1,055,260 us +1.3% +9.8%
bs=1000 sw=10 sl=64 p99 1,158,281 us 1,143,127 us 1,074,689 us +1.3% +7.8%
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,519.42,200,128000,385,0.235,25513.80,35984.12,35984.12
1,100,10,64,20,2564.54,2000,1280000,780,0.476,127587.14,149078.34,149078.34
2,1000,10,64,20,22281.01,20000,12800000,898,0.548,1107728.30,1158281.16,1158281.16

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.72%. Comparing base (45b7b20) to head (03d2b6c).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #7157      +/-   ##
============================================
+ Coverage     79.62%   79.72%   +0.09%     
- Complexity     3836     3855      +19     
============================================
  Files          1160     1160              
  Lines         46188    46188              
  Branches       5145     5145              
============================================
+ Hits          36777    36823      +46     
+ Misses         7778     7735      -43     
+ Partials       1633     1630       -3     
Flag Coverage Δ *Carryforward flag
access-control-service 70.00% <ø> (ø) Carriedforward from 45b7b20
agent-service 77.42% <ø> (ø) Carriedforward from 45b7b20
amber 73.36% <ø> (+0.27%) ⬆️
computing-unit-managing-service 20.49% <ø> (ø) Carriedforward from 45b7b20
config-service 65.97% <ø> (ø) Carriedforward from 45b7b20
file-service 66.80% <ø> (ø) Carriedforward from 45b7b20
frontend 83.16% <ø> (ø) Carriedforward from 45b7b20
notebook-migration-service 78.89% <ø> (ø) Carriedforward from 45b7b20
pyamber 97.36% <ø> (ø) Carriedforward from 45b7b20
workflow-compiling-service 26.31% <ø> (ø) Carriedforward from 45b7b20

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

Adds a new Amber unit-test spec to validate server-side websocket session state behavior, aligning with the Amber web layer’s SessionState responsibilities (event delivery + workflow-service subscription lifecycle + session registry + computing-unit privilege state).

Changes:

  • Introduces SessionStateSpec covering websocket JSON event serialization via SessionState.send.
  • Adds coverage for workflow-service subscription replacement / cleanup via SessionState.subscribe + unsubscribe.
  • Adds coverage for session registry operations (setState / getState / removeState) and computing-unit access state behavior.

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

Comment on lines +129 to +147
it should "remove registered sessions and retain their computing-unit access level" in {
val sessionId = UUID.randomUUID().toString
val state = new SessionState(stub[Session])
SessionState.setState(sessionId, state)
try {
SessionState.getState(sessionId) shouldBe state
SessionState.getAllSessionStates should contain(state)

state.getUserComputingUnitAccess shouldBe PrivilegeEnum.NONE
state.setUserComputingUnitAccess(PrivilegeEnum.WRITE)
state.getUserComputingUnitAccess shouldBe PrivilegeEnum.WRITE

SessionState.removeState(sessionId)
SessionState.getAllSessionStates should not contain state
a[NoSuchElementException] should be thrownBy SessionState.getState(sessionId)
} finally {
removeStateIfPresent(sessionId)
}
}
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 tests for websocket session state

4 participants