This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/texera.git
The following commit(s) were added to refs/heads/main by this push:
new b01b11f818 test(amber): cover PveManager system-package resolution and
package deletion (#8036)
b01b11f818 is described below
commit b01b11f8182313b9f3bff877c661f7ae0f0b5a1c
Author: Xinyuan Lin <[email protected]>
AuthorDate: Thu Aug 27 08:21:43 2026 +0000
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)
---
.../pythonvirtualenvironment/PveResourceSpec.scala | 229 ++++++++++++++++++++-
1 file changed, 227 insertions(+), 2 deletions(-)
diff --git
a/amber/src/test/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveResourceSpec.scala
b/amber/src/test/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveResourceSpec.scala
index 926aa87ada..dd59386e64 100644
---
a/amber/src/test/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveResourceSpec.scala
+++
b/amber/src/test/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveResourceSpec.scala
@@ -58,10 +58,42 @@ class PveResourceSpec
private var installExit = 0
private var uninstallExit = 0
+ // Kept separate from installExit so a test can let `pip freeze` print a
package
+ // and *still* fail — a freeze that dies halfway is the case where "return
what
+ // was collected" and "return nothing" differ.
+ private var freezeExit = 0
+
+ // When set, the mocked runner throws for a `pip uninstall` instead of
returning
+ // an exit code. `Process.!` throws rather than returning non-zero when the
+ // interpreter has gone missing or is not executable, which is reachable
here:
+ // nothing stops the venv directory being removed between the guard and the
spawn.
+ private var uninstallThrows = false
+
+ // Every command the mocked runner is handed, in order. Lets a test assert
what
+ // did *not* run, which is the only thing that separates "gave up at this
step"
+ // from "ran everything and happened to produce nothing".
+ private val recordedCommands =
scala.collection.mutable.ListBuffer[Seq[String]]()
+
// What the mocked `pip freeze` reports as the resolved system set. pyarrow
is
// always a hard dependency in amber/requirements.txt, so it stands in for "a
// system package the user may neither install nor delete".
- private val systemFreeze = Seq("pyarrow==23.0.1")
+ //
+ // Deliberately not clean: a real `pip freeze` emits blank lines and `##
FIXME:`
+ // comment lines for editable/VCS installs it cannot pin, and surrounding
whitespace
+ // is not guaranteed. Those lines must not reach `systemPackages`, because
they are
+ // written verbatim into the `--constraint` file every user install is
pinned against,
+ // where a malformed line makes pip abort. A fixture of one already-trimmed,
+ // already-non-comment line leaves the resolver's sanitising step
unconstrained.
+ //
+ // The `## FIXME:` line is indented too, which is what makes the *order* of
the two
+ // sanitising steps observable: trimming and then filtering is not the same
as filtering
+ // and then trimming, and an indented comment is the only line that can tell
them apart —
+ // it is a comment only once the leading whitespace is gone.
+ private val systemFreeze =
+ Seq("", " ## FIXME: could not find svn location", " pyarrow==23.0.1 ")
+
+ // What the resolver is required to turn `systemFreeze` into.
+ private val expectedSystemPackages = Seq("pyarrow==23.0.1")
private val realRunner = PveManager.runProcess
@@ -79,6 +111,7 @@ class PveResourceSpec
runProcessMock
.expects(*, *, *)
.onCall { (command: Seq[String], _: Seq[(String, String)], logger:
ProcessLogger) =>
+ recordedCommands += command
if (command.contains("venv")) {
if (venvExit == 0) {
val bin = Paths.get(command.last).resolve("bin")
@@ -92,9 +125,14 @@ class PveResourceSpec
venvExit
} else if (command.contains("freeze")) {
systemFreeze.foreach(line => logger.out(line))
- 0
+ freezeExit
} else if (command.contains("uninstall")) {
+ if (uninstallThrows) throw new java.io.IOException("boom")
logger.out("mock uninstall")
+ // A failing pip writes the reason to stderr; that is the only line
telling the
+ // user *why* the delete failed, so the fixture has to produce one
for the
+ // stderr arm of deletePackages' ProcessLogger to be reachable at
all.
+ if (uninstallExit != 0) logger.err("ERROR: Cannot uninstall
colorama")
uninstallExit
} else if (command.contains("install")) {
logger.out("mock install")
@@ -123,6 +161,9 @@ class PveResourceSpec
venvExit = 0
installExit = 0
uninstallExit = 0
+ freezeExit = 0
+ uninstallThrows = false
+ recordedCommands.clear()
testPveName = s"testenv${System.currentTimeMillis()}"
testRoot = Paths.get("/tmp/texera-pve/venvs").resolve(testCuid.toString)
queue = new LinkedBlockingQueue[String]()
@@ -147,6 +188,40 @@ class PveResourceSpec
queue.iterator().asScala.toList.mkString("\n")
}
+ /**
+ * Puts just the interpreter where PveManager looks for it, without going
through
+ * `createNewPve`. The mocked venv creation fabricates a POSIX `bin/python`
layout,
+ * so the create flow cannot stand a PVE up on a platform whose interpreter
lives
+ * somewhere else; `pythonBinFor` asks PveManager's own question instead.
+ */
+ private def fabricatePve(pveName: String): Path = {
+ val python = pythonBinFor(pveName)
+ Files.createDirectories(python.getParent)
+ Files.write(python, Array.emptyByteArray)
+ python.toFile.setExecutable(true)
+ python
+ }
+
+ /** Where PveManager records the packages the user installed into a PVE. */
+ private def userPackagesFile(pveName: String): Path =
+ testRoot.resolve(pveName).resolve("user-packages.txt")
+
+ /**
+ * Calls `PveManager.resolveSystemPackages()` directly.
+ *
+ * Its public face is `getSystemPackages`, which reads the `systemPackages`
lazy val.
+ * That val is memoised for the life of the JVM and amber's suites share
one, so a test
+ * that forced it through a failing runner would fix the system package set
at empty for
+ * every suite that follows — starting with this spec's own two
system-package tests.
+ * Reaching the resolver directly is what makes its failure arms testable
without that
+ * side effect; there is no seam that would let an ordinary call do the
same.
+ */
+ private def resolveSystemPackages(): Seq[String] = {
+ val method = PveManager.getClass.getDeclaredMethod("resolveSystemPackages")
+ method.setAccessible(true)
+ method.invoke(PveManager).asInstanceOf[Seq[String]]
+ }
+
/**
* A computing-unit id whose venv directory does not exist on this machine.
* PveManager.getEnvironments lists /tmp/texera-pve/venvs/<cuid> directly,
so a fixed id
@@ -639,4 +714,154 @@ class PveResourceSpec
s"[PVE][ERR] Python executable not found for PVE:
${pythonBinFor(absent).toAbsolutePath}"
}
+ /*
+ * The uninstall's two unhappy endings. Both are about the same thing: the
recorded package
+ * list is a claim about what is installed in the venv, so it may only
change when pip
+ * actually removed something.
+ */
+ it should "report the failure and leave the recorded package list alone when
pip uninstall exits non-zero" in {
+ expectProcessCalls()
+ fabricatePve(testPveName)
+ val metadata = userPackagesFile(testPveName)
+ Files.write(metadata, Seq("colorama==0.4.6").asJava)
+ uninstallExit = 1
+
+ val output = PveManager.deletePackages(testCuid, "colorama", testPveName)
+
+ output should contain("[PVE][ERR] Failed to uninstall package: colorama")
+ output should not contain "[PVE] Uninstalled colorama successfully"
+ // colorama is still in the venv, so the manifest has to keep saying so.
+ Files.readAllLines(metadata).asScala should contain("colorama==0.4.6")
+
+ // What was actually handed to pip. `PIP_NO_INPUT=1` is set in pipEnv, so
an
+ // uninstall missing `-y` aborts instead of prompting — dropping it would
break
+ // every user package deletion while leaving every exit-code assertion
above green,
+ // because the fake runner answers on the command's *shape*, not its argv.
+ val uninstall = recordedCommands.last
+ uninstall should contain("uninstall")
+ uninstall should contain("-y")
+ uninstall.last shouldBe "colorama"
+ uninstall.head shouldBe pythonBinFor(testPveName).toAbsolutePath.toString
+
+ // pip's own output is the payload PveResource.deletePackage hands back to
the UI;
+ // both streams have to survive the trip, or "report the failure" reports
nothing
+ // but PveManager's own generic wrapper line.
+ output should contain("[pip] mock uninstall")
+ output should contain("[pip][ERR] ERROR: Cannot uninstall colorama")
+ }
+
+ it should "return an error list rather than throwing when the uninstall
cannot be spawned" in {
+ expectProcessCalls()
+ fabricatePve(testPveName)
+ uninstallThrows = true
+
+ val output = PveManager.deletePackages(testCuid, "colorama", testPveName)
+
+ // The caller is a JAX-RS resource that turns this list into a 200/400; an
escaping
+ // exception would reach it as a 500 with no message about which PVE
failed.
+ output should have size 1
+ output.head should include(s"cuid=$testCuid")
+ output.head should include("boom")
+ }
+
+ /*
+ * The other half of what the resolved system set is for. Refusing to
install a package
+ * whose name collides with a system one is covered above; this is the
constraint file,
+ * which is how the resolved *versions* reach pip. Without it a user install
is free to
+ * pull a different pyarrow in as a transitive dependency of something else
and break the
+ * Python workers, and the install still reports success.
+ */
+ "PveManager.installUserPackages" should "pin the install against the
resolved system versions" in {
+ expectProcessCalls()
+ fabricatePve(testPveName)
+
+ PveManager.installUserPackages(List("colorama==0.4.6"), testCuid, queue,
testPveName)
+
+ val install = recordedCommands.last
+ install should contain("--constraint")
+
+ // pip reads the path as the argument of the flag, so the two travel
together and in
+ // this order; handing it the package name instead is still a well-formed
command line.
+ val constraintFile = Paths.get(install(install.indexOf("--constraint") +
1))
+
+ // Equality, against the same sanitised set the resolver is required to
produce: this
+ // file is where those lines end up, and it is the reason the blank and
`## FIXME:`
+ // lines of `systemFreeze` may not survive resolution — pip aborts the
whole install
+ // on a constraint line it cannot parse.
+ Files.readAllLines(constraintFile).asScala.toSeq shouldBe
expectedSystemPackages
+ }
+
+ /*
+ * `resolveSystemPackages`' three give-up arms.
+ *
+ * The resolved set is what stops a user from installing a package that
shadows one the
+ * Python workers depend on, so what matters at each failure is that the
resolver stops
+ * there rather than carrying a half-built answer forward: no pip install
into a venv that
+ * was never created, no `pip freeze` of a venv whose install failed, and no
partial freeze
+ * promoted to "the system set".
+ *
+ * These assert the giving up, not that an empty set is a good answer to
give the rest of
+ * the app — an empty set is what `systemPackageNames` and `--constraint`
both degrade to,
+ * which is fail-open. That is a property of the caller, and it is not
pinned here.
+ */
+ "PveManager's system-package resolution" should "give up without attempting
the install when the throwaway venv cannot be created" in {
+ expectProcessCalls()
+ venvExit = 1
+
+ resolveSystemPackages() shouldBe empty
+
+ recordedCommands should have size 1
+ recordedCommands.head should contain("venv")
+ }
+
+ it should "give up without freezing when the requirements install fails" in {
+ expectProcessCalls()
+ installExit = 1
+
+ resolveSystemPackages() shouldBe empty
+
+ recordedCommands should have size 2
+ recordedCommands.exists(_.contains("freeze")) shouldBe false
+
+ // The install has to go into the throwaway venv, whose directory is the
last argument
+ // of the create that preceded it. Aiming it at the system interpreter
instead still
+ // produces two commands in the right order and still returns empty here,
so the count
+ // above cannot tell the two apart — and getting it wrong would pip-install
+ // amber/requirements.txt straight into the machine's Python.
+ recordedCommands(1).head should startWith(recordedCommands.head.last)
+
+ // The throwaway venv is a real directory tree by this point — the create
step above
+ // succeeded, so it holds the interpreter the fake runner fabricated, as a
real
+ // `python -m venv` would. Giving up early must still take it away with
it: nothing
+ // revisits the directory afterwards, and its name is a fresh temp path
each time, so
+ // whatever is left there is left for good.
+ Files.exists(Paths.get(recordedCommands.head.last)) shouldBe false
+ }
+
+ it should "discard what a failing pip freeze already printed" in {
+ expectProcessCalls()
+
+ // Control: with the freeze succeeding, this same fixture does produce a
system set. Without
+ // it, the assertion below would hold just as well for a resolver that
returned whatever the
+ // freeze printed, because a fixture that printed nothing would look
identical.
+ //
+ // Pinned as an equality, not a `contain`: `systemFreeze` deliberately
includes a blank
+ // line and a `## FIXME:` comment, and the exact answer is what says those
are dropped
+ // and the surviving line is trimmed. A `contain` would pass on a resolver
that handed
+ // the raw freeze output through into the `--constraint` file.
+ resolveSystemPackages() shouldBe expectedSystemPackages
+
+ // The freeze has to interrogate the throwaway venv, not whatever
interpreter is on PATH:
+ // freezing the system Python would report that machine's packages as "the
system set".
+ recordedCommands.last.head should startWith(recordedCommands.head.last)
+
+ recordedCommands.clear()
+ freezeExit = 1
+
+ // The mock still emits pyarrow==23.0.1 before reporting the failure, so a
resolver that
+ // kept the collected lines would answer with a system set of one.
+ resolveSystemPackages() shouldBe empty
+ recordedCommands.last should contain("freeze")
+ }
+
}
