aglinxinyuan opened a new pull request, #7855: URL: https://github.com/apache/texera/pull/7855
### What changes were proposed in this PR? `DatasetSearchQueryBuilderSpec` goes from 9 tests to 18. The existing spec asserted on rendered SQL only, so `toEntryImpl` — the half that builds what the dashboard actually displays — was entirely unexercised. Measured with one fresh sbt JVM per run, `rm -rf` on the jacoco dir between them, and the same suite-name filter for before and after. Two metrics, because they differ: Codecov's (fully-covered lines, so a line with any missed branch arm counts against you) and JaCoCo's line-hit. | File | Codecov | JaCoCo line-hit | Branches | |---|---|---|---| | `DatasetSearchQueryBuilder.scala` | 34/58 = 58.6% → **53/58 = 91.4%** | 35/58 → **56/58** | 6/18 → **13/18** | Incidental, same module and same measurement, all reproduced from a HEAD baseline: | File | Codecov | |---|---| | `FulltextSearchQueryUtils.scala` | 15/33 = 45.5% → **20/33 = 60.6%** | | `UnifiedResourceSchema.scala` | 61/76 = 80.3% → **65/76 = 85.5%** | | `SearchQueryBuilder.scala` | 14/20 = 70.0% → **16/20 = 80.0%** | | `DashboardResource.scala` | 14/89 = 15.7% → **18/89 = 20.2%** | What the new tests pin: the size that reaches the dashboard, `isOwner`, the `NONE` privilege fallback, the resource-type literal that `DashboardResource` `MatchError`s on, the null that becomes the user-visible `hasMismatch` flag, every dataset column under the alias its schema slot names, the owner join predicate, the AND-composition of the date/id/keyword filters, and one keyword per reserved character. ### Two mechanical notes a reviewer will want **`toEntryImpl` is `override protected` here**, while the `Project` and `Workflow` siblings widen theirs to `override def`. So the `ProjectSearchQueryBuilderSpec` pattern does not transfer, and widening the modifier would be a production edit. The route in is the trait's public `toEntry`, which runs `UnifiedResourceSchema.translateRecord` first — so the record must carry the aliased select fields. The spec fetches the builder's own query to get them. **The LakeFS loopback stub binds at the host and port the config already names**, rather than repointing `StorageConfig.lakefsEndpoint`. That is deliberate and load-bearing: `LakeFSStorageClient`'s `apiClient`/`refsApi`/`objectsApi` are private `lazy val`s that capture the endpoint once per JVM, and amber has `Tags.limit(Tags.Test, 1)` but no `Test / fork`, so the module shares one JVM. `workflow-core` gets away with mutating that global only because it forks its `@NonParallelTest` suites; in amber the same move is order-dependent and would leave the client pointed at a dead port for every later suite. The stub itself is ~90 duplicated lines, because `amber/build.sbt` declares only `DAO % "test->test"` and `Auth % "test->test"`, so `workflow-core`'s stub is not on amber's test classpath. ### Verification 29 mutations. Every survivor is stated below rather than dropped. A selection, weighted toward the ones that pin something a reader would otherwise assume: | Mutation | Killed by | |---|---| | **exchange** the `name` and `description` schema slots | projects every dataset column under the alias its schema slot names | | the same for `is_public`/`is_downloadable`, `did`/`ownerId`, `repository`/… | same test, one assertion per pair | | drop the `creation_time` slot | the projection test, plus a non-null check on the column | | alter the owner join predicate | joins the owner row on the dataset's owner | | drop any one of the three where-clause connectives | ANDs the date, id and keyword filters together | | render only one half of the full-text field list | searches the dataset's name *and* its description | | drop either date bound | the same where-clause test, which pins the rendered range in order | | break the split for any one of `+ - ( ) < > ~ * @ "` | one assertion per reserved character, each in a `withClue` naming it | | construct the entry with the wrong resource type | tags the entry as a dataset and fills the dataset payload slot | **Survivors, stated plainly:** - Replacing `record.into(USER).into(classOf[User])` with a fresh `User` survives. It is an equivalent mutant *given* a production defect — `ownerEmail` is always null — so killing it would need either a production change or an assertion that cements the bug. Neither was done. - Deleting `.filter(_.nonEmpty)` at line 110 survives: `getFullTextSearchFilter` re-filters downstream in `FulltextSearchQueryUtils`. Equivalent. - **Environmental, and worth knowing:** with port 8000 held by another process, the eight mutants behind the stub all become survivors, because their killer tests `cancel`. This was measured, not assumed. CI is deterministic here — the amber unit job that runs `WorkflowExecutionService/jacoco` has no LakeFS, and the `treeverse/lakefs` container at `build.yml:1011` belongs to a different job. But a local run with `bin/local-dev.sh up` will silently disarm those assertions and still go green, so the spec documents that a `canceled N` line is a disarmed suite rather than a skipped nicety. ### Five claims corrected during review Adversarial review found 11 defects in the first draft; three of its suggested fixes were refused as not actually discriminating the mutant they targeted, or as adding a permanently-dead test-only branch. Corrections worth recording because they were stated confidently and wrongly: - "The frontend renders `resourceName`/`resourceDescription` straight from the search response" — **false**. `searchAllResources` consumes every record into a `DashboardClickableFileEntry`; the only grep hit is unrelated. - "pgroonga is never reached, so one fewer unrestored global is needed" — **false**. Dumping the render of the exact call the keyword test makes shows the pgroonga operator present. - "Two of the six branch arms of `dataset.getOwnerUid == uid`" — the jacoco XML says three unreached arms, not two. - An in-test comment claimed an entry-level assertion would catch a schema field wired to the wrong column. It cannot, when the wrong column belongs to another slot — `translateRecord` hides it. - The first draft presented a name/description swap as a hole it had found and fixed. True but incomplete: it closed exactly one pair and left three others, which is why the projection test now carries ten alias assertions instead of two. ### Reported, not pinned `ownerEmail` on the constructed entry is always null. Pinning current behaviour would cement it, so the spec asserts nothing about it. No production file is touched. ### Any related issues, documentation, discussions? Closes #7854 ### How was this PR tested? ``` sbt "WorkflowExecutionService/testOnly org.apache.texera.web.resource.dashboard.DatasetSearchQueryBuilderSpec" ``` ``` [info] Total number of tests run: 18 [info] Suites: completed 1, aborted 0 [info] Tests: succeeded 18, failed 0, canceled 0, ignored 0, pending 0 ``` `canceled 0` means port 8000 was free and the suite ran fully armed. `Test/scalafmtCheck` and `Test/scalafix --check` both pass. ### 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]
