aglinxinyuan opened a new pull request, #7954: URL: https://github.com/apache/texera/pull/7954
### What changes were proposed in this PR? Two new specs for two files that had none: `AmberClientSpec` and `AsyncRPCServerSpec`. 13 tests. Measured with `WorkflowExecutionService/jacoco` scoped to these two suites, one fresh sbt JVM per run. | File | Codecov (scoped) | JaCoCo line-hit | |---|---|---| | `AsyncRPCServer.scala` | 21/32 = 65.6% → **27/32 = 84.4%** | 28/32 → **32/32 = 100%** | | `AmberClient.scala` | 21/47 = 44.7% → **40/47 = 85.1%** | 22/47 → **43/47 = 91.5%** | **Those scoped figures overstate what CI will show, and the difference is the whole story of this file.** Codecov reports `AmberClient` at 83.0%, not the 44.7% a scoped run sees, because the e2e specs (`DataProcessingSpec`, `PauseSpec`) are **not** tagged `@IntegrationTest` and therefore run in the coverage job, already driving a real `AmberClient`. So the CI-visible gain is bounded by the 8 lines Codecov actually reports as missing — not +25. I am stating it that way because a sibling file, `ClientActor.scala`, was assessed earlier in exactly this trap and turned out **saturated rather than undertested**: ~190 lines of new test bought 1 line. These two files are not that — `notifyNodeFailure` is genuinely unexercised, and `ClusterListenerSpec`'s own header says so in writing — but the scoped number is not the number to quote. ### Verification 17 mutations, **all 17 killed, no survivors.** Each applied one at a time against a hash-verified pristine tree. The first draft claimed no survivors on a 10-mutant table; **six further mutants survived it**, and all six now die. Four of its claims were overstated and are corrected here: - A dispatch test was described as pinning that the handler saw "the request and the context — not the two swapped". It did not. - One kill was credited to a `notifyNodeFailure` test, but it only pinned *which branch ran*: `ClientActor` replies `Ack` to any message, so the assertion could not distinguish the call from a no-op. - The error-handler test was described as pinning the `catch` at line 144. It pins only that `errorHandler` is *called*; the catch's other job — swallowing so the stream survives — was unasserted. ### Traps recorded in the specs, because each would produce a false pass - **`AmberClient`'s constructor is not side-effect-free.** It does `system.actorOf(Props(new ClientActor))` and then blocks on an ask whose handler spawns a real `Coordinator` child. It is safe only with an empty `PhysicalPlan` and an all-`None` `CoordinatorConfig`, and every client must be `shutdown()` in a `finally` or the shared serialized amber JVM accumulates live actors. - **`AsyncRPCServer.methodsByName` is a memoized `@transient lazy val`** built from `getClass.getMethods` on the *first* `receive()`. Reassigning `server.handler` afterwards is silently ignored, so a shared server across tests would produce a false pass. Every test builds a fresh one. - **The ask inside `notifyNodeFailure` is governed by a 1-minute implicit timeout.** A non-replying actor would stall a full minute and then report "1 TEST FAILED" without naming it, so the spec awaits with an explicit 5-second bound instead. - **`registerCallback` asserts `clientActor.path.address.hasLocalScope`**, so the spec's `ActorSystem` must be plain local — not `AmberRuntime.pekkoConfig`, which is clustered. - The debug-level test restores the previous logback level in a `finally`. amber suites are strictly serial, so a leaked level would deterministically pollute later suites rather than occasionally. ### Deliberately not included Eight of the sixteen gap lines are structurally unwinnable: both lazy-val bitmaps, a `MatchError` arm, an `$outer` null guard, an `instanceof Object` arm, and three scala-logging `isEnabled` arms. `AsyncRPCServer:104`'s error-disabled arm has no test-only seam — `invokeMethod`, `returnResult` and `noReplyNeeded` are all private, and everything here is driven through the public `receive` plus the `handler` var. Accepted limitation; no production seam was requested. Both new specs carry the Apache licence header. No production file is touched. ### Any related issues, documentation, discussions? Closes #ISSUE_Y1 ### How was this PR tested? ``` sbt "WorkflowExecutionService/testOnly org.apache.texera.amber.engine.common.client.AmberClientSpec org.apache.texera.amber.engine.common.rpc.AsyncRPCServerSpec" ``` ``` [info] Total number of tests run: 13 [info] Tests: succeeded 13, failed 0, canceled 0, ignored 0, pending 0 ``` `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]
