You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@texera.apache.org by gi...@apache.org on 2026/09/05 05:52:17 UTC
(texera) 01/01: test(amber): restore the pgroonga global WorkflowResourceSpec clobbers (#8404)
This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-8404-873c91ce70f15266383e82c2e0f74050692c2b84
in repository https://gitbox.apache.org/repos/asf/texera.git
commit 39e17fd1d1d4eb608300be19a31fc99a8cdc2d46
Author: Xinyuan Lin <xi...@uci.edu>
AuthorDate: Sat Sep 5 05:51:51 2026 +0000
test(amber): restore the pgroonga global WorkflowResourceSpec clobbers (#8404)
### What changes were proposed in this PR?
`FulltextSearchQueryUtils.usePgroonga`
(`amber/src/main/scala/org/apache/texera/web/resource/dashboard/FulltextSearchQueryUtils.scala:32`)
is a JVM-global `var` whose production default is `true`. It is read at
exactly one site, `FulltextSearchQueryUtils.scala:52`, to choose between
the pgroonga arm and the `to_tsvector`/`to_tsquery` fallback.
`WorkflowResourceSpec.beforeAll` forces it `false` and never puts it
back.
amber declares no `Test / fork` — the two `Test / fork := true` settings
a grep finds in `build.sbt` belong to `ComputingUnitManagingService`
(`build.sbt:191`) and `FileService` (`build.sbt:227`), while the amber
project (`WorkflowExecutionService`, `build.sbt:252`) declares neither
`fork` nor `parallelExecution` — and suites are serialized by
`Tags.limit(Tags.Test, 1)` (`amber/build.sbt:48`). One
`WorkflowExecutionService/test` run is therefore a single JVM sharing a
single flag: 193 completed suites in the run measured here.
```
one WorkflowExecutionService/test JVM -- unforked, suites serialized
... -> WorkflowResourceSpec -> ...
beforeAll: usePgroonga = false
afterAll : (before) nothing every later suite
(after) usePgroonga = <captured> renders the fallback
arm, not production's
```
This suite genuinely needs the `false` arm, so the write is restored
rather than deleted: the embedded Postgres it runs against has no
pgroonga extension, and with the flag left `true` 11 of its 78 tests
fail with `ERROR: function pgroonga_condition(unknown,
fuzzy_max_distance_ratio => numeric) does not exist`. The 11 are named
under *How was this PR tested?*.
The capture is taken **in `beforeAll`, immediately before the write**,
and written back as the **first** statement of `afterAll`, ahead of
`closeConnectionPool()`. That ordering is hygiene, not a fix for a live
hazard: `MockTexeraDB.closeConnectionPool`
(`common/dao/src/test/scala/org/apache/texera/dao/MockTexeraDB.scala:183-192`)
already swallows any `Exception` itself (`catch { case e: Exception =>
e.printStackTrace() } finally { ... }`), so only an `Error` could escape
it today. Putting the restore first means the ordering does not depend
on that staying true.
The alternative — capturing into a `val` at construction time — restores
whatever the flag held when sbt happened to instantiate the class, which
is a property of sbt's scheduling rather than of what the suite
clobbered. Both forms were run against the same scenario: an earlier
suite leaves the flag `false`, then this suite runs, then a third suite
reads it.
| capture site | value restored | scenario outcome |
| --- | --- | --- |
| construction time (`private val`) | `true` — the value at
instantiation | 79/80, `true was not equal to false` |
| write time (`private var`, this PR) | `false` — the value it clobbered
| 80/80 |
Honest qualification, since it cuts against this change: that scenario
is built with a nested `org.scalatest.Suites`, which evaluates its
nested suites as **constructor arguments** and therefore instantiates
all of them before any of their tests run. Measured separately,
sbt/ScalaTest does *not* do that for discovered suites — it constructs
each one immediately before running it (order log: `ctor:Bb,
beforeAll:Bb, test:Bb, afterAll:Bb, ctor:Aa, ...`). Nothing touches the
flag between this suite's construction and its `beforeAll`, and there
are zero `extends Suites` classes in `amber/src/test/scala`, so **the
two forms restore the same value in a real `test` run today and are
behaviourally indistinguishable there**. The write-time form is
preferred only because its correctness does not depend on that
measurement continuing to hold: a future `Test / fork`, a
`OneInstancePerTest` mixin, or any nesting suite would each change the
answer. The in-code comment carries that same qualification, so the file
does not overstate the case either.
The field is also initialised from the live value at its declaration.
That is not decoration — `afterAll` was measured to run even when
`beforeAll` throws, so the restore can execute on a path where the
capture line never did, and the initialiser keeps it from writing back
an invented default.
**The restore is unpinned: nothing in the repo would go red if it were
deleted again.** Pinning the arm `false` for an entire module run — a
conservative superset of the leak's effect — leaves the failing-test
identities byte-identical to baseline, so no suite in
`WorkflowExecutionService` currently reads the flag in an arm-sensitive
way. This PR removes latent cross-suite state leakage; it does not fix a
currently-failing test, and no fragile ordering-dependent guard suite
was added to manufacture a pin.
**A comment elsewhere that this change falsifies.**
`DatasetSearchQueryBuilderSpec`'s header comment (`:114-123`) explains
why that suite's keyword assertions are deliberately arm-independent,
and its parenthetical names both writers of the global:
"`DatasetResourceSpec` or `WorkflowResourceSpec` ran earlier in this JVM
and left the global `false` (both set it and neither restores it...)".
After this commit that sentence is false — this suite does restore it.
This PR deliberately does **not** edit that comment. Any narrowing
written here ("only `DatasetResourceSpec` leaks") would itself become
false the moment the sibling PR lands, so the correction belongs in
exactly one place: the sibling (#8403) rewrites that paragraph into a
form that names no suite at all and is therefore true whichever of the
two merges first. If this PR merges first, the comment is stale until
the sibling lands — and the guardrail it states, "anything added here
must keep that property", stays valid in every one of the four states,
so nothing that reads it is misled about what to do.
Writers of the global today:
| writer | restores? |
| --- | --- |
| `FulltextSearchQueryUtilsSpec:64,71,87` | yes — `after { ... }`, per
test |
| `WorkflowResourceSpec:179` | this PR |
| `DatasetResourceSpec:99` | no — deleted outright by #8403, since that
suite never reaches the flag read |
Once both land, this spec is the only writer of the global outside
`FulltextSearchQueryUtilsSpec`. With only **one** of the two applied the
flag's end state differs — with only this PR, `DatasetResourceSpec`
still writes `false` without restoring; with only the sibling, this
suite still leaks `false` — and whichever of the two sbt schedules last
decides it (sbt's order was measured to be neither alphabetical nor
command-line order). *Inference, not a measurement:* since nothing in
the module was found to be arm-sensitive, the failing-test identity set
should not move in either partial state. The sibling-only tree was never
built or run, so that half is reasoning from the arm-sensitivity
measurement rather than an observation.
**Scope.** One test file, +17 lines: the capture, the restore, and the
comment explaining why the capture sits where it does. No `src/main`
file is touched and no other test file is touched. No test is added,
renamed or deleted; the suite's own 78 tests are unchanged and so are
their outcomes.
### Any related issues, documentation, discussions?
Closes #8400
### How was this PR tested?
Everything below is read out of `amber/target/test-reports/TEST-*.xml`,
not the sbt console summary. All probe suites were deleted before
committing; the branch is one commit.
**1. The suite really does need the `false` write.** With `beforeAll`
changed to leave the flag `true`, `WorkflowResourceSpec` alone reports
78 tests and **11 failures**, every one an
`org.jooq.exception.DataAccessException` caused by `ERROR: function
pgroonga_condition(unknown, fuzzy_max_distance_ratio => numeric) does
not exist`. The suite passes keywords through a `getKeywordsArray`
helper at 16 call sites (`:331-751`), one of them with the
reserved-character string `"+-@()<>~*\""`, so it reaches the flag read
for real instead of taking the empty-keywords early return at
`FulltextSearchQueryUtils.scala:45-47`.
```
/search API should be able to search for resources by keyword
/search API should be able to search for workflows in different columns in Workflow table
/search API should be able to search text phrases
/search API should be able to search with arbitrary number of keywords in different combinations
/search API should filter results by different resourceType
/search API should handle multiple keywords correctly
/search API should handle reserved characters in the keywords
/search API should not be able to search workflows from different user accounts
/search API should not return resources that belong to a different user
/search API should return multiple matching resources from a single resource type
/search API should return resources that match any of all provided keywords
```
So the right fix here is a restore, not the deletion the sibling PR
makes.
**2. Red before / green after,** with an explicitly-ordered throwaway
probe — `Suites(new WorkflowResourceSpec, new PgroongaProbeTailSpec)`,
where the tail asserts the global still holds production's default. The
nesting pins the ordering because a plain `testOnly` of two classes does
not order them. Both suites in the **same** invocation, since separate
invocations get fresh classloaders and reset the static:
| tree | result | non-passing identity in the XML |
| --- | --- | --- |
| baseline `1cbe857007` | 78 passed, **1 failed** | `the JVM-global
usePgroonga, after the preceding suite finished should still hold
production's default` |
| with this PR | **79 passed, 0 failed** | none |
| control: probe alone, baseline | 1 passed | none |
The control matters: the tail assertion is not unconditionally red, so
its red above is caused by the leak.
**3. Write-time vs construction-time capture** — the table in the first
section. Same probe shape, with an added first nested suite that leaves
the flag `false`. Both arms were measured on the committed content: the
write-time arm as committed, the construction-time arm by changing only
`var` to `val` and dropping the capture line from `beforeAll`. The
construction-time form restores `true` and clobbers it (XML: `true was
not equal to false` on `usePgroonga after WorkflowResourceSpec should
still be the false the earlier suite left`); the write-time form
restores `false` and the run is 80/80. As stated above, this reflects
the eager instantiation that `Suites` nesting creates rather than sbt's
own (measured lazy) behaviour, so it is a design argument, not a live
bug.
**4. Instantiation and lifecycle semantics,** measured with an
append-only order log from two suites plus one whose `beforeAll` throws:
```
ctor:Bb flag=true <- Bb fully constructed, run and torn down
beforeAll:Bb flag=true
test:Bb flag=true
afterAll:Bb
ctor:Aa flag=true <- only now is Aa constructed
beforeAll:Aa flag=true
test:Aa sets flag=false
afterAll:Aa
throwspec:beforeAll entered, about to throw
throwspec:afterAll RAN <- afterAll runs even when beforeAll throws
```
Four findings: construction is lazy, per suite, immediately before that
suite runs; suites do not interleave; `afterAll` still runs when
`beforeAll` throws (sbt reported `Suites: completed 2, aborted 1` and
the throwing suite's test never logged a line, so the restore can
execute on a path where the capture never did); and `Bb` ran before `Aa`
although `Aa` was listed first on the `testOnly` command line and sorts
first alphabetically.
**5. Arm sensitivity, i.e. why the restore is unpinned.** Production
default mutated `true` -> `false` in `src/main` for one whole module
run, then reverted: 195 report files and **86 non-passing identities,
byte-identical to the baseline list** (`diff` empty). That is a superset
of the leak's effect — every one of the 193 suites ran on the fallback
arm, not just the ones scheduled after this spec — so no suite outcome
in this module depends on the arm. The mutation was reverted and
verified: `git diff --name-only 1cbe857007 -- '*/src/main/*'` is empty
and line 32 reads `var usePgroonga: Boolean = true` again.
**6. Regression,** widest scope that runs locally —
`AMBER_TEST_FILTER=skip-integration sbt WorkflowExecutionService/test`,
baseline (both touched files at their `1cbe857007` content) measured
first. *Counting rule:* a non-passing identity is a `<testcase>` element
in `amber/target/test-reports/TEST-*.xml` carrying a `<failure>`,
`<error>` or `<skipped>` child, printed as `KIND \t suite \t test name`
and sorted; the same rule is applied to both runs.
| run | report files | sbt summary | non-passing identities |
| --- | --- | --- | --- |
| baseline `1cbe857007` | 195 | 2297 tests: 2214 succeeded, 83 failed, 1
canceled, 1 pending; 193 suites completed, 1 aborted | 86 |
| with this PR | 195 | identical, line for line | 86 |
`diff` of the two sorted identity lists is **empty**. The 86 break down
as 84 `<failure>` + 1 `<error>` (the aborted suite's `SuiteSelector`
pseudo-testcase) + 1 `<skipped>`, and the run's own reporter tally
agrees: `Total 2301, Failed 84, Errors 1, Passed 2216, Canceled 1,
Pending 1`. Excluding `<skipped>` the same runs read as 85 rows — the
absolute number is method-dependent, which is why the rule is stated;
the load-bearing fact is that the two lists are byte-identical, not the
count.
All 86 are pre-existing on this box, in 13 suites, none of them touched
by this PR:
| suite | rows | why it is red here |
| --- | --- | --- |
| ResultExportServiceSpec | 17 |
`org.apache.iceberg.exceptions.RESTException` — REST catalog GET to
`localhost:8181`, no Docker on this box |
| DataProcessingSpec | 16 | same catalog GET, wrapped in
`java.lang.Throwable` |
| ExecutionStatsServiceSpec | 12 | same |
| ExecutionResultServiceSpec | 11 | same |
| SyncExecutionResourceSpec | 8 | same |
| InputPortMaterializationReaderThreadSpec | 8 | same (an engine
worker-manager suite, not a dashboard one) |
| PveResourceSpec | 6 | python virtual environment: `Python executable
not found for PVE` |
| ReconfigurationSpec, PauseSpec | 2 + 2 | same catalog GET |
| WorkflowExecutionServiceSpec | 1 | same |
| DefaultCostEstimatorSpec | 1 | the aborted suite: `RESTException` from
the same catalog GET at construction |
| GitVersionControlLocalFileStorageSpec | 1 | local file-tree assertion,
`testFileTreeRetrieval` |
| NetworkOutputBufferSpec | 1 | the `<skipped>` row — a
`pendingUntilFixed` test, not a failure |
`WorkflowResourceSpec`, the only file this PR touches, is green in both
wide runs at `tests="78" errors="0" failures="0"`; so is
`DatasetSearchQueryBuilderSpec` (`tests="24"`), the spec whose header
comment describes this leak. The change alters what the suite leaves
behind, not what it asserts.
**7. Lint** (CI gates): `WorkflowExecutionService/scalafmtCheck`,
`WorkflowExecutionService/Test/scalafmtCheck` and
`WorkflowExecutionService/scalafixAll --check` all report `[success]` on
the committed tree.
**Corrections made after review.** Four claims in an earlier draft of
this description were wrong or unstated, and are corrected above rather
than quietly dropped: (a) the restore-before-teardown ordering was
justified as protecting against a teardown throw, but
`closeConnectionPool` catches `Exception` itself, so the justification
is hygiene only; (b) the non-passing identity count is counting-method
dependent (the same runs read as 85 rows if `<skipped>` is excluded), so
the rule is now stated; (c) the pre-existing local failures were
described as Iceberg/Docker dashboard suites, which under-describes them
— see the table above; and (d) this PR falsifies a sentence in
`DatasetSearchQueryBuilderSpec`'s header comment, which the earlier
draft cited as supporting evidence without disclosing that it goes
stale; An earlier revision of this branch corrected that comment here;
the edit has been reverted, because the narrowing it wrote would go
false as soon as the sibling landed. The correction now lives only in
the sibling PR, in an order-neutral form, and this PR discloses the
staleness instead.
### Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 5)
---
.../resource/dashboard/file/WorkflowResourceSpec.scala | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/amber/src/test/scala/org/apache/texera/web/resource/dashboard/file/WorkflowResourceSpec.scala b/amber/src/test/scala/org/apache/texera/web/resource/dashboard/file/WorkflowResourceSpec.scala
index 1816ce9cd8..88f429a3e6 100644
--- a/amber/src/test/scala/org/apache/texera/web/resource/dashboard/file/WorkflowResourceSpec.scala
+++ b/amber/src/test/scala/org/apache/texera/web/resource/dashboard/file/WorkflowResourceSpec.scala
@@ -160,8 +160,22 @@ class WorkflowResourceSpec
new DashboardResource()
}
+ // `FulltextSearchQueryUtils.usePgroonga` is a JVM-global `var` and amber's tests are
+ // unforked, so every suite in one `test` invocation shares it. This spec must force it
+ // `false` — its embedded Postgres has no pgroonga extension, and the `true` arm fails 11
+ // of the search tests with `function pgroonga_condition(...) does not exist`. Capture the
+ // live value in `beforeAll` right before the write, so what is put back is exactly the
+ // value this suite clobbered. Capturing at construction instead would put back whatever
+ // the flag held when sbt instantiated this class; under sbt today that is the same value,
+ // since a suite is constructed immediately before it runs and nothing touches the flag in
+ // between — but that is a property of sbt's scheduling, not of this suite, and it stops
+ // holding under eager construction (a nesting `Suites`, `OneInstancePerTest`, forking).
+ // The initialiser below is only so the field never holds an invented default.
+ private var pgroongaBeforeWrite: Boolean = FulltextSearchQueryUtils.usePgroonga
+
override protected def beforeAll(): Unit = {
initializeDBAndReplaceDSLContext()
+ pgroongaBeforeWrite = FulltextSearchQueryUtils.usePgroonga
FulltextSearchQueryUtils.usePgroonga = false // disable pgroonga
// add test user directly
val userDao = new UserDao(getDSLContext.configuration())
@@ -195,6 +209,9 @@ class WorkflowResourceSpec
}
override protected def afterAll(): Unit = {
+ // Restore before the teardown below, so this does not depend on `closeConnectionPool`
+ // staying non-throwing (today it swallows any `Exception` itself).
+ FulltextSearchQueryUtils.usePgroonga = pgroongaBeforeWrite
closeConnectionPool()
}