aglinxinyuan opened a new pull request, #7694:
URL: https://github.com/apache/texera/pull/7694
### What changes were proposed in this PR?
None of the five Dropwizard service entry points had a spec for
`initialize()`, which left them the least-covered files in their own modules.
Adds one `*RunSpec` per service.
| File | Before | After |
|---|---|---|
| `WorkflowCompilingService.scala` | 48.6% | **29/37 (78.4%)** |
| `AccessControlService.scala` | 37.9% | **21/29 (72.4%)** |
| `ConfigService.scala` | 36.7% | **22/30 (73.3%)** |
| `NotebookMigrationService.scala` | 46.9% | **24/32 (75.0%)** |
| `ComputingUnitManagingService.scala` | 53.6% | **20/28 (71.4%)** |
Across the five: **~44.9% -> 74.4%**, and 15 -> 35 tests. Nothing binds a
port or starts a server -- `new Bootstrap[Configuration](new Service)` only
builds an object mapper and a file source provider.
Covered: environment-variable substitution including the `:-` default
delimiter, the substituting source provider's delegate, the Scala module on
Dropwizard's object mapper (with a `readValue` round trip), the connection
pool's JDBC URL and the lifecycle phase that opens it, and the request-logging
filter's registration, dispatch set, and forward-and-log behaviour.
### Verification
The build applied 18 mutations, all red. Review then proposed 15 more; each
was **run** before anything was concluded. Outcome: 11 fixed and proven red on
a named test, 2 recorded as unpinnable with evidence, 2 refuted.
Three are worth spelling out.
**An assertion that was carried by an accidental exception.** The
substitution test asserted that an unset variable survives as a literal -- but
with a *strict* substitutor the throw happens inside `open()` on the preceding
setup line, so the matcher was never reached. Proof: under
`EnvironmentVariableSubstitutor(false) -> (true)` the substitution test now
**passes**, and a separate new test, "leave a variable with neither a value nor
a default as a literal", is what goes red. The strictness claim now has its own
assertion instead of riding on an exception.
**A lifecycle claim that needed the effect pinned, not the phase.**
Faithfully relocating the whole `SqlServer.initConnection` block from
`initialize()` into `run()` now fails "initialize should open a connection pool
against the configured JDBC URL" with `Some(false) was not equal to
Some(true)`. `Some(false)` rather than `None` is the point: an earlier `run()`
test had already installed a pool, so a URL-only assertion would have passed --
it is the SqlServer-identity check that kills it.
**A refuted finding.** `EnvironmentVariableSubstitutor(false) -> (false,
true)` survives, but it is an equivalent mutant over every config this repo
ships: `StringSubstitutor` only differs on those arguments when a `${` appears
*inside* a variable expression, and no nested form exists anywhere in the tree.
Adding a `${${...}}` fixture would cement syntax nothing uses, so argument 2 is
left deliberately unpinned.
Two suggestions were also declined with reasons: asserting
`databaseReachable shouldBe true` when CI env vars are set (it makes the spec
environment-aware and breaks for developers without local Postgres), and
asserting `FAIL_ON_UNKNOWN_PROPERTIES` (`initialize()` never touches it, so it
pins a Dropwizard default rather than our code).
### One thing these tests cannot pin
`storage.conf:173-178` ships `username == password == "postgres"`, and CI
authenticates the default superuser with that same password. Swapping the last
two arguments of `SqlServer.initConnection` therefore survives here --
measured, 6/6 green -- and so does hard-coding either to the literal. The
test's claim is narrowed accordingly (it now says "against the configured JDBC
URL"). Killing it needs a `common/dao` spec with a purpose-built role where
user != password; that is out of scope for a test-only PR on these five files.
### Deliberately not included
`main()` on all five: `Application.run(String...)` binds a real port, and
dropwizard-core's `onFatalError` calls `System.exit(1)`, which would kill the
shared sbt test JVM.
Two defects are reported rather than pinned:
- **Lifecycle inconsistency.** Three of eight entry points open the
connection pool in `initialize()` (`AccessControlService:50`,
`ConfigService:48`, `NotebookMigrationService:53`) while five do it in `run()`.
Since `Application.run` calls `initialize()` before the CLI parses arguments,
`check` or `--help` on those three requires a live Postgres and opens a
10-connection pool that nothing ever closes. The spec comment says explicitly
that the current phase is recorded, not endorsed.
- **`WorkflowCompilingService.scala:75-96` duplicates
`RequestLoggingFilter`** from `common/auth`, which the other four services call
via `RequestLoggingFilter.register`. The new tests read the filter back out of
the captured `FilterHolder` rather than by class, so that refactor would leave
them green.
No production file is touched.
### Any related issues, documentation, discussions?
Closes #7693
### How was this PR tested?
```
sbt "WorkflowCompilingService/test" "AccessControlService/test"
"ConfigService/test" "NotebookMigrationService/test"
"ComputingUnitManagingService/test"
```
```
[info] Tests: succeeded 13, failed 0, canceled 0, ignored 0, pending 0
[info] Tests: succeeded 6, failed 0, canceled 0, ignored 0, pending 0
[info] Tests: succeeded 42, failed 0, canceled 0, ignored 0, pending 0
[info] Tests: succeeded 33, failed 0, canceled 0, ignored 0, pending 0
[info] Tests: succeeded 119, failed 0, canceled 0, ignored 0, pending 0
```
Zero canceled, which is worth checking explicitly: sbt's JUnit XML does not
mark ScalaTest cancellations, so a spec that silently cancels reads as green in
the XML. `Test/scalafmtCheck` and `Test/scalafix --check` pass on all five
projects.
### 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]
