aglinxinyuan opened a new pull request, #7273: URL: https://github.com/apache/texera/pull/7273
### What changes were proposed in this PR? `LakeFSStorageClient` was the largest unclaimed gap in the repo — 83 missed, 37.9%. The existing spec only exercised `parsePhysicalAddress`; everything else needed a lakeFS server, and the only harness for that is the Docker/testcontainers `MtimeSpec`. This adds **20 tests** driven by a JDK `com.sun.net.httpserver` stub on `127.0.0.1:0` — no new dependency, no container. `StorageConfig.lakefsEndpoint` is a `var` precisely so tests can repoint it. **This is not an absence-of-service test.** Nothing asserts a connection failure or an `ApiException` caused by one, so nothing inverts if CI ever gains a lakeFS container. Every response is an explicit canned payload carrying exactly the fields the generated SDK marks required (read out of the sdk-1.51.0 sources), so the fixtures cannot silently rot into "any JSON passes". Several behaviours are **only** observable against a stub: | Behaviour | Why a live server can't show it | |---|---| | `fetchAllPages` cursor threading | `PageSize` is 1000 — a real server needs 1001 objects for a second page | | `retrieveVersionsOfRepository` descending sort | lakeFS already returns commits newest-first, so the sort is a no-op; the stub returns them out of order with the newest neither first nor last | | `completePresignedMultipartUploads` part ordering | a live server absorbs the order silently; the stub inspects the serialized body | | "rejected without issuing a request" | indistinguishable from a server-side reject | **Both suites are now tagged `@NonParallelTest`.** This matters: `apiClient` is a `private lazy val` (as are the seven `*Api` vals derived from it), so the endpoint is captured **once per JVM** on first force, and `MtimeSpec` repoints it at a container. The old pair avoided the clash only by accident — the old spec never forced `apiClient`. `build.sbt` already gives each tagged suite its own forked `Tests.Group`; tagging both means the isolation survives either tag being dropped later. Verified with `show WorkflowCore/Test/testGrouping` rather than assumed. Tagging `MtimeSpec` is independently correct anyway — it mutates `StorageConfig.s3Endpoint`, exactly the hazard that tag exists for. **The page-1 stub routes serve once.** Without that guard, a client that stopped threading the cursor would re-match a `has_more:true` route forever and **hang CI** rather than fail — the regression these tests exist to catch would have become a timeout. Verified by breaking the cursor in production: the suite now fails 3 tests in seconds instead of spinning. Measured with jacoco on a clean single-suite run: ``` before: 23/152 lines covered (15.1%) after: 132/152 lines covered (86.8%) ``` Assertion strength checked by **20 production mutations**, each killed by exactly the test that claims to guard it, no survivors — the repo-name regex bounds, the per-repository storage namespace, page-buffer accumulation, the commit sort, sum-vs-max, the empty-repo short-circuit, part ordering, the physical-address on abort, reset type, commit branch, stat field, download ref, the null guard, part count, size-vs-mtime, `PageSize`, delete argument order, diff branch, and `.trim`. All reverted; the production diff is empty. Deliberately left uncovered, with a comment so they stay easy to delete: `removeFileFromRepo` and `retrieveFileContent` have **no callers anywhere in the repo**, `withCreateVersion`'s only caller is a file-service test, and `stagingApi` is referenced nowhere — not even inside its own class. `deleteRepo` is a bodyless passthrough with no choice of its own to assert. Also skipped: `healthCheck`'s retry path, which would burn ~3.2s sleeping and pin `HealthCheckMaxAttempts`, a constant a harmless tuning change should be free to alter — the success path does assert exactly one probe, which is the invariant worth holding. ### Any related issues, documentation, discussions? Closes #7272 ### How was this PR tested? ``` sbt "WorkflowCore/testOnly org.apache.texera.amber.core.storage.util.LakeFSStorageClientSpec" ``` ``` [info] Tests: succeeded 25, failed 0, canceled 0, ignored 0, pending 0 [info] All tests passed. ``` `WorkflowCore/Test/scalafmtCheck` and `WorkflowCore/Test/scalafix --check` both `[success]`. One caveat worth stating plainly: **`LakeFSStorageClientMtimeSpec` could not be run locally** — Docker Desktop would not come up on this machine, so it aborts with "Could not find a valid Docker environment". The only edits to that file are the `@NonParallelTest` annotation and a scaladoc paragraph; no test body was touched. CI will be the first green run of that suite. ### 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]
