aglinxinyuan opened a new issue, #7543: URL: https://github.com/apache/texera/issues/7543
### Task Summary `amber/src/main/scala/org/apache/texera/web/service/WorkflowService.scala` has **no spec and sits at 14.6% of its 89 lines**. It owns what a connected client actually sees: - `connect` — registers the session and subscribes to every one of the workflow's own stores; - `connectToExecution` — re-points those subscriptions at the newest execution whenever one is published, dropping the previous execution's; - `disconnect` — reports the current execution's state, which is the value `WorkflowLifecycleManager.decreaseUserCount` branches on to decide whether clean-up is postponed; - `unsubscribeAll` — tears down its own, the execution's and the result service's subscriptions. Construction is provably database-free: `SessionStateSpec` already subclasses `WorkflowService` with no fixture. Everything a test needs is public — the three-argument constructor, the `executionService` `BehaviorSubject`, `stateStore`, and `resultService` — and executions can be real `WorkflowExecutionService` instances with `coordinatorConfig`/`resultService` passed as `null`, the pattern `WorkflowExecutionServiceSpec` establishes, so the observed events travel through the production diff handler. Two stubs are worth explaining rather than discovering: - `lifeCycleManager`, because the real one schedules on `AmberRuntime.actorSystem` (which no unit test starts) and its callback reads `workflow_executions` from the database. Stubbing it is also what makes `connect` registering a user and `disconnect` reporting a state observable at all. - `resultService`, because a real `ExecutionResultService` holds no subscriptions until `attachToExecution` gives it a live Amber client — so "unsubscribed" would otherwise be indistinguishable from "never called". Not worth covering, and worth recording so nobody re-derives it: - `initExecutionService` past its user-id check, and `createWorkflowContext` with it: the rest inserts an execution row and 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 — pinning an accident, not a contract. - `clearExecutionResources` and its clean-up callback: both resolve URIs out of the database and open Iceberg documents. - `lastCompletedLogicalPlan` and the constructor block that maintains it: **nothing in the repository reads that field**, so a test could only assert which plan a write-only var holds. One further line resists a behavioural test: dropping the outer handle from `new CompositeDisposable(localDisposable, disposable)` — the line carrying the `DO NOT OPTIMIZE` comment — leaks the `executionService` subscription but changes nothing observable, because the already-disposed inner composite immediately disposes anything a later callback adds to it. ### Task Type - [ ] Refactor / Cleanup - [ ] DevOps / Deployment / CI - [x] Testing / QA - [ ] Documentation - [ ] Performance - [ ] Other -- 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]
