aglinxinyuan opened a new pull request, #8035: URL: https://github.com/apache/texera/pull/8035
### What changes were proposed in this PR? `WorkflowExecutionServiceSpec` goes from 3 tests to 11. The three existing tests stop at construction; the new ones drive the metadata-store handler and the teardown path. | Metric | Before | After | |---|---|---| | **Codecov (fully-covered lines)** | 26/72 = 36.1% | **36/72 = 50.0%** | | JaCoCo line-hit | 30/72 | 39/72 | | Branch arms | 5/18 | 10/18 | **+10 fully-covered lines and +5 branch arms.** The two metrics differ by one because line 179 was already line-hit and flips only by completing its second arm — they are not interchangeable, so both are given. **Of the +10, six are logic and four are not.** Lines 95, 179, 181–184 are the recovery-banner arm and the four `unsubscribeAll` calls. Lines 101, 107, 108 and 109 are scalac-generated public accessor pairs (confirmed with `javap -p -l`) that move because the teardown test assigns those vars from outside. Those four are a real consequence of driving teardown rather than coverage farming, but they are not logic and I would rather split them out than present all ten as equivalent. ### What the reviewers found Two independent adversarial reviewers ran against the first draft. **Six of eight fresh mutants survived it.** The three worth naming: - **`workflowContext.workflowSettings = request.workflowSettings` (line 90) could be deleted outright and all 8 tests still passed.** Every test executes that line, so it was fully covered and entirely unconstrained. It is not cosmetic: `outputPortsNeedingStorage` is what `CostBasedScheduleGenerator` reads to decide which output ports get materialized, and `dataTransferBatchSize` / `executionMode` feed the resource allocator. Dropping it silently reverts every execution to the default settings while the request's are ignored. - **The recovering-state guard was pinned one-sidedly.** The test only ever drove `isRecovering` false → true, so weakening `newState.isRecovering != oldState.isRecovering` to a bare `newState.isRecovering` survived. That mutant is the mirror of the failure the test's own comment claimed to guard: an update that *clears* the flag emits no event, so the frontend's Recovering banner never comes down. - **Both the state guard and the `fatalErrors` guard could be deleted wholesale** and nothing failed. All are now killed by a named test. The published mutation table was also re-run from scratch, one mutant at a time, because one row's failure message had been copy-pasted from another row. ### Verification Measured with an identical suite-name filter on both sides, one fresh sbt JVM per measurement, `rm -rf` of the jacoco dir between runs, counters read per-line out of `jacoco.xml`. An independent measurer re-derived the figure a second way — over the **whole amber unit module**, the actual CI scope, rather than the six-suite filter — and got byte-identical per-line data on both sides. That independently confirms the six-suite list is complete and that 26 → 36 is what Codecov will show. Two corrections that measurement forced, both worth stating: - The repair round added 3 tests **after** the original measurement, and the expectation was that the figure would rise above 36. It did not. Those 3 tests bought **zero** additional fully-covered lines — they add branch arms on line 94 and mutation-kill strength only. 36 is the number. - Line 185 already counted as a Codecov hit before this PR despite `mi=4, ci=1`, because the method's `return` instruction is attributed to it. It is genuinely executed only now. Full amber unit scope, both sides: **190 suites, the same 7 pre-existing Windows-only failures by name, zero new.** The only per-suite change anywhere is this spec going 3 → 11 tests, so the new `beforeAll` inserts do not leak — `MockTexeraDB` gives each suite its own database, and ids 9207–9210 are unique across the repo because the Iceberg statistics URI is machine-global. ### Deliberately not included `executeWorkflow`'s live-runtime half — the 44-line hole — has no seam. Line 124 calls `ComputingUnitMaster.createAmberRuntime`, which builds an `AmberClient` over `AmberRuntime.actorSystem`, a JVM-global `private var` that nothing in unit scope initialises. Injecting a seam would be a production change. Lines 113/114/115 are refused for a sharper reason: JaCoCo probes the `try` block only at its exit, so they flip only if compile *and* `Workflow.fromCompilationResult` both succeed — which falls straight into `createAmberRuntime`. Whether that is survivable depends on `ClientEventSpec` having restored the global to `null` in its `afterAll`. `WorkflowServiceSpec`'s own header documents this cross-suite hazard as its reason for deliberately steering into a compile failure instead. Cementing an accident is worse than leaving three lines. Three mutants are reported as live rather than dropped: reordering `client.shutdown()` against the four `unsubscribeAll` calls (order is not a stated contract), and two whose only kill would be to assert current behaviour that is arguably wrong — adding `FAILED` to the stuck-banner guard, and pinning which field the teardown guard reads, where the only discriminating state currently NPEs. No production file is touched. ### Any related issues, documentation, discussions? Closes #8033 ### How was this PR tested? ``` sbt "WorkflowExecutionService/testOnly org.apache.texera.web.service.WorkflowExecutionServiceSpec" ``` ``` [info] Tests: succeeded 11, failed 0, canceled 0, ignored 0, pending 0 [info] All tests passed. ``` `WorkflowExecutionService/Test/scalafmtCheck` and `WorkflowExecutionService/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]
