aglinxinyuan opened a new pull request, #7845:
URL: https://github.com/apache/texera/pull/7845
### What changes were proposed in this PR?
Three small targets, bundled because each is a few lines with no coverage.
10 tests added (52 → 62 across the three specs).
All figures below were measured with `<Module>/jacoco` under a
`Tests.Filter` naming only the spec(s) in question, the same filter for the
before and after run, parsing `jacoco.xml` per `<sourcefile>`. Two numbers per
file, because they differ: JaCoCo line-hit (any instruction covered) and the
Codecov metric (fully-covered lines, so a line with any missed branch arm
counts against you).
| File | JaCoCo line-hit | Codecov metric |
|---|---|---|
| `UserActivityTracker.scala` | 47/56 = 83.9% → **56/56 = 100%** | 42/56 =
75.0% → **49/56 = 87.5%** |
| `Utils.scala` | 58/69 = 84.1% → **66/69 = 95.7%** | 38/69 = 55.1% →
**44/69 = 63.8%** |
| `WorkflowActor.scala` | 83/89 = 93.3% → **88/89 = 98.9%** | 48/89 = 53.9%
→ **52/89 = 58.4%** |
All 9 previously-missed lines of `UserActivityTracker` (151-157, 159, 160 —
the whole of the private `defaultUpsert`) are covered, plus the two
fatal-escapes-the-catch arms at 89 and 103.
**Two honest caveats on those numbers.** First, `WorkflowActor`'s apparent
gain is inflated by the narrow filter: against a full CI amber run only lines
236 and 237 are new, because 231-233 are already covered there by real workers.
Second, `UserActivityTracker`'s Codecov metric carries about ±2 lines of
run-to-run noise, because the 16-thread CAS race at lines 77 and 79 does not
always cover both arms — do not read a 2-line drop on this file as a regression.
### What adversarial review changed, because it caught a test that pinned
nothing
The first draft of the `Utils` test asserted `amberHome.getFileName.toString
== Utils.AMBER_HOME_FOLDER_NAME`. That is tautological: production selects the
directory *using* that constant, so mutating it to `"common"` moves both sides
of the equality together and the test stays green — `./common` also exists at
depth 1, so the walk finds it and the two supporting legs (strict descendant,
not equal to cwd) are satisfied by any descendant. It was the assertion the
whole target rested on, and it constrained nothing.
It now asserts the literal `"amber"`, with the constant pinned separately by
its own one-line test. Mutating the constant kills both.
### Verification
33 mutations, **28 killed, 5 survivors**, all 5 stated below. Each was
applied one at a time from a scratch-dir snapshot with an applier that aborts
unless the anchor occurs exactly once, reverted by copying the snapshot over
the one exact file path, with the production diff and an md5 re-checked after
every revert.
A selection — the full table is long, so these are the ones that pin
something a reader would otherwise assume:
| Mutation | Killed by |
|---|---|
| `AMBER_HOME_FOLDER_NAME` `"amber"` → `"common"` | the constant test
**and** the `getFileName` literal leg |
| negate the `isAmberHomePath(cwd)` early-return guard | resolves to an
`amber` directory beneath the working directory |
| `.filter(isAmberHomePath)` → `.filter(_ => true)` | same test |
| walk root `Paths.get(".")` → `.getParent` | the strict-descendant leg
(environment-sensitive — see below) |
| **reorder** `preStart` to initState / register / initialize | the recorded
event sequence |
| **reorder** `preStart` to hoist register above `initState` | the failure
case's `expectNoMessage` on the parent probe |
| `RegisterActorRef(actorId, …)` →
`ActorVirtualIdentity(context.self.path.name)` | the identity assertion, after
the fixture stopped reusing one literal for both |
| `context.parent !` → `context.self !` | parent probe times out |
| `postStop`'s `transferService.stop()` → `()` | the post-stop
handle-cancellation assertion |
| `NonFatal` → `Throwable` at each of the three catch sites | three
fatal-escape cases, one per site |
| `NonFatal` → `RuntimeException` at each of the three catch sites | three
checked-`IOException` shapes, one per site |
| `OffsetDateTime.ofInstant(ts, UTC)` → `now(UTC)` (discard `ts`) | the
queued-write case — **and nothing else**, see below |
| `.set(UID, uid)` → `uid + 1` | both singleton cases, each on its own
assertion |
| VALUES arm and DO UPDATE arm timestamps, separately | insert case and
overwrite case respectively — the arms are orthogonal |
| delete `.onConflict(UID).doUpdate()` | the overwrite case |
| cooldown comparison `< 0` → `<= 0`, eviction `×2` → `×3`, `isBefore` →
`!isAfter` | the interval and eviction boundary cases |
Three of these are worth calling out because the obvious version of the test
does not kill them:
- **Discarding the upsert's `ts` argument survived a tightened wall-clock
window.** The single-threaded writer picks the task up inside the clock's own
granularity — measured `beforeCall == afterCall` to the tick on this machine.
Killing it needed the stall the property exists for: an exclusive lock on
`texera_db.user_last_active_time` held on a raw connection *outside* the Hikari
pool the writer borrows from, which blocks one insert so the writer cannot
reach the next task for 1.5s. If that case is ever deleted, this mutation goes
straight back to surviving.
- **The three `NonFatal` catch sites needed a checked non-fatal**, since a
suite where every non-fatal thrower is a `RuntimeException` lets `case
NonFatal(e)` → `case e: RuntimeException` survive everywhere. The third site
(inside the executor task) also needed a capturing executor to be observable at
all — an `IOException` from `upsertFn` on a same-thread executor is caught one
level up by `markActive`, so the caller sees nothing either way.
- **The fixture uids moved to 8810/8820/8830/8840.** With adjacent uids, the
off-by-one mutation redirected one case's write onto the other case's seed row,
and the resulting "kill" was really a test aborting on its own clobbered
precondition.
### The five survivors, and the equivalent mutants among them
- **`Files.walk(cwd, 2)` → `1` survives.** Unpinnable from a test:
`amberHomePath` is a `lazy val` reading the process working directory, so one
JVM takes one branch, and `isAmberHomePath` is private. `amber/` is a direct
child of the repo root, so depth 1 still finds it. Only depth 0 dies, and it
dies through production's own exception rather than any assertion about depth.
Both suggested fixes needed a production visibility change, so neither was
taken; a comment in the spec records this so the depth-0 kill is not mistaken
for a depth pin.
- **Dropping `.toRealPath()` from `isAmberHomePath` survives** — every
`Files.walk` result is already rooted at a real path. Pinning the symlink
intent would need a temp symlink tree.
- **Deleting `preStart`'s entire try/catch survives, and is an equivalent
mutant.** The `IllegalStateException` leaves `preStart` either way, Pekko wraps
it in `ActorInitializationException` either way, the default decider Stops
either way, `postStop` runs either way, and `Terminated` arrives either way.
Asserting on the exception's cause does not discriminate it either — the cause
is identical with and without the catch. So `throw t` is pinned *given the
catch exists*, and nothing more than that is claimed.
- **Deleting the catch's `logger.warn` survives** — a strict subset of the
above. Line 236 is line-hit only, never verified, and is stated as such in the
spec.
- **Deleting `markActive`'s `if (uid == null) return` survives.**
`ConcurrentHashMap.get(null)` throws an NPE, which is non-fatal, so the outer
catch swallows it and the caller still observes a no-op with no write and no
cooldown entry. Only the log noise differs. Reported rather than repaired for
that reason; it was untried by the first pass and found while re-deriving the
table.
One kill is environment-sensitive and says so: retargeting the walk root at
`cwd.getParent` dies here only because sibling worktrees contain their own
`amber/` directories. In a checkout whose parent has no sibling `amber`, the
walk would return `<checkout>/amber`, which still starts with cwd, and the
mutant would survive.
### Deliberately not included
`WRITE_INTERVAL` (5 minutes), `WRITER_QUEUE_CAPACITY` (256), the
`DiscardOldest` policy and the periodic eviction scheduler are unverified — the
singleton fixture is deliberately built *around* the throttle by giving each
case its own uid. So `UserActivityTracker.scala` reaching 100% line-hit should
not be read as 100% verified, and the fixture carries a comment saying so.
`WorkflowActor.scala:47` (the companion's module initializer) remains
uncovered; nothing here can reach it.
One inherent fragility, recorded in the fixture: the singleton cases assume
no other `Auth` suite calls `UserActivityTracker.markActive` for uids
8810/8820/8830/8840, since the cooldown is per-uid and per-JVM and a prior call
would suppress the write. That is unavoidable when testing a JVM singleton with
a 5-minute throttle.
No production file is touched.
### Any related issues, documentation, discussions?
Closes #7844
### How was this PR tested?
```
sbt "Auth/testOnly org.apache.texera.auth.UserActivityTrackerSpec"
```
```
[info] Tests: succeeded 28, failed 0, canceled 0, ignored 0, pending 0
```
```
sbt "WorkflowExecutionService/testOnly
org.apache.texera.amber.engine.common.UtilsSpec
org.apache.texera.amber.engine.architecture.common.WorkflowActorSpec"
```
```
[info] Tests: succeeded 34, failed 0, canceled 0, ignored 0, pending 0
```
The whole `Auth` module is green at 105 tests across 10 suites, which is the
check that matters here — it confirms the new `MockTexeraDB` mixin does not
disturb the sibling suites sharing the JVM and the `SqlServer` singleton.
`Test/scalafmtCheck` and `Test/scalafix --check` pass for both modules.
### 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]
