aglinxinyuan opened a new pull request, #7831:
URL: https://github.com/apache/texera/pull/7831
### What changes were proposed in this PR?
Test-only. **No production file changes** — `git diff -- '*/src/main/*'` is
empty.
`LakeFSFileDocument` looked Docker-gated and is not:
`LakeFSStorageClientSpec` (merged in #7273) already drives
`LakeFSStorageClient` against an in-process `com.sun.net.httpserver` loopback
stub, and already covers the exact two calls the document makes. This PR reuses
that harness.
Measured with `WorkflowCore/jacoco` under an identical filter for the before
and after runs (the Docker-backed `LakeFSStorageClientMtimeSpec` is excluded
from **both**, so these differ from the Codecov figures, which include it):
| File | Lines | Branches |
|---|---|---|
| `LakeFSFileDocument.scala` | 32/80 (40.0%) → **65/80 (81.2%)** | 16/46 →
**27/46** |
| `LakeFSStorageClient.scala` | 120/146 (82.2%) → **133/146 (91.1%)** |
41/60 → **45/60** |
**+33 lines and +11 branch arms** on the document, **+13 lines** on the
client. Tests **38 → 49**.
Covered: `asInputStream`'s presign happy path and its fallback (the local
`def fallbackToLakeFS` lifts to `fallbackToLakeFS$1`, a counted method rather
than an `$anonfun$`, which is why these lines move the number at all), `asFile`
both arms including the `tempFile` memoization, `clear` both arms, and
`LakeFSStorageClient.put`.
The 15 lines still missed on the document are exactly the out-of-scope
region — lines 133-161, the `userJwtToken`-non-empty file-service presign
branch. Reaching it needs a forked JVM with `USER_JWT_TOKEN` set **and** a
presign endpoint on a *fixed* port known before JVM start, i.e. a `build.sbt`
`testGrouping` change plus a hardcodable port that can collide in CI.
Deliberately excluded.
### Verification
Review proposed 8 findings. **Six were real survivors and are now killed**,
each proved red on a named test after the repair; two are genuinely unpinnable
here and are recorded in the spec rather than papered over. Suite 47 → 49 after
repair.
| Survivor found by review | How it is now killed |
|---|---|
| both `asInputStream` call sites reduced to
`fileRelativePath.getFileName.toString` | a nested-fixture read test that kills
the pair *and* each site alone |
| widening the `put` success guard to `code >= 400` | rejection test
extended to 403/204/307; the 204 and 307 single-leg widenings each die
separately |
| deleting `inputStream.close()` | a recording-subclass test, deterministic
on any platform |
| re-downloading into the memoized `File` | `requests.clear()` plus `hits
shouldBe empty` |
| dropping `setChunkedStreamingMode` | the stub now records request headers;
the put test pins `content-length: 5` and the absence of `transfer-encoding` |
| `Files.exists shouldBe true` was JDK-guaranteed | now `Files.size == 2502`
— and the mutant fails *at that line* rather than 12 lines later |
**Two limits, argued rather than cargo-culted.** The fallback `logger.warn`
at :110 cannot be pinned: `workflow-core`'s test classpath has no SLF4J
provider at all ("No SLF4J providers were found", NOP loggers), and
scala-logging's `isWarnEnabled` guard means the call is not even evaluated — a
capturing appender has nothing to attach to. And `deleteRepo` (1 line) was left
alone: it is `repoApi.deleteRepository(name).execute()` with no branch, so the
only assertable fact is the generated SDK's own URL template. The existing spec
already documents that rationale and I did not contradict it for one line.
### Two production bugs found while doing this, reported not fixed
Both are out of scope for a test-only PR and neither is cemented by any new
test.
1. **`LakeFSFileDocument.scala:204` renders the relative path with the
platform separator.** On a Windows worker, every nested dataset/model file is
requested as `dir\file.csv` in the lakeFS `path` query parameter — 404, silent
fallback, 404 again. A reviewer's suggested assertion of
`Some("nested/dir/records.csv")` would be red against today's production, which
is how this surfaced. Fix is a one-line `'/'`-join and wants its own PR.
2. **`tempFileStream.close()` is load-bearing on Windows only.** Deleting it
fails 4 tests here with "file is being used by another process" (the cleanup
hits an open write handle) while shipping green through Linux CI.
`FileOutputStream` is unbuffered, so no portable in-process assertion can
observe it; wrapping the copy in `try`/`finally` is production work.
### Any related issues, documentation, discussions?
Closes #7830
### How was this PR tested?
```
sbt "WorkflowCore/testOnly
org.apache.texera.amber.core.storage.model.LakeFSFileDocumentSpec
org.apache.texera.amber.core.storage.util.LakeFSStorageClientSpec"
```
```
[info] Suites: completed 2, aborted 0
[info] Tests: succeeded 49, failed 0, canceled 0, ignored 0, pending 0
```
`scalafmtCheckAll` and `scalafixAll --check` both pass.
`LakeFSFileDocumentSpec` gains `@NonParallelTest` so it gets its own forked
JVM, matching the sibling spec.
### 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]
