aglinxinyuan opened a new pull request, #7554: URL: https://github.com/apache/texera/pull/7554
### What changes were proposed in this PR? `SyncExecutionResource` was the largest uncovered file in the repository — 406 instrumented lines, no spec, **0%**. It is the endpoint an external caller uses to run a workflow and get results back in one request, so its result assembly, error classification and cell-truncation rules are all user-visible. Adds 23 tests, taking the file to **56.7% of lines** (230/406). Nothing here needs infrastructure: the class is a zero-arg Jersey resource that constructs with no fixture, and MockTexeraDB plus a single `workflow_computing_unit` row is enough to drive the public endpoint end to end — `initExecutionService` absorbs the missing engine into a FAILED state, after which the whole result-assembly tail runs. Covered: the state mapping and terminal-state predicate, console error detection, symmetric cell truncation and tuple-size estimation, error classification, sub-DAG computation, the 100-line operator-info aggregator, console-log retrieval, and `executeWorkflowSync` itself. ### Verification 52 mutations applied one at a time and reverted, with the production file's md5 compared against its pre-mutation value after each revert. Production diff empty. Highlights: swapping input for output metrics in the aggregator, taking the console title instead of the longer message as the error, walking the sub-DAG by the wrong link end, dropping the visited-set early return, promoting a mid-line "WARNING:" mention to a real warning, and reporting KILLED as FAILED. **Two assertions were found vacuous in review; one was fixed and one is reported as unpinnable.** *Fixed* — the in-memory console fallback's `.filter(_.nonEmpty)` could be dropped and nothing noticed, because no fixture had an operator that was *present* in `operatorConsole` while carrying zero messages. That state is real: `ExecutionConsoleService` creates exactly that shape via `getOrElse(opId, OperatorConsole())` before it has anything to add. Added a `"silent"` operator and asserted `consoleLogs` is `None` rather than `Some(Nil)` — the frontend renders a console pane for `Some`, so `Some(Nil)` is an empty pane. That mutation is now red. *Reported, not papered over* — inverting the console-error arm at line 328 leaves the suite green. The one test that exercises `stateString` runs a path where `terminatedByConsoleError` is false **and** `stateToString(finalState.state)` is also `"Failed"`, so the assertion cannot tell the two arms apart. Distinguishing them needs a run whose final state is not FAILED, which is impossible without a live engine — with no coordinator, `initExecutionService` always stamps FAILED. This is stated in the spec at that test rather than left for the next reader to discover. One further mutation survived and is an **equivalent mutant** rather than a gap: `truncateSingleTuple`'s `text.length > maxCellChars` flipped to `>=`. The truncation it guards re-checks the same bound and returns the cell unchanged, so no input can distinguish the two spellings. The test was kept (the behaviour is real) and its comment corrected to say why the boundary is unobservable. ### Deliberately not included - **~139 lines** behind real Iceberg result storage (`collectOperatorResult`, the symmetric-truncation engine). `DocumentFactory.openDocument` is an un-displaceable static for `vfs://` URIs. `common/workflow-core`'s `LocalHadoopIcebergCatalog` would unlock it, but amber declares only `DAO % "test->test", Auth % "test->test"` (build.sbt:270), so it is not on amber's test classpath. Adding `WorkflowCore % "test->test"` would take this file to roughly 87% — worth doing, but as its own change. - **The `Observable.amb` wait and its timeout/termination arms**, which need an execution that is still non-terminal, i.e. a live engine. - **`validateWorkflow` (905–924)** — it has **zero call sites** repo-wide. It can be driven reflectively, which is not the same as being live; testing it would cement dead code. Delete instead. - **The `if (executionService == null)` early return**, effectively unreachable: `initExecutionService` publishes before calling `executeWorkflow()`, and the constructor is documented and verified side-effect-free. ### Known follow-up Most helpers are reached through `PrivateMethodTester` because 14 of the 16 methods are `private def` (there is repo precedent — eight existing specs use it). One case was avoidable: `handleExecutionError` is reachable from the public endpoint by passing a wid with no `workflow` row, which would additionally cover lines 343–346. Left as noted rather than restructured here. Widening the nine pure helpers to `private[resource]` would remove nearly all the reflection for a one-word change each. No production file is touched. ### Any related issues, documentation, discussions? Closes #7553 ### How was this PR tested? ``` STORAGE_ICEBERG_CATALOG_TYPE=postgres sbt "WorkflowExecutionService/testOnly org.apache.texera.web.resource.SyncExecutionResourceSpec" ``` ``` [info] Total number of tests run: 23 [info] Tests: succeeded 23, failed 0, canceled 0, ignored 0, pending 0 ``` Coverage measured with sbt-jacoco filtered to this spec (a bare `jacoco` runs amber's `@IntegrationTest` specs, which hang on Windows): 0/406 before, 230/406 after. `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]
