aglinxinyuan opened a new pull request, #8036: URL: https://github.com/apache/texera/pull/8036
### What changes were proposed in this PR? Six tests added to `PveResourceSpec`, covering `PveManager`'s error paths. That file already owns this class's tests — 15 `"PveManager" should` blocks — so this extends it rather than adding a second spec. | Metric | Before | After | |---|---|---| | **Codecov (fully-covered lines)** | 198/228 = 86.8% | **205/228 = 89.9%** | | Branch arms | 53/140 | 60/140 | **+7 fully-covered lines and +7 branch arms.** The newly covered lines are the three give-up arms of `resolveSystemPackages` (149/151, 170/172, 181/183) and the uninstall-failure arm of `deletePackages` (565) — all plain control flow, no `logger.info`/`debug` bodies, so none is a line that looks green locally and dies in CI. These figures are the **CI-equivalent projection**, not the raw local numbers. `PveResourceSpec` has 6 pre-existing Windows-only failures — its fake runner fabricates a POSIX `<venv>/bin/python` while `PveManager` resolves `<venv>/Scripts/python.exe` — so the raw local delta is inflated and is deliberately not quoted. The projection was built by applying a throwaway test-file scaffold that makes the runner platform-aware, measuring both sides with it applied byte-identically, then reverting it. **On the absolute endpoints:** two independent measurements agree on +7 but differ by 3 on the endpoints (198 → 205 vs 195 → 202). The delta is solid and reproduced three times; the endpoint is the softer number. I would rather say that than publish a figure with more confidence than it has. ### What the reviewers found This bundle went through a mutation audit and then a separate vacuity review, and **each found real defects the other could not**. The mutation audit's most serious catch: the resolver tests recorded what the runner was handed but asserted only its *length*, never *what ran*. Retargeting the install from the throwaway venv to `PythonUtils.getPythonExecutable` was **byte-identical to baseline** — meaning `resolveSystemPackages` would pip-install `requirements.txt` into the machine's system Python and report that interpreter's package set as "the system set", with the test named *"give up without attempting the install"* unable to tell the difference. The vacuity review then found three things a mutation audit structurally cannot: - **The throwaway-venv cleanup was executed four times per run and constrained by nothing.** Swapping `Comparator.reverseOrder()` for `naturalOrder()` makes the delete hit the non-empty parent first, throw, and get swallowed by the block's own `catch` — the entire temp tree leaks, and the suite stayed green. - **The sanitiser's composition order was unpinned.** Each rule (trim, drop-blank, drop-comment) was pinned individually, but exchanging `.map(_.trim).filter(…)` for `.filter(…).map(_.trim)` survived, because every fixture line landed the same way either way. Fixed by indenting the `## FIXME` fixture line two spaces — it is a comment only *after* trimming, so it is the one line that separates the two orders. - **Three comments staked the fixture's whole rationale on the `--constraint` file, and the word appeared in the spec only inside comments.** No assertion referenced the flag, the file, or its contents. Two mutants were therefore unkillable — including one that keeps package *names* but drops every *version pin*, leaving pip free to resolve any `pyarrow`. All are now killed by a named test. The new `--constraint` test adds **zero** coverage — its path was already covered — and is included purely as a mutation-strength test; it is the sole killer of the constraint-file mutant. ### Verification Four load-bearing mutants were re-run against the final content, one at a time, each with a uniqueness-asserted anchor, reverted from a scratch snapshot, with the production diff verified empty and an md5 match before every subsequent compile. **All four killed**, each by a named test. One published mutation row was discarded rather than reported: applied exactly as written it failed to *compile* (`forward reference to value collected`), and a compile-only mutant proves nothing. It was replaced with a semantically equivalent hoisted variant that does compile and does die. **No regression.** The 6 pre-existing failures are identical in *name* on `main` and on this branch, not merely equal in count. All six new tests pass on Windows unscaffolded. ### Deliberately not included `resolveSystemPackages` **fails open** and this PR does not pin that as correct. All three give-up arms return `Seq.empty`, and callers degrade silently: `systemPackageNames` becomes empty, so a user may install or delete any package including one the Python workers depend on, and the `--constraint` file becomes empty, so user installs are no longer pinned. The tests assert the *giving up* — that the next command never runs, that a partial freeze is discarded — which is unambiguously right. No assertion blesses "empty set" as the correct answer for the application, and the spec says so. Fixing it is a production change. Two survivor probes are recorded rather than pinned: the three `logger.error` calls can all be deleted with the suite outcome-identical (asserting on log lines means attaching an appender inside a shared, strictly-serial JVM, and those lines stay Codecov-missed regardless because of the scalalogging guard arm), and the cleanup `finally` block can be replaced with `()` — those lines already count as hits through try/finally bytecode duplication, so pinning them is worth zero. `systemPackages` / `systemPackageNames` / `systemConstraintFile` are JVM-lifetime lazy vals, so resolution can be driven exactly once per JVM and the public `getSystemPackages` can never be exercised against a failing runner. The tests reach the private method reflectively; a rename yields `NoSuchMethodException`, i.e. a test error rather than a false pass, which was verified. No production file is touched. ### Any related issues, documentation, discussions? Closes #8034 ### How was this PR tested? ``` sbt "WorkflowExecutionService/testOnly org.apache.texera.web.resource.pythonvirtualenvironment.*" ``` ``` [info] Total number of tests run: 57 [info] Tests: succeeded 51, failed 6, canceled 1, ignored 0, pending 0 ``` The 6 failures are the pre-existing Windows-only ones described above and are present on `main` unchanged; under the CI-equivalence scaffold the same scope reports `succeeded 57, failed 0`. `WorkflowExecutionService/Test/scalafmtCheck` and `WorkflowExecutionService/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]
