aglinxinyuan commented on code in PR #7556:
URL: https://github.com/apache/texera/pull/7556#discussion_r3763515997
##########
amber/src/test/scala/org/apache/texera/web/service/ExecutionResultServiceSpec.scala:
##########
@@ -79,12 +115,20 @@ class ExecutionResultServiceSpec
private val testUid: Integer = 9001
private var executionsDao: WorkflowExecutionsDao = _
private var testVid: Integer = _
+ private var testCuid: Integer = _
+
+ // AmberClient needs an ActorSystem to host its ClientActor. A bare one is
enough:
+ // the client is constructed over an empty PhysicalPlan, so its
InitializeRequest
+ // completes without an engine, and the subclass below never sends anything
to it.
+ private var system: ActorSystem = _
override protected def beforeAll(): Unit = {
initializeDBAndReplaceDSLContext()
+ system = ActorSystem("ExecutionResultServiceSpec")
}
override protected def afterAll(): Unit = {
+ system.terminate()
shutdownDB()
}
Review Comment:
Same fix as the thread above — c0edd92b53 awaits termination and gives the
system a UUID-suffixed name.
Two of the four suppressed comments are also applied there (disposing the
`ResultEvents` subscriptions, and giving `RecordingCancellable` the standard
`cancel()` return semantics). The other two I looked into and did **not**
apply, because both rest on a premise that does not hold in this codebase —
flagging that rather than leaving them silently unaddressed:
**Closing the document after `getTotalFileSize`.** `VirtualDocument` has no
`close()`. Its only teardown method is `clear()`, which deletes the data rather
than releasing a handle, so there is nothing to close — I tried the suggested
`try/finally` and it fails to compile (`value close is not a member of ...
VirtualDocument`). I have added a comment at that call site recording why
nothing is closed, so the next reader does not retry it.
**Setting `StorageConfig.warehouseEnabled` explicitly for the warehouse
test.** It is a `val`, not a `var`, so a test cannot set or restore it.
`WarehouseReadGuard.assertReadable` does take an injectable `enabled`
parameter, but the production call site inside `ExecutionResultService` uses
the default, so the test cannot reach it either. The dependency on the ambient
default is real and the test already documents it; `WarehouseReadGuardSpec`
pins both settings of the flag directly, which is where that coverage belongs.
Making it settable would be a production change and does not belong in a
test-only PR.
--
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]
