test(amber): add unit test coverage for AdminUserResource - #7167
Conversation
Automated Reviewer SuggestionsBased on the
|
There was a problem hiding this comment.
🟡 Not ready to approve
The new spec’s cleanup logic does not remove addUser()-created rows (and includes a misleading comment about when cleanup runs), which breaks test hermeticity and can cause future flakiness.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Adds an initial ScalaTest spec for the Amber admin user-management REST resource (AdminUserResource), using MockTexeraDB (embedded Postgres + jOOQ DAOs) to seed and assert DB-backed behavior.
Changes:
- Introduces
AdminUserResourceSpecwith unit tests covering list/add/update and several per-user workflow/quota endpoints. - Seeds users/workflows/executions via jOOQ DAOs and validates both empty and populated cases.
- Adds a delete-collection test asserting DB-side deletion behavior.
File summaries
| File | Description |
|---|---|
| amber/src/test/scala/org/apache/texera/web/resource/dashboard/admin/user/AdminUserResourceSpec.scala | New unit-test suite for AdminUserResource using embedded Postgres and jOOQ DAO seeding. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 381 | 0.232 | 26,610/36,003/36,003 us | 🔴 +16.6% / 🔴 +128.1% |
| 🔴 | bs=100 sw=10 sl=64 | 894 | 0.546 | 108,431/155,063/155,063 us | 🔴 +20.5% / 🔴 +40.6% |
| ⚪ | bs=1000 sw=10 sl=64 | 1,086 | 0.663 | 918,541/964,470/964,470 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 | 381 tuples/sec | 442 tuples/sec | 771.62 tuples/sec | -13.8% | -50.6% |
| bs=10 sw=10 sl=64 | MB/s | 0.232 MB/s | 0.27 MB/s | 0.471 MB/s | -14.1% | -50.7% |
| bs=10 sw=10 sl=64 | p50 | 26,610 us | 22,829 us | 12,626 us | +16.6% | +110.8% |
| bs=10 sw=10 sl=64 | p95 | 36,003 us | 31,050 us | 15,786 us | +15.9% | +128.1% |
| bs=10 sw=10 sl=64 | p99 | 36,003 us | 31,050 us | 19,344 us | +15.9% | +86.1% |
| bs=100 sw=10 sl=64 | throughput | 894 tuples/sec | 975 tuples/sec | 971.56 tuples/sec | -8.3% | -8.0% |
| bs=100 sw=10 sl=64 | MB/s | 0.546 MB/s | 0.595 MB/s | 0.593 MB/s | -8.2% | -7.9% |
| bs=100 sw=10 sl=64 | p50 | 108,431 us | 101,558 us | 103,463 us | +6.8% | +4.8% |
| bs=100 sw=10 sl=64 | p95 | 155,063 us | 128,690 us | 110,296 us | +20.5% | +40.6% |
| bs=100 sw=10 sl=64 | p99 | 155,063 us | 128,690 us | 118,690 us | +20.5% | +30.6% |
| bs=1000 sw=10 sl=64 | throughput | 1,086 tuples/sec | 1,087 tuples/sec | 1,001 tuples/sec | -0.1% | +8.5% |
| bs=1000 sw=10 sl=64 | MB/s | 0.663 MB/s | 0.663 MB/s | 0.611 MB/s | 0.0% | +8.5% |
| bs=1000 sw=10 sl=64 | p50 | 918,541 us | 919,757 us | 1,008,988 us | -0.1% | -9.0% |
| bs=1000 sw=10 sl=64 | p95 | 964,470 us | 963,532 us | 1,055,260 us | +0.1% | -8.6% |
| bs=1000 sw=10 sl=64 | p99 | 964,470 us | 963,532 us | 1,074,689 us | +0.1% | -10.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,525.54,200,128000,381,0.232,26610.37,36002.97,36002.97
1,100,10,64,20,2237.07,2000,1280000,894,0.546,108430.69,155063.35,155063.35
2,1000,10,64,20,18415.94,20000,12800000,1086,0.663,918540.62,964470.00,964470.00
e4ee7eb to
346f427
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7167 +/- ##
============================================
+ Coverage 79.62% 79.82% +0.20%
- Complexity 3836 3842 +6
============================================
Files 1160 1160
Lines 46188 46188
Branches 5145 5145
============================================
+ Hits 36777 36870 +93
+ Misses 7778 7678 -100
- Partials 1633 1640 +7
*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:
|
What changes were proposed in this PR?
Adds
AdminUserResourceSpec, the first spec forAdminUserResource(previously~0% coverage). It runs against embedded Postgres via
MockTexeraDBand seedsrows through the jOOQ DAOs, with 12 tests covering the admin user-management
methods, empty and populated:
list— returns the seeded users with name / email / role.addUser— persists a newINACTIVEuser.updateUser— round-trips an editable-field change; rejects an email thatalready belongs to another user (
WebApplicationException).getCreatedWorkflow/getAccessedWorkflow— the per-user created / accessibleworkflow lists.
getUserQuota— the per-user quota view.deleteCollection— removes the target execution row.Two side-effecting branches are intentionally left to their owning code paths so
this stays a hermetic unit test:
updateUser's role-change branch (sends ane-mail via
GmailResource) is not exercised — only the no-role-change andconflict paths are; and
deleteCollectionis asserted on its DB delete (theIceberg document cleanup is a best-effort, exception-swallowed step).
Any related issues, documentation, discussions?
Closes #7163.
How was this PR tested?
sbt "WorkflowExecutionService/testOnly *AdminUserResourceSpec"— 12 succeeded, 0 failed.Test/scalafmtCheckandTest/scalafix --checkclean. Failure path verified by breaking an assertion (→ 1 failed, non-zero exit) and restoring.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 4.8 [1M context])