The GitHub Actions job "Required Checks" on texera.git/main has failed. Run started by GitHub user github-merge-queue[bot] (triggered by github-merge-queue[bot]).
Head commit for run: ae04417af8a7da46c5b3c2a3d8da67c7065b58f4 / Xinyuan Lin <[email protected]> test(amber): cover the websocket handshake and session wiring (#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) Report URL: https://github.com/apache/texera/actions/runs/31862719277 With regards, GitHub Actions via GitBox
