Copilot commented on code in PR #7875:
URL: https://github.com/apache/texera/pull/7875#discussion_r3894171961
##########
amber/src/test/scala/org/apache/texera/web/resource/dashboard/DatasetSearchQueryBuilderSpec.scala:
##########
@@ -491,14 +492,37 @@ class DatasetSearchQueryBuilderSpec
sql should include("dataset.is_downloadable as is_dataset_downloadable")
sql should include("dataset_user_access.privilege as user_dataset_access")
sql should include("dataset.cover_image as cover_image")
+ // The one projected column that is not a DATASET column. It is also the
slot this schema used to
+ // leave at its `DSL.inline("")` default, which is why it gets an
assertion of its own rather
+ // than trusting the entry-level test: a slot dropped back to a literal
renders `'' as email`
+ // here, and the failure names the projection instead of a null three
layers downstream.
+ sql should include("user.email as email")
+ }
+
+ it should "stay union-compatible with the workflow and project branches" in {
+ // `DashboardResource.searchAllResources` stacks the three builders with
`unionAll` for a
+ // resourceType of "" — the dashboard's default view — so every branch
must project the same
+ // aliases in the same order with types Postgres will unify. Each slot one
builder fills is a
+ // literal placeholder in the other two (`'' as email` here until
`userEmail` was named), which
+ // makes the contract easy to break from inside a single builder and
impossible to break loudly:
+ // nothing fails to compile, and the mismatch surfaces only as a failed
query at runtime. This is
+ // the only test that executes the union; the rest of the suite renders or
fetches one branch.
+ val union = WorkflowSearchQueryBuilder
+ .constructQuery(uid, params(), includePublic = true)
+ .unionAll(ProjectSearchQueryBuilder.constructQuery(uid, params(),
includePublic = true))
+ .unionAll(DatasetSearchQueryBuilder.constructQuery(uid, params(),
includePublic = true))
+
+ // Both seeded datasets are public, so both reach `uid`; no workflow or
project rows are seeded.
+ // The row count is incidental — that Postgres accepts the union at all is
the assertion.
+ getDSLContext.fetch(union).size() shouldBe 2
Review Comment:
This test’s stated intent is “Postgres accepts the union at all”, but it
asserts an exact row count (`shouldBe 2`), which couples the test to fixture
cardinality. To better match the intent and reduce brittleness, consider
asserting that executing the query does not throw (and optionally that the size
is non-negative / equals the dataset count derived from the fixture rather than
a hard-coded literal).
##########
amber/src/test/scala/org/apache/texera/web/resource/dashboard/DatasetSearchQueryBuilderSpec.scala:
##########
@@ -491,14 +492,37 @@ class DatasetSearchQueryBuilderSpec
sql should include("dataset.is_downloadable as is_dataset_downloadable")
sql should include("dataset_user_access.privilege as user_dataset_access")
sql should include("dataset.cover_image as cover_image")
+ // The one projected column that is not a DATASET column. It is also the
slot this schema used to
+ // leave at its `DSL.inline("")` default, which is why it gets an
assertion of its own rather
+ // than trusting the entry-level test: a slot dropped back to a literal
renders `'' as email`
+ // here, and the failure names the projection instead of a null three
layers downstream.
+ sql should include("user.email as email")
Review Comment:
The SQL assertion expects `user.email as email`, but other assertions in
this suite suggest jOOQ renders the fully-qualified table name (e.g.,
`texera_db.user`). If the actual projection is `texera_db.user.email as email`
(as described in the PR), this test will fail despite correct behavior. Update
the assertion to match the rendered SQL (or make it resilient by accepting
either qualified or unqualified forms).
##########
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:
Relying on `cancel` when the lakeFS stub can’t bind (and allowing sbt to
exit green) creates a real risk of false-positive test runs locally—especially
now that more correctness assertions depend on the stub. Consider changing the
stub failure mode to fail the suite (or otherwise make canceled tests fail the
build), so local runs can’t silently skip the assertions that protect
`toEntryImpl`.
--
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]
