aglinxinyuan commented on code in PR #7875: URL: https://github.com/apache/texera/pull/7875#discussion_r3894988715
########## amber/src/test/scala/org/apache/texera/web/resource/dashboard/DatasetSearchQueryBuilderSpec.scala: ########## @@ -89,14 +89,15 @@ import scala.jdk.CollectionConverters._ * later suite. Binding at the address the config already names is order-independent and * memoizes nothing wrongly. The price is that the port is machine-global: if something else * holds it (a local `bin/local-dev.sh up` lakeFS, or a sibling worktree running these tests), - * the four tests that need a *successful* size `cancel` rather than fail. CI has no lakeFS in + * the six tests that need a *successful* size `cancel` rather than fail. CI has no lakeFS in * this job, so it is deterministic there. * - * A cancel is louder than it looks, and the sbt log shows it only as `canceled 4` while the + * A cancel is louder than it looks, and the sbt log shows it only as `canceled 6` while the * build stays green. It disarms the whole `toEntry` half of this suite: not just its coverage * (which falls back toward the ~59% this file had before), but every assertion protecting - * `toEntryImpl`. Measured: with the port held, mutating `size` to `0L` in the entry leaves - * `succeeded 14, failed 0, canceled 4` and sbt printing "All tests passed" at exit 0. The + * `toEntryImpl` — the owner-email pair included, so the `new User` mutant this suite otherwise + * kills goes unnoticed too. Measured: with the port held, mutating `size` to `0L` in the entry + * leaves `succeeded 15, failed 0, canceled 6` and sbt printing "All tests passed" at exit 0. The Review Comment: Not changed here — the cancel is pre-existing deliberate design from #7855, not something this PR introduces, and the rationale is in the class comment: the stub must bind the port `StorageConfig.lakefsEndpoint` already names (the lakeFS client memoises it in a `lazy val` and amber runs every suite in one unforked JVM), so failing instead would break the suite for any developer with `bin/local-dev.sh up` running. CI has no lakeFS in this job, so the assertions are always armed there. The risk you name is real, though, so this PR strengthens the warning rather than the failure mode: the class comment now records that the owner-email pair is disarmed too, with the re-measured `succeeded 18, failed 0, canceled 6`. Making canceled tests fail the build is worth its own issue. _🤖 Addressed by [Claude Code](https://claude.com/claude-code)_ -- 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]
