aglinxinyuan opened a new pull request, #7870: URL: https://github.com/apache/texera/pull/7870
### What changes were proposed in this PR? `DPThreadSpec` goes from 8 tests to 16, covering the DP thread's lifecycle edges: stopping before it ever starts, returning from `stop()` only after the thread has left the main loop, parking when idle and exiting quietly when interrupted there, and a pause arriving mid-batch. Measured with one fresh sbt JVM per run, `rm -rf` on the jacoco dir between them, and an identical suite-name filter before and after. | Metric | Before | After | |---|---|---| | Codecov (fully-covered lines) | 65/87 = 74.7% | **74/87 = 85.1%** | | Missed lines | 3 (157, 158, 185) | **0** | | Branch arms | 50 covered / 26 missed | **58 covered / 18 missed** | Adding `WorkerSpec` — the only other in-repo spec that constructs a `DPThread` — gives an identical 74/87, so this is the whole picture from unit tests. Worth naming the classification rule, since it changes the number: Codecov treats a line as covered only when `ci>0 && mb==0`, so partial-arm lines count against you. A stricter rule that also demands `mi==0` gives a different figure for this file, because four lines have missed *instructions* but no missed *branches*. All figures above use Codecov's rule. ### A production defect, found by a mutation that cannot be killed Replacing `dp.inputGateway.tryPickControlChannel` with `None` at line 185 **survives all 16 tests**, and no test can kill it: the value is dead. This is a `match`-precedence defect at lines 182-193 — re-verified independently in bytecode rather than taken on trust. It is therefore an *equivalent* mutant, meaning unkillable by any test whatsoever, not merely unkilled by these. That is reported rather than pinned, and it is the one survivor in the table. ### Verification 13 production mutations, **12 killed, 1 equivalent survivor.** Eleven of the twelve kills are attributable to exactly one named test. Every mutant was applied from and reverted to a pristine snapshot, with an exactly-once anchor assertion, a "replacement count grew by exactly one" assertion, and a pristine-hash plus empty-production-diff check immediately *before* each compile and again after each revert — the harness aborts on any of those, so no mutant was ever live during another's run. ### Corrections made during review, including to the reviewers The first draft reported no survivors on a narrow table. Adversarial review produced 8 findings; several were right, and three were wrong in ways worth recording: - **A reviewer's suggested fix would not have killed that reviewer's own mutant.** Their finding proposed adding `assert(delegates.isEmpty)` to the stop-flag test. It was implemented verbatim and measured: the mutant left that test green, because of where its execution actually diverges. Replaced with a gate that does discriminate. - **A reviewer's suggested mechanism was unsound here** — locating the DP thread by the name `"DP-thread"`. Five tests in this spec never stop their thread, so several live threads share that name and a lookup over an unordered key set could sample the wrong one. - **A reviewer overstated one relocation** as establishing "the causal claim". It remains unfalsifiable by any production mutation, because with a one-tuple frame the relevant predicate is false throughout the drain. Corrections to my own first draft: one mutation's kill was credited to a single test but actually fails two, both legitimately; another's kill was credited to an *unsynchronized* read of a plain `var` written by the DP thread, and is now credited to a properly synchronized gate; a third's failure mode moved to an earlier gate after a relocation. The diff is also larger than first stated — +394/-3 on one file, not +188/-1. One projection was downgraded rather than restated: the first draft called a CI figure "provably" bounded. It rests on assumptions about which arms the e2e specs reach, and those specs need an Iceberg REST catalog that is not running here, so "argued" is the honest word. ### Deliberately not included 74/87 is the hard ceiling without a production change. The 13 remaining partial lines are the lazy-val bitmap, four scala-logging `isXEnabled` guards, `$outer == null` guards, a `MatchError` arm of a sealed trait with three final case classes, `MatchError` arms of exhaustive `Option` matches, and the null-equals-null leg of a state comparison whose operand is never null. Lines 204-223 — the `withFaultTolerant` payload-dispatch block — and line 201's filter lambda are outside the 87-line denominator entirely: they compile to `ACC_SYNTHETIC` `$anonfun` methods that JaCoCo drops, verified in the tracked line list jumping 203 → 224. Tests aimed there earn behaviour but no lines. Scope: `DPThreadSpec` and `WorkerSpec` were run, not the whole module, so the module's overall baseline was not re-established. `DPThreadSpec` is untagged and runs in the CI amber job. No production file is touched — sha256 of `DPThread.scala` is byte-identical to the pre-mutation snapshot. ### Any related issues, documentation, discussions? Closes #7867 ### How was this PR tested? ``` sbt "WorkflowExecutionService/testOnly org.apache.texera.amber.engine.architecture.worker.DPThreadSpec" ``` ``` [info] Total number of tests run: 16 [info] Tests: succeeded 16, failed 0, canceled 0, ignored 0, pending 0 ``` The four new tests add roughly 4-5s of wall clock. `Test/scalafmtCheck` passes. ### 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]
