The GitHub Actions job "Comment commands" on texera.git/main has succeeded. Run started by GitHub user tanishqgandhi1908 (triggered by tanishqgandhi1908).
Head commit for run: b01b11f8182313b9f3bff877c661f7ae0f0b5a1c / Xinyuan Lin <[email protected]> test(amber): cover PveManager system-package resolution and package deletion (#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) Report URL: https://github.com/apache/texera/actions/runs/33098799114 With regards, GitHub Actions via GitBox
