aglinxinyuan opened a new issue, #7953: URL: https://github.com/apache/texera/issues/7953
### Task Summary Two amber files have **no spec at all**, which is unusual at this point: | File | Coverage | |---|---| | `amber/.../engine/common/client/AmberClient.scala` | 83.0% — 4 missed + 4 partial of 47 lines | | `amber/.../engine/common/rpc/AsyncRPCServer.scala` | 75.0% — 3 missed + 5 partial of 32 lines | `AmberClient`'s four missed lines are the whole of `notifyNodeFailure` plus the not-active guard in `registerCallback`, and `ClusterListenerSpec`'s own header states in writing that nothing exercises `notifyNodeFailure` anywhere in the suite. **Weigh one prior finding first.** A sibling file, `ClientActor.scala`, was assessed earlier and turned out **saturated rather than undertested** — roughly 190 lines of new test bought 1 line, because the e2e specs (`DataProcessingSpec`, `PauseSpec`) are *not* tagged `@IntegrationTest` and so run in the coverage job, already driving a real client end to end. That is why a scoped local measurement of these files reads far lower than Codecov: quote the CI-visible gap, not the scoped one. Traps, each of which would otherwise produce a false pass: 1. **`AmberClient`'s constructor is not side-effect-free.** It calls `system.actorOf(Props(new ClientActor))` and blocks on an ask whose handler spawns a real `Coordinator` child. Safe only with `PhysicalPlan(Set.empty, Set.empty)` and an all-`None` `CoordinatorConfig`; every client must be `shutdown()` in a `finally`, or the shared serialized amber JVM accumulates live actors. 2. **`AsyncRPCServer.methodsByName` is a memoized `@transient lazy val`** built from `getClass.getMethods` on the *first* `receive()`. Assigning `server.handler` after any `receive()` is silently ignored, so a server shared across tests yields a false pass. Use a fresh one per test. 3. **The ask inside `notifyNodeFailure` uses a 1-minute implicit timeout.** If the actor never replies the test stalls a full minute and the sbt log then says "1 TEST FAILED" without naming it. Await with an explicit short bound. 4. **`registerCallback` asserts `clientActor.path.address.hasLocalScope`**, so the spec needs a plain local `ActorSystem` — not the clustered `AmberRuntime.pekkoConfig`. 5. **A logback level change is JVM-global by logger name.** `AmberLogging` names loggers from the actor id, so a distinctive id isolates it — but restore the level in a `finally`, since amber suites are strictly serial and a leak would deterministically pollute later suites. 6. **The registered handler class must be genuinely public.** If it is emitted package-private, `method.invoke` throws `IllegalAccessException` for every dispatch, routing the happy path into the same catch as the failure test and making both vacuous. Assert a successful dispatch first. Eight of the sixteen gap lines are structurally unwinnable — two lazy-val bitmaps, a `MatchError` arm, an `$outer` null guard, an `instanceof Object` arm, and three scala-logging `isEnabled` arms — so do not promise more than about 8. ### Task Type - [ ] Refactor / Cleanup - [ ] DevOps / Deployment / CI - [x] Testing / QA - [ ] Documentation - [ ] Performance - [ ] Other -- 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]
