aglinxinyuan opened a new pull request, #7692: URL: https://github.com/apache/texera/pull/7692
### What changes were proposed in this PR? `ExecutionReconfigurationService` was at **51.4% of 35 lines**, and the cause was structural: the existing spec's `RecordingService` passed `client = null` / `workflow = null` and overrode all three protected seams, so the uncovered residue was exactly what those overrides skip. In particular, any test that keeps the `dispatch` override leaves line 110 dark however much it asserts. Tests **10 -> 13**. Lines **18/35 (51.4%) -> 35/35 (100%)**, branches 5/6, measured with only this spec running. The spec now builds a real empty-plan `AmberClient` with a mocked `coordinatorInterface` (the pattern at `ExecutionRuntimeServiceSpec.scala:71-95`) and leaves the seams alone, so `modifyOperatorLogic`, the dispatch body, the worker-completion callback and the completion diff handler all execute for real. ### Verification Review proposed nine production mutations. **All nine survived the 10-test suite.** Eight are now killed, each verified red on a named test from the JUnit XML, one mutation at a time with an asserted-empty production diff between runs. The ninth is deliberately left alive, for a reason worth spelling out below. | Surviving mutation | Why nothing noticed | |---|---| | `old.unscheduledReconfigurations :+ op` -> `List(op)` | only one edit was ever made, so append and overwrite were indistinguishable | | the failure arm wiping the queue | no pending work existed when the rejection was tested | | `runtimeReconfiguration(..., currentOp, newOp)` -> `(..., newOp, newOp)` | nothing recorded which descriptor the service treats as "old" | | dropping the `StateTransferFunc` | the queue assertion discarded the tuple's second element | | carrying `completedReconfigurations` across a new round | no stale worker was ever seeded | | deleting the guard's second leg | no test straddled a round boundary | | announce-once-per-operator, and `(new -- old).take(1)` | every diff carried exactly one worker | The fixes strengthen existing tests rather than bolting on new ones where possible: the queue test now makes two edits and checks order, per-slot limit and the state-transfer closure; the rejection test seeds pending work; the dispatch test seeds a stale completed worker. Three tests are new, including two late-subscriber cases that exercise `StateStore`'s catch-up replay pair -- one with a two-worker diff, one straddling a round boundary. ### Two mutations are deliberately left alive **The `&&` -> `||` flip at line 138** is not killable through the service's own API. It only changes the outcome for a transition that atomically retires a round *while* adding a completed worker, and production has exactly three transitions on this store, none of which can produce that state. The single-leg flip of the same condition **is** killed, so the line is pinned; only the connective is not. **Wrapping the discarded `Disposable` at line 117 in `addSubscription`** survives by design. That is the shape of the *production fix* for a leak (see below), and pinning it either way would mean asserting the leak. The suite was instead made agnostic to how it gets resolved. ### Deliberately not included Line 142's `workflow.physicalPlan` dereference is covered but **not pinnable**: `getPhysicalOpByWorkerId` looks the operator up by the name parsed out of the worker id, so every observable value is identical to a plan-free implementation. The only difference is that the real code throws for a worker absent from the plan -- asserting that would cement a crash, not a contract. Two defects are reported rather than pinned. Line 117 discards the `Disposable` returned by the engine callback registration, so the callback outlives the service. And `ReconfigurationHandler.scala:138-139` attaches `notifyOnComplete` per **worker**, while this file's comment describes per-operator announcement -- so an operator with several workers is announced more than once. The announcement test is renamed to the contract-neutral "announce the logical operator of every worker that reports in" and a second worker is added to the fixture, rather than asserting a once-per-operator contract that current production does not honour. No production file is touched. ### Any related issues, documentation, discussions? Closes #7691 ### How was this PR tested? ``` STORAGE_ICEBERG_CATALOG_TYPE=postgres sbt "WorkflowExecutionService/testOnly org.apache.texera.web.service.ExecutionReconfigurationServiceSpec" ``` ``` [info] Total number of tests run: 13 [info] Tests: succeeded 13, failed 0, canceled 0, ignored 0, pending 0 ``` The spec's base becomes `TestKit(ActorSystem(...))` with `AnyFlatSpecLike` so the empty-plan client can be built; the system name is suite-unique and shut down in `afterAll`, as two sibling specs in the same package already do. `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]
