aglinxinyuan opened a new pull request, #7545: URL: https://github.com/apache/texera/pull/7545
### What changes were proposed in this PR? `WorkflowService` had no spec and sat at **14.6% of its 89 lines**. It owns what a connected client sees: the per-session subscriptions to the workflow's own stores, the switch to the newest execution's stores when one is published, and the execution state reported on disconnect — the value `WorkflowLifecycleManager.decreaseUserCount` branches on to decide whether clean-up is postponed. Adds 8 tests. Construction is database-free (`SessionStateSpec` already subclasses the class with no fixture), and executions are real `WorkflowExecutionService` instances with `coordinatorConfig`/`resultService` as `null` — the pattern `WorkflowExecutionServiceSpec` establishes — so the observed events travel through the production diff handler. Two collaborators are stubbed, for reasons stated in the spec header: `lifeCycleManager`, because the real one schedules on an actor system no unit test starts and reads `workflow_executions` from the database; and `resultService`, because a real one holds no subscriptions until `attachToExecution` gives it a live client, so "unsubscribed" would be indistinguishable from "never called". ### Verification 14 mutations applied and reverted, production diff empty after each. **Four assertions were found to be vacuous in review and strengthened** — all four on lines the tests claimed to cover: | Weakness | Why it passed | Fix | |---|---|---| | `disconnect` state read unpinned | RUNNING was the only non-empty case, so reading the execution's state was indistinguishable from returning the constant RUNNING — which would postpone clean-up forever for any workflow that ever had an execution | a second disconnect asserting COMPLETED distinctly | | execution fan-out unpinned | `getAllStores` returns five stores; the spec drove only `metadataStore`, so the fan-out could be narrowed to that one and a client would silently stop receiving stats, console, breakpoint and reconfiguration events | drive a second store, with a diff handler registered since `ExecutionStatsService` is not attached here | | workflow-level forwarding truncation | every diff yielded one event, so `evts.foreach(onNext)` could become `evts.headOption.foreach(onNext)` | a two-event diff, asserted as two | | execution-level forwarding truncation | same, on the other closure | same | All four mutations are now red. Multi-event diffs are not hypothetical: `WorkflowExecutionService`'s own handler appends a state event and an error event together. ### Deliberately not included - `initExecutionService` past its user-id check: the rest inserts an execution row then hands a compiled plan to `ComputingUnitMaster.createAmberRuntime`, which builds an `AmberClient` on a null actor system outside a started coordinator. Reaching past the insert would mean asserting on that NPE — an accident, not a contract. - `clearExecutionResources` and its clean-up callback: both resolve URIs out of the database and open Iceberg documents. - `lastCompletedLogicalPlan`: nothing in the repository reads it, so a test could only pin a write-only var. - Dropping the outer handle from `new CompositeDisposable(localDisposable, disposable)` — the `DO NOT OPTIMIZE` line. That mutation **survives**, and it is reported rather than papered over: it leaks the `executionService` subscription but changes nothing observable, because the already-disposed inner composite immediately disposes anything a later callback adds. Pinning it would mean asserting on subscriber bookkeeping rather than behaviour. Noted in the spec so the next reader does not re-derive it. No production file is touched. ### Any related issues, documentation, discussions? Closes #7543 ### How was this PR tested? ``` sbt "WorkflowExecutionService/testOnly org.apache.texera.web.service.WorkflowServiceSpec" ``` ``` [info] Total number of tests run: 8 [info] Tests: succeeded 8, failed 0, canceled 0, ignored 0, pending 0 ``` Also green in a five-suite single-JVM run alongside `WorkflowExecutionServiceSpec`, `ClusterListenerSpec` and `ExecutionResultServiceSpec` (36 tests). `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]
