The GitHub Actions job "Required Checks" on texera.git/main has succeeded. Run started by GitHub user github-merge-queue[bot] (triggered by github-merge-queue[bot]).
Head commit for run: f8e3e9ef082f69b6409371502706b0af9b4d62ff / Xinyuan Lin <[email protected]> test(auth, amber): cover the activity tracker upsert and the amber home path (#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) --------- Signed-off-by: Xinyuan Lin <[email protected]> Co-authored-by: Copilot Autofix powered by AI <[email protected]> Report URL: https://github.com/apache/texera/actions/runs/32618752690 With regards, GitHub Actions via GitBox
