aglinxinyuan opened a new pull request, #7847:
URL: https://github.com/apache/texera/pull/7847
### What changes were proposed in this PR?
A new spec for `PveWebsocketResource.scala`, which had none, plus one test
for `PveManager.deletePackages`' missing-interpreter guard. 43 → 47 tests.
| File | Before | After |
|---|---|---|
| `PveWebsocketResource.scala` | 0/9 lines (0%) | **9/9 (100%)** |
**That 9-line figure is the only gain this PR can measure locally, and the
PveManager half is CI-only.** Measured before *and* after, `PveManager.scala`
is bit-for-bit unchanged locally: 170/228 lines, 154/228 branch-complete,
40/140 branches. The reason is that local Windows coverage of that file is the
mirror image of CI's — the interpreter is never found here, so
`PveResourceSpec`'s 6 pre-existing failures already take both guards' true arms
incidentally. On Linux those are precisely the missed arms. On CI the bundle
adds 4 fully-covered lines (453, 454, 525, 527) and completes 2 branch arms
(452, 524); since Codecov counts a line with any missed arm as missed, those
two arms are themselves 2 more fully-covered lines, so **15 lines on Codecov's
metric, 13 of which go missed → covered**.
Line 526 stays partial permanently: `logger.error(msg)` expands to
LazyLogging's one-sided `if (isErrorEnabled)`.
Attribution was measured per spec with suite-name filters, not inferred.
`PveWebsocketResourceSpec` owns all 9 lines of the websocket file *and*
`PveManager` 453/454 with arm 452; the `deletePackages` test owns
521/522/525/527 with arm 524. The two owned sets do not overlap.
### What adversarial review changed
The first draft reported 8 mutations and 0 survivors. Re-derived
independently, it was 14 mutations with **4 survivors**, and two of the four
were real defects rather than equivalent mutants:
- **Setting the pump's done flag to `false` passed every test.** That is not
equivalent: with the flag never set, the pump re-enters `queue.take()` after
`session.close()` and parks forever, because nothing further is ever enqueued —
leaking one global-EC thread per websocket connection. The blind spot was the
fixture, which stubbed `isOpen` as always-true and `close()` as a counter, so
neither termination condition was observable. Now the fixture counts guard
evaluations: the guard reads the session exactly n times for an n-line
sentinel-terminated stream, because the (n+1)th pass short-circuits on the
flag, so a third read can only happen under the mutant. All three tests now
kill it.
- **The catch arm's `[ERR] ${e.getMessage}` could be replaced by a constant
and survive**, and the `finally` sentinel was only ever exercised on the happy
path. A new test passes an empty `packages` list, so `.get(0)` throws inside
the `Future` — before `PveManager` is entered at all, needing no fake runner.
It pins the catch arm *and* makes "the client always gets a sentinel even when
the action blows up" a proven claim rather than a happy-path inference.
One new test was **dropped as redundant**, proven by measurement rather than
argument: `PveWebsocketResourceSpec` alone already covers the whole line set
the dropped `installUserPackages` test owned, and under the mutation that
deletes the guard's `return`, `PveResourceSpec`'s failure count *drops* from 6
to 4 — it contributes nothing to that kill.
A fixture literal was also deleted: `"packages" ->
["colorama==0.4.6","idna"]` is unobservable, since the guard returns before the
list is read. Two version-pinned strings implying a contract that no test
constrains.
### Verification
14 mutations, **10 killed, 4 survivors**, every survivor listed. The two
repaired survivors were each confirmed *surviving on the unrepaired bundle
first*, then confirmed killed after — not assumed.
| Mutation | Killed by |
|---|---|
| pump's `done = true` → `done = false` | all 3 websocket tests, on the
guard-evaluation assertion |
| catch arm message → a constant | reports the action's own exception and
still sends the sentinel |
| `finally` sentinel `__DONE__` → `__FINISHED__` | all 3 websocket tests,
including the new failure-path one |
| `if (line == "__DONE__")` → `!=` | all 3 websocket tests |
| unknown-action message → a constant | reports an unknown action and closes
on the sentinel |
| pveName argument → a literal | routes action=install with the cuid and
pveName from the handshake |
| `cuid` parse → `0` | same test, distinct assertion |
| `deletePackages` guard message → a constant | reports a missing
interpreter and stops before the system-package check |
| `deletePackages` guard `return List(msg)` → empty list | same test, plus
an existing `PveResource` case |
| `installUserPackages` guard `return` deleted | the websocket install test
**alone** |
**The four survivors, stated plainly.** All four are mutant-surviving gaps,
not merely uncovered paths:
- `while (!done && session.isOpen)` → `while (!done)` survives: every
session in this spec is open for the whole exchange, so that conjunct never
decides anything. The pump's guard is pinned at **entry only**. Killing it
needs a closed-session test whose only available assertion is "nothing was
sent", which against a `Future` means a sleep with no event to wait on. (The
new guard-evaluation counter does not accidentally cover it — under this mutant
`isOpen` is never called at all.)
- The missing-`action` default can be replaced with garbage and survive: all
three tests supply `action` explicitly. Exercising the default means driving
`action=create`, which goes straight to the real process runner — a real venv
plus `pip install -r amber/requirements.txt`.
- Replacing the parsed package list with `Nil`, and inverting
`filter(_.nonEmpty)`, both survive — the whole 8-line parse chain is
unobservable behind the guard's early return.
### On the file reaching 100%
Worth qualifying rather than leaving as a headline. Those 9 tracked lines
are the straight-line handshake prologue. JaCoCo emits **no BRANCH counter
element** for this file: the class element contains exactly `onOpen` and
`<init>`, with no `$anonfun$onOpen$*` lambdas, because every conditional sits
inside a `Future { … }` body that the `SyntheticFilter` drops. Those
conditionals are asserted but not counted, and only 2 of the 4 things `action`
can dispatch to are asserted at all.
### Reported, not pinned
`onOpen` parses `cuid` **outside** the `Future`, so a handshake missing
`cuid` or `pveName` throws `NumberFormatException`/NPE straight out of `onOpen`
and the client is told nothing — the catch arm inside the `Future` cannot see
it. The new failure-path test deliberately keeps `cuid` valid and throws inside
the `Future`, so it pins the catch arm without cementing that behaviour.
No production file is touched.
### Any related issues, documentation, discussions?
Closes #7846
### How was this PR tested?
```
sbt "WorkflowExecutionService/testOnly
org.apache.texera.web.resource.pythonvirtualenvironment.PveResourceSpec
org.apache.texera.web.resource.pythonvirtualenvironment.PveWebsocketResourceSpec"
```
```
[info] Tests: succeeded 41, failed 6, canceled 1, ignored 0, pending 0
```
`PveWebsocketResourceSpec` is 3/3 green across 5 runs. The 6 failures are
pre-existing on Windows and untouched by this change — they are the
`Scripts/python.exe` family, identical by name before and after, and the
pre-bundle baseline on this machine is 43 run / 37 succeeded / 6 failed / 1
canceled. The new guard-evaluation assertion carries a bounded 500 ms wait per
test, and it is not a race: on correct code the third guard pass short-circuits
and never reads the session, so the latch can only be counted down by a mutant.
`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]
