Ma77Ball commented on code in PR #8978:
URL: https://github.com/apache/texera/pull/8978#discussion_r4287625461
##########
common/workflow-operator/src/test/scala/org/apache/texera/amber/operator/source/scan/file/FileScanSourceOpExecSpec.scala:
##########
@@ -185,6 +191,11 @@ class FileScanSourceOpExecSpec extends AnyFlatSpec with
BeforeAndAfterAll {
assert(field.isInstanceOf[LargeBinary])
assert(field.asInstanceOf[LargeBinary].getUri.startsWith("s3://"))
Review Comment:
Nit (P3, optional): nothing ties the *produced* pointer to the prefix this
test cleans and then asserts on, so the guard and the produce-check can
silently drift apart.
Cleanup runs unconditionally in the `finally` above, before this block, so
`directoryExists("objects/-8670/")` is already false whether the object was
cleaned up or was never written under this prefix in the first place. The
existing assertions only prove a `LargeBinary` with an `s3://` URI was
produced, not that it lives under `objects/$testExecutionId/`. Asserting that
in one extra line keeps the write side and the leak-guard side in lockstep
(e.g. if a later change seeds a different id on one path but not the other,
this catches it instead of passing green).
Suggested fix:
```suggestion
assert(field.asInstanceOf[LargeBinary].getUri.startsWith("s3://"))
assert(field.asInstanceOf[LargeBinary].getUri.contains(s"objects/$testExecutionId/"))
```
--
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]
