aglinxinyuan opened a new pull request, #7676:
URL: https://github.com/apache/texera/pull/7676
### What changes were proposed in this PR?
`ServletAwareConfigurator` had **no spec**, and it is the handshake hook
both websocket endpoints declare via `@ServerEndpoint(configurator = ...)` --
it decides who the user is and what computing-unit privilege they carry for
every connection. `WorkflowWebsocketResource` consumes exactly what it writes,
and its `myOnOpen` was entirely uncovered. Covering them together is what makes
the pair meaningful: the tests hand the configurator's real output to the
resource rather than a hand-built map.
Measured with only these two specs running, so the numbers are attributable
to them alone:
| File | Lines before | Lines after | Branches after |
|---|---|---|---|
| `ServletAwareConfigurator.scala` | 0/27 (0%) | **27/27 (100%)** | 16/18 |
| `WorkflowWebsocketResource.scala` | 53.2% | **61/62 (98.4%)** | 26/34 |
Tests **6 -> 22**. `ServletAwareConfiguratorSpec` is new (7 tests);
`WorkflowWebsocketResourceSpec` gains 9.
Covered: both handshake modes and the four-way header guard leg by leg, the
JWT query-parameter path with a token minted in-process, both failure arms and
the exact partial state each leaves behind, `myOnOpen`'s
workflow/computing-unit/privilege binding and its deliberate event ordering,
and `myOnMsg`'s pagination, modify-logic, unrecognised-command and
failure-mapping arms.
### Verification
32 mutations applied one at a time and reverted, production diff empty after
each. **All 32 red, no survivors.** 18 during the build; 14 more added by
review, every one of which was first *run* to confirm it survived before any
test was changed, then re-run to prove the fix goes red on the intended test by
name (read from the JUnit XML, since the console log does not name tests).
The review round is where the real work was. Examples of what it caught:
| Weakness | Why it passed | Fix |
|---|---|---|
| the pagination arm asserted only the response type | the stub returned a
canned value, so a rewritten `pageIndex` shipped green | assert the stub
received the same requestID/operatorID/pageIndex it was sent |
| `numWorkers` asserted by value | `0` is the literal initializer of
`numWorkerNodesInCluster` | assert event presence and order |
| the repeated-header case | `headOption` vs `lastOption` was
indistinguishable | feed a genuinely repeated header |
### On amber's test parallelism
Three findings hinged on whether sibling amber suites run concurrently, so
review measured it instead of arguing: three probe suites each sleeping 4s
while bumping a shared high-water mark ran in 12.3s with a concurrent maximum
of **1**. `amber/build.sbt:52` (`concurrentRestrictions in Global +=
Tags.limit(Tags.Test, 1)`) does serialize suites within this project. The probe
file was deleted afterwards. Suites still share one JVM, so the spec restores
every global it touches.
### Deliberately not included
One partial branch in `myOnOpen` needs a real cluster. Several defects found
along the way are reported rather than pinned, so a fix is not blocked by a
test asserting the current behaviour:
- `WorkflowWebsocketResource.scala:89` computes `executionStateOpt`
**before** the try, so a failure raised while the execution is being created
goes to the socket instead of the new execution's metadata store, where the
frontend's error panel reads it. *This one is pinned*, as characterization,
with a comment saying so.
- `:98-106` and `:124-126` both test `workflowStateOpt` where they mean
`executionStateOpt`, so a workflow with no execution NPEs instead of reporting
"not initialized".
- `:81` puts `objectMapper.readValue` outside the try, so an unparseable
frame escapes the error mapper and the client is told nothing.
- `ServletAwareConfigurator.scala:62`'s `.getOrElse("")` yields a privilege
string `PrivilegeEnum.valueOf` cannot parse; unreachable today, but
`getOrElse(PrivilegeEnum.NONE.name())` would be safer.
- `WorkflowService.getOrCreate` keys its cache on the workflow id alone, so
a second opener's `computingUnitId` is silently dropped. The cached arm is
covered by asserting *identity*; the dropped id is described in a comment but
**not** asserted, so fixing it will not break this spec.
No production file is touched.
### Any related issues, documentation, discussions?
Closes #7675
### How was this PR tested?
```
STORAGE_ICEBERG_CATALOG_TYPE=postgres sbt "WorkflowExecutionService/testOnly
org.apache.texera.web.ServletAwareConfiguratorSpec
org.apache.texera.web.resource.WorkflowWebsocketResourceSpec"
```
```
[info] Total number of tests run: 22
[info] Tests: succeeded 22, failed 0, canceled 0, ignored 0, pending 0
[info] All tests passed.
```
`Test/scalafmtCheck` and `Test/scalafix --check` both pass.
### Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 5)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
