aglinxinyuan opened a new pull request, #7678:
URL: https://github.com/apache/texera/pull/7678
### What changes were proposed in this PR?
`WorkflowExecutionsResource` sat at **83.5% of 316 lines** behind a 44-test
spec, with the residue concentrated in four endpoints that need nothing but the
embedded test DB.
Tests **44 -> 64**. Lines **264/316 (83.5%) -> 298/316 (94.3%)**, measured
with only this spec running so the figure is attributable to it alone.
| Endpoint | What is now pinned |
|---|---|
| `getWorkflowResultDownloadability` | the `is_downloadable` predicate
itself, the label format, the set union when two restricted scans meet
downstream, and BFS termination on a cyclic graph |
| `exportResultToLocal` | the id plumbing through the zip branch over a
seeded execution, both allowed roles and two rejected ones, and the `@Consumes`
+ `@FormParam` contract against the form the frontend actually posts |
| `retrieveInteractionHistory` | the replay destination ids themselves, read
back from a real two-record log; plus NULL and empty `log_location` |
| `retrieveWorkflowRuntimeStatistics` | the warehouse guard's exact message,
read access, and an execution id belonging to a different workflow |
### Verification — and a correction worth reading
The first pass applied 11 mutations and reported all 11 killed, zero
survivors. **That number was misleading.** Four independent reviewers were then
asked to *refute* the suite rather than confirm it, and between them proposed
19 distinct mutations. Every one of the 19 was run against the original suite
first: **all 19 survived 54/54.** None of the findings could be dismissed on
survival.
All 19 are now killed, each verified red on its intended test by name (read
from the JUnit XML, since the console log does not name tests), one mutation at
a time with a checkout revert and an asserted-empty production diff between
every run.
The lesson generalises beyond this file: *a mutation set written by the same
author as the tests tends to probe the lines the tests were built around.*
Examples of what survived:
| Surviving mutation | Why the suite did not notice |
|---|---|
| `WarehouseReadGuard.assertReadable(uri)` -> unconditional throw | the only
assertion was `contains("byo")`, and `byo` came from the test's own URI |
| delete `.and(DATASET.IS_DOWNLOADABLE.eq(false))` | every dataset the spec
seeded was already non-downloadable |
| ignore the form body entirely, hard-coding the parsed request | no seeded
execution, so the ids were unobservable |
| collapse the whole replay-log branch to `List()` | three tests asserted
only `isEmpty` |
| `if (merged != existing)` -> `if (true)` | every fixture graph was acyclic
|
| drop `ADMIN` from the allowed roles | only `REGULAR` was ever exercised |
### Deliberately not included
The Iceberg record mapping and the `WorkflowRuntimeStatistics` constructor
it solely feeds (needs a live Iceberg table). One line is executed but
uncreditable: the call on it throws, so JaCoCo's probe never fires.
Two defects are reported rather than pinned, so neither is cemented as a
contract:
- `removeAllExecutionFiles` (`:386`) runs its `filterNot` **outside** the
per-URI `try` and **after** the DB delete, so one undecodable stored URI
deletes the rows and then 500s the caller — contradicting the "best-effort
cleanup" comment. This is why no `mock:///` cleanup test appears here.
- `exportResultToDataset`'s catch body (`:859-864`) is unreachable: the
service catches per operator and nothing after it can throw.
Three reviewer suggestions were deliberately not taken, each explained in
the spec: a "legal `vfs://` URI passes the guard" companion would reach
`IcebergCatalogInstance` against a real Postgres rather than the embedded DB,
and two mutations on `exportResultToDataset` need a live Iceberg document to
observe.
No production file is touched.
### Any related issues, documentation, discussions?
Closes #7677
### How was this PR tested?
```
STORAGE_ICEBERG_CATALOG_TYPE=postgres sbt "WorkflowExecutionService/testOnly
org.apache.texera.web.resource.dashboard.user.workflow.WorkflowExecutionsResourceSpec"
```
```
[info] Total number of tests run: 64
[info] Tests: succeeded 64, failed 0, canceled 0, ignored 0, pending 0
[info] All tests passed.
```
The one case that round-trips real replay records needs
`AmberRuntime.serde`, injected by the same reflection pattern
`ReplayLogGeneratorSpec` uses, but narrowed twice because amber runs every
suite in one JVM: it is scoped to that single case rather than `beforeAll`, and
a runtime another suite already initialised is reused as-is rather than swapped
out underneath it. Both fields are restored and the system it creates is shut
down.
`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]
