aglinxinyuan opened a new pull request, #7800:
URL: https://github.com/apache/texera/pull/7800
### What changes were proposed in this PR?
Three test-only fixes. **45 insertions, 9 deletions across 3 spec files;
`git diff -- '*/src/main/*'` is empty.**
### 1. A leaked file handle was aborting the operator suite and killing the
module's coverage run
The exact mechanism, established by instrumenting the run rather than by
inspection: in `"create LargeBinary when reading file with LARGE_BINARY type"`,
`createTuplesFromFile` throws mid-`map` with `IllegalStateException:
LargeBinaryManager.create() requires a base URI, but none was set on the
current thread`. That abandons the `AutoClosingIterator` before exhaustion, so
its close-on-exhaustion hook never fires. `afterAll` then fails with
`test_large_binary.txt: The process cannot access the file because it is being
used by another process`, aborting the suite.
The blast radius is the whole module:
| | before | after |
|---|---|---|
| Suites | 289 completed, **1 aborted** | **290 completed, 0 aborted** |
| sbt exit | 1 | **0** |
| jacoco report | **no report directory emitted** | `jacoco.xml`, 1.8 MB |
| Tests | 2301 succeeded | 2301 succeeded |
So `WorkflowOperator/jacoco` could not produce coverage for *any* file in
the module on Windows. POSIX `unlink` masks this on Linux CI.
**The fix is in the test**: the read is wrapped so the iterator is drained
in a `finally`, firing the close hook on both the success and failure paths.
`AutoClosingIterator` and `createTuplesFromFile` are untouched — the leak is a
test that stops early, not a broken production contract.
Also worth recording: the `.zip` fixture was stranded too. It only *looked*
fine because `.gitignore:10` is `*.zip`; `afterAll` never reached it, because
the `.txt` delete threw first.
### 2. `IntervalOpExecSpec` is now deterministic
It imported `scala.util.Random.{nextInt, nextLong}` and used them at four
sites — input ordering twice, a 1k-row dataset, and the interval constant — so
`IntervalJoinOpExec`'s coverage footprint drifted between runs of identical
source (CI has reported 2 missed + 14 partial where a local run gave 0 + 13).
Now a fixed `Seed`, with a fresh `new Random(Seed)` created at each use site
so determinism does not depend on test execution order either. No assertion
changed.
Verified rather than assumed: instrumented to print every generated input,
two runs produced byte-identical output (22 lines, empty `diff`), and **two
full `WorkflowOperator/jacoco` runs now give byte-identical
`IntervalJoinOpExec` counters and all 110 per-line entries.**
One observation left alone as out of scope: the 1k test deterministically
yields 0 matches, because random 64-bit longs essentially never fall inside a
sub-1000-wide window, so `outputTuples.size == bruteForceResult` is `0 == 0`.
That was equally true before this change.
### 3. `PveResourceSpec`'s traversal assertion said something it did not test
`getPythonBin(testCuid, "..") shouldBe None` appeared to pin the guard at
`PveManager.scala:91`, but `".."` **matches** the name regex on line 88
(`^[A-Za-z0-9._-]+$` admits dots), and `<VenvRoot>/<cuid>/../pve` normalizes to
`<VenvRoot>/pve` — still under root. The `None` came from the `Files.exists`
check on line 92.
**Proven, not argued:** deleting line 91 produced results identical to
baseline (43 run, 37 succeeded, 6 failed, 1 canceled) and the traversal test
still **passed**. Nothing in the spec detects that guard's removal. The
production file was reverted afterwards.
Split into two honest cases:
- `"reject pveNames containing a path separator"` keeps `"../../../etc"` and
`"foo/bar"` — both carry a `/`, so they genuinely pin the **name regex**.
- `"return None for a dot-only pveName, which has no venv"` keeps the `".."`
case and pins what it actually pins: a name with no venv on disk yields `None`.
A comment records that line 91 is **unreachable by construction** — the
regex forbids `/` and `cuid` is an `Int` — so it is defensive code rather than
untested code. The guard is left in place; removing it is a production
decision, not a test one.
### Verification
- Full `WorkflowOperator/jacoco`: **290 suites, 0 aborted, 2301 tests
succeeded, sbt exit 0**, report emitted.
- `PveResourceSpec`: 44 run, 38 succeeded — both reshaped tests pass. The 6
failures are pre-existing and environmental (the mock fabricates `bin/python`
while `PveManager` looks for `Scripts/python.exe` on Windows); baseline shows
the identical 6.
- `git diff -- '*/src/main/*'` is empty; no stranded fixtures (checked with
`git status --ignored`).
- Lint green: `WorkflowOperator/Test/scalafmtCheck`,
`WorkflowOperator/scalafixAll --check`,
`WorkflowExecutionService/Test/scalafmtCheck`,
`WorkflowExecutionService/scalafixAll --check`.
### One further defect found, deliberately not fixed here
The same `FileScanSourceOpExecSpec` test wraps its assertions in `catch {
case e: Exception => info(...) }`. ScalaTest's `TestFailedException` extends
`Exception`, and `info` output is invisible under this build's `-u`-only
reporter (`build.sbt:37`) — so **that test currently cannot fail**. Narrowing
the catch (or using `cancel`/`assume`) would make it a real test again, but it
needs care about how `LargeBinaryManagerSpec` binds a base URI, so it does not
belong in this change.
### Any related issues, documentation, discussions?
Closes #7799
### How was this PR tested?
```
sbt "WorkflowOperator/jacoco"
```
```
[info] Suites: completed 290, aborted 0
[info] Tests: succeeded 2301, failed 0, canceled 0, ignored 0, pending 2
```
### 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]
