aglinxinyuan opened a new pull request, #7770: URL: https://github.com/apache/texera/pull/7770
### What changes were proposed in this PR? Six small amber files, three of which had no spec at all. 12 new tests across 4 new spec files and 2 extended ones. | File | Before | After | |---|---|---| | `DebugCommandHandler` | 33.3% (1/3) | **100%** (3/3) | | `ConsoleMessageHandler` | 33.3% (1/3) | **100%** (3/3) | | `SystemMetadataResource` | **0%** (0/2) | **100%** (2/2) | | `StartHandler` | 73.9%, 2 missed + 4 partial | **87.0%**, 0 missed | | `PrepareCheckpointHandler` | 61.9%, 2 missed + 6 partial | **71.4%**, 0 missed | | `PekkoActorRefMappingService` | 85.7%, 2 missed + 5 partial | **87.8%**, 0 missed | **This is 12 lines, and I would rather say so than dress it up.** What earns it is that every one is live code, three files go from partly- or wholly-untested to 100%, none of it needs infrastructure, and all 10 mutations die. The `PrepareCheckpointHandler` case also closes the prepare-to-finalize handshake gap that file's own scaladoc flagged as a follow-up — it is now driven against a real `WorkflowWorker` via `TestKit` rather than stopping at the main-thread hand-off. ### Verification 10 mutations, **10 killed, no survivors**. Each was applied alone by a driver that asserts the anchor occurs exactly once in both directions, reverted with `git diff -- '*/src/main/*'` confirmed empty afterwards, and the failing test read by name from `amber/target/test-reports`. | Mutation | Killed by | |---|---| | `DebugCommandHandler` addresses the sender instead of the named worker | addresses the command to the worker the request names, not to the sender | | **exchange** the debug request's two fields | relays the request body verbatim | | **exchange** `source` and `title` on the console message | hands the client the reported message unchanged | | **exchange** the two ends of the client channel in `sendToClient` | puts the message on the client channel and nowhere else | | `SystemMetadataResource` filters out `Filter` operators | hands back an entry for every operator the workspace can place | | `SystemMetadataResource` reverses the group order | hands back the palette's group order | | `StartHandler`'s reader-thread check `nonEmpty` -> `isEmpty` | two tests, incl. refuses a worker that is neither a source nor a reader of materialized input | | `PekkoActorRefMappingService` registers the id before the parent send | swallows a failed parent lookup and still asks again for the same id | | **exchange** the checkpoint's queued-input and output keys | saves the worker's queued input and starts recording what arrives next | | drop the recorded-inputs write | same test | ### Four sibling files were assessed and deliberately left alone Recording the evidence so this is not re-derived: - **`PekkoActorService`** — its two uncovered lines are the entire bodies of `sendToSelfOnce` and `ask`, and both have **zero call sites** anywhere in main, test, bench or scripts. Dead code. (Its other originally-missed lines are in fact already covered.) - **`WorkflowMessage`** — `case _ => 200L` is unreachable: the trait is `sealed` and has a single subtype in the file, so only a `null` argument could reach the default arm. Its other line is a case-class declaration whose 28 uncovered branches are scalac-generated `equals`/`copy`/`productElement`. - **`RecoveryPayload`** — the classes are already constructed by `AmberMessageEnvelopesSpec`; the uncovered remainder is purely scalac-generated members, so nothing hand-written could be deleted to make a test fail. - **`ComputingUnitWorker`** — the uncovered lines are `main`'s body, and `AmberRuntime.startActorWorker` binds a cluster seed and mutates `AmberConfig.masterNodeAddr`, which is global state in amber's shared test JVM. Within the accepted files, the remaining partials are `logger.info`/`logger.debug` macro guards (which do not execute under CI's `TEXERA_SERVICE_LOG_LEVEL=WARN`) and `val (a, b) = ...` tuple-destructuring `MatchError` arms. ### A production fragility, reported and not pinned `PekkoActorRefMappingService.retrieveActorRef`'s catch block builds its warning as `s"... parentRef = " + actorService.parent` — it re-reads the very thing whose failure it is handling. A parent lookup that fails *persistently* rather than transiently therefore throws out of the handler that exists to contain it. The new test injects exactly one failure for that reason, and says so in its scaladoc, so a fix is not blocked. No production file is touched. ### Any related issues, documentation, discussions? Closes #7769 ### How was this PR tested? ``` STORAGE_ICEBERG_CATALOG_TYPE=postgres sbt "WorkflowExecutionService/testOnly org.apache.texera.amber.engine.architecture.coordinator.promisehandlers.DebugCommandHandlerSpec org.apache.texera.amber.engine.architecture.coordinator.promisehandlers.ConsoleMessageHandlerSpec org.apache.texera.amber.engine.architecture.worker.promisehandlers.StartHandlerSpec org.apache.texera.web.resource.SystemMetadataResourceSpec org.apache.texera.amber.engine.architecture.common.PekkoActorRefMappingServiceSpec org.apache.texera.amber.engine.architecture.worker.promisehandlers.PrepareCheckpointHandlerSpec" ``` ``` [info] Total number of tests run: 20 [info] Tests: succeeded 20, failed 0, canceled 0, ignored 0, pending 0 ``` The amber unit suite goes from 1919 to 1931 tests. `Test/scalafmtCheck` and `Test/scalafix --check` both pass. ### 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]
