The GitHub Actions job "Required Checks" on texera.git/main has failed. Run started by GitHub user github-merge-queue[bot] (triggered by github-merge-queue[bot]).
Head commit for run: 5fc901727c4582778c14035bfe6e4be6ccd02770 / Xinyuan Lin <[email protected]> test(amber): cover the amber client and the async RPC server (#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 #7953 ### 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) --------- Signed-off-by: Xinyuan Lin <[email protected]> Co-authored-by: Copilot Autofix powered by AI <[email protected]> Report URL: https://github.com/apache/texera/actions/runs/32903123081 With regards, GitHub Actions via GitBox
