aglinxinyuan opened a new pull request, #8177: URL: https://github.com/apache/texera/pull/8177
### What changes were proposed in this PR? Two new specs for classes that had none, plus one append. 33 tests across the three suites. | File | Codecov lines | Newly covered | |---|---|---| | `AsyncReplayLogWriter.scala` | 38/44 → **42/44** | 51, 58, 71, 72 | | `CongestionControl.scala` | 30/34 → **32/34** | 70, 71 | | `ReplayLogManager.scala` | 30/34 → **31/34** | 122 | **+7 fully-covered lines** (120/137 → 127/137 across the bundle). The plain line-hit metric moves only +3, because four of the seven were already executed and flip by completing a branch arm. New files: `AsyncReplayLogWriterSpec` (7 tests), `ReplayLogManagerImplSpec` (6 tests), and a shared `ReplayLogWriterFixtures` support object. `AsyncReplayLogWriter` and `ReplayLogManagerImpl` had no spec of their own before this. The lines closed are the terminated-writer arms of `assert(!stopped)` in `putLogRecords` and `putOutput`, the non-default `logInterval > 0` sleep arm, the `ssThreshold` clamp reached only after five consecutive ack timeouts, and `markAsReplayDestination`. ### Honesty about the bound The measurement filter was 110 suite FQCNs — every spec under `amber/src/test/scala/.../engine/**` plus `clustering/**`. CI's full-module `WorkflowExecutionService/jacoco` runs 188. **Adding suites can only raise the before-figure, so +7 is an upper bound on the CI gain, not a floor.** The specific lines closed are not plausibly reached by a `core/` operator, tuple or storage spec, so I believe +7 is also the actual figure — but a bound is what was measured and that is what is claimed. ### Four files from the original scope contribute nothing `AmberFIFOChannel`, `WorkflowMessage`, `RecoveryPayload` and `RegionPlan` were in the bundle and are worth **zero**; they already have thorough specs, including `AmberMessageEnvelopesSpec`, whose name mirrors neither source class it covers. **`NetworkOutputGateway`'s tests were written and then deleted.** It measured 22/25 before and after — lines 50, 51, 98 and 99 are already covered by pre-existing suites at this scope. A third jacoco run on the trimmed tree reproduces 127/137 exactly, confirming those tests contributed nothing. `NetworkOutputGatewaySpec.scala` is byte-identical to `HEAD` in this PR. ### Two hazards handled rather than discovered late **`terminate()` can hang the build.** It does `stopped = true; writerQueue.put(TerminateSignal); gracefullyStopped.get()` — an untimed `CompletableFuture.get()` completed only at the end of `run()`. On a writer that was never started it blocks forever, and amber's suites are strictly serial in one JVM, so that hangs the module rather than failing a test. Every test starts before terminating, with `terminate()` in a `finally`; the class extends `Thread`, so a missed one would also leak a live thread into the shared JVM. **`logInterval` is a JVM-global.** It comes from `ApplicationConfig.faultToleranceLogFlushIntervalInMs`, a memoized `val` that is 0 by default. Poking that object would poison every later suite. Instead the tests reflect on the **instance** field — `javap` confirms `private final long logInterval`, and setting it via `Field.setLong` on the instance was verified to work on this JDK. Instance-local, poisons nothing. ### Deliberately not included `AmberFIFOChannel` lines 47/48/51 are `logger.debug` bodies. There is no `logback-test.xml`, ROOT governs, and CI sets `TEXERA_SERVICE_LOG_LEVEL=WARN`, so the scala-logging `isDebugEnabled` arm never fires in CI — they would look green locally and be missed in CI. Not counted, and not chased by raising the log level in a test, which would mutate a shared logging context in a serial JVM. `NetworkOutputGateway:41` is the `AmberLogging` lazy `logger` accessor, and the class body contains no logging call at all, so there is no call site to reach it from. Covering it needs a production seam — refused. ### Verification All mutations in the final table die, including the four the reviewers measured as surviving the first draft (complete-before-close, flush-after-release, drop-`Left`s, and the `INIT_STEP` mutant), re-verified on the final tree. Four `NetworkOutputGateway` mutants are recorded as killed by **pre-existing** suites without help from this bundle, rather than credited to it. Measured with one fresh sbt JVM per side, the jacoco directory removed between runs, an identical throwaway suite-name filter both times (deleted afterwards), and `STORAGE_ICEBERG_CATALOG_TYPE=postgres` with per-worktree namespaces — which takes the local baseline from 28 failures and 1 abort to **zero**, so neither run is a partial or all-zero report. No production file is touched. ### Any related issues, documentation, discussions? Closes #8175 ### How was this PR tested? ``` sbt "WorkflowExecutionService/testOnly org.apache.texera.amber.engine.architecture.logreplay.AsyncReplayLogWriterSpec org.apache.texera.amber.engine.architecture.logreplay.ReplayLogManagerImplSpec org.apache.texera.amber.engine.architecture.messaginglayer.CongestionControlSpec" ``` ``` [info] Total number of tests run: 33 [info] Tests: succeeded 33, failed 0, canceled 0, ignored 0, pending 0 [info] All tests passed. ``` `WorkflowExecutionService/Test/scalafmtCheck` and `WorkflowExecutionService/Test/scalafix --check` both pass. Re-run after rebasing onto current `main`. ### 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]
