diff --git a/AGENTS.md b/AGENTS.md index 3707fd0..a3d9bb6 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -90,16 +90,16 @@ accepted the prerequisite order `E3-06 -> E3-01 -> E0-07` in `D-42`. `E3-01` is `E0-07`, the walking skeleton, `E1-02`, the Vehicle domain story, `E1-03`, the Vehicle data story, `E1-04`, the Fuel Entry domain story, and `E1-05`, the human-gated consumption calculation story, `E1-06`, Fuel Entry data, `E1-07`, Android Vehicle UI, `E1-08`, Android Fuel Entry UI, and -`E1-09`, iOS UI: Vehicles and Fuel Entries, `E1-10`, Settings, and `E1-11`, the Vehicle outbox -payload `entityType` fix, are complete. `E1-13` owns the +`E1-09`, iOS UI: Vehicles and Fuel Entries, `E1-10`, Settings, `E1-11`, the Vehicle outbox +payload `entityType` fix, and `E1-12`, the shared-test graph-close race fix, are complete. `E1-13` owns the executable iOS locale-provider behavior coverage gap recorded by the E1-10 review. ### Delivery status and remaining work -- **Completed:** all Phase 0 stories; `E1-01` through `E1-10`; the pulled-forward `E3-06` and +- **Completed:** all Phase 0 stories; `E1-01` through `E1-12`; the pulled-forward `E3-06` and `E3-01` prerequisites; and the `E0-07` walking-skeleton gate, including D-73 cleanup evidence. -- **Next:** `E1-12`, `FuelEntryStateHolderTest` Kotlin/Native SIGSEGV on graph close. -- **Remaining Phase 1:** `E1-12`, `E1-13`. +- **Next:** `E1-13`, executable iOS locale-provider behavior coverage. +- **Remaining Phase 1:** `E1-13`. - **Remaining Phase 2:** `E2-01`, `E2-02`, `E2-03`, `E2-06`, `E2-04`, `E2-07` and `E2-05`. - **Remaining Phase 3:** `E3-10`, `E3-11`, `E3-02`, `E3-03`, `E3-08`, `E3-04`, `E3-12`, `E3-05`, `E3-07` and `E3-09`. `E3-01` and `E3-06` are already complete. diff --git a/README.md b/README.md index 2e53384..5a2ae8a 100644 --- a/README.md +++ b/README.md @@ -4,7 +4,7 @@ Cross-platform mobile app for Android and iOS to track vehicle costs. The MVP is intentionally limited to **fuel expenses**: users can create vehicles, log refueling events, review their history, and calculate real-world fuel consumption in **L/100 km**. Later phases may add maintenance, insurance, taxes, and other expense types, but they are out of scope for the MVP. -> **Project status:** **Phase 1 open.** `E1-01` through `E1-08`, `E3-06`, `E3-01` and `E0-07` have delivered the SQLDelight database, complete local Vehicle and Fuel Entry repositories, Fuel Entry validation, reviewed full-to-full consumption, executable provider decoupling, reviewed Firestore rules, the native walking skeleton and the Android Vehicle and Fuel Entry flows. `E1-09`, iOS UI, is next. The project builds on both platforms and `main` is protected by ten required CI checks. +> **Project status:** **Phase 1 open.** `E1-01` through `E1-12`, `E3-06`, `E3-01` and `E0-07` have delivered the SQLDelight database, complete local Vehicle and Fuel Entry repositories, Fuel Entry validation, reviewed full-to-full consumption, executable provider decoupling, reviewed Firestore rules, the native walking skeleton, both native Vehicle and Fuel Entry flows, device-local settings, and deterministic shared graph-test teardown. `E1-13`, executable iOS locale-provider behavior coverage, is next. The project builds on both platforms and `main` is protected by ten required CI checks. ## Start here @@ -177,7 +177,7 @@ document wins and the discrepancy is escalated. | Phase | Goal | Main gate | |-------|------|-----------| | 0 - Foundations **(complete)** | Owner decisions closed, KMP skeleton, convention plugins, core modules, quality tooling, CI, ADRs | Android and iOS build in CI; architecture rules fail correctly | -| 1 - Local persistence **(active)** | `E1-01` through `E1-11` and the `E0-07` gate are complete; `E1-12` and `E1-13` remain | The walking skeleton proves the native local/remote path, both native Vehicle and Fuel Entry flows are executable, device-local settings are persistent, and the Vehicle outbox payload `entityType` contract compliance is restored | +| 1 - Local persistence **(active)** | `E1-01` through `E1-12` and the `E0-07` gate are complete; `E1-13` remains | The walking skeleton proves the native local/remote path, both native Vehicle and Fuel Entry flows are executable, device-local settings are persistent, Vehicle outbox payload `entityType` compliance is restored, and shared graph tests tear down deterministically | | 2 - Authentication **(planned)** | E2-01 through E2-07 remain | Adoption and normal linking preserve data; confirmed collisions preserve the current anonymous snapshot | | 3 - Backend and backup **(partially complete)** | E3-06 and E3-01 are complete; the remaining backend, cleanup, sync and recovery stories are open | Recovery and provider decoupling are executable checks | | 4 - MVP hardening **(planned)** | E4-01 through E4-04 remain | Store-readiness checklist complete | diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 6b98a85..0880464 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -504,7 +504,8 @@ Blocks: E2-06. ### E1-12 - `FuelEntryStateHolderTest` Kotlin/Native SIGSEGV on Graph Close - S -Status: open. Registered as GitHub issue #42. Tracked as a follow-up of `E1-08` introduced by +Status: completed on 2026-09-02. See `docs/handoff-E1-12.md`. Registered as GitHub issue #42. +Tracked as a follow-up of `E1-08` introduced by commits `a5150d4`, `87abdd0`, `ba8823a`. This story is a test-infrastructure defect, not a production defect: no production code path closes an `AppGraph` while its state holders are still collecting. @@ -1090,8 +1091,8 @@ proof after E3-04. | E1-08 Android UI fuel entries (completed) | 1 | L | — | | E1-09 iOS UI (completed) | 1 | L | — | | E1-10 Settings persistence (completed) | 1 | S | — | -| E1-11 `:feature:vehicle` outbox payload entityType fix | 1 | S | — | -| E1-12 `FuelEntryStateHolderTest` Kotlin/Native SIGSEGV on graph close (issue #42) | 1 | S | Yes | +| E1-11 `:feature:vehicle` outbox payload entityType fix (completed) | 1 | S | — | +| E1-12 `FuelEntryStateHolderTest` Kotlin/Native SIGSEGV on graph close (completed; issue #42) | 1 | S | Yes | | E1-13 Executable iOS locale-provider behavior coverage | 1 | S | Yes | | E2-01 `:core:auth` | 2 | S | — | | E2-02 Firebase Auth integration | 2 | L | Yes | diff --git a/docs/PROJECT_LOG.md b/docs/PROJECT_LOG.md index 6d63c9f..3371d1b 100644 --- a/docs/PROJECT_LOG.md +++ b/docs/PROJECT_LOG.md @@ -38,6 +38,59 @@ ## Entries +### 2026-09-02 — E1-12 review corrections: orphaned harness Job and exception-safe teardown + +- **Type:** correction +- **Story / Decision:** `E1-12` / — +- **Author:** opencode (GLM), on behalf of David Ruiz +- **What changed:** corrects the E1-12 story entry above on two points. First, `AppGraphTestHarness` + created `scopeJob` before validating that `parentScope` contained a `TestCoroutineScheduler`, so a + failed construction left an orphaned child `Job` on the parent; the scheduler is now resolved and + validated first, and the constructor test proves no child remains attached. Second, the harness + test teardowns now use nested `try/finally` so `owningFactory.close()` always runs, and the + obsolete `DatabaseFactory` / `DatabaseHandle` imports were removed from `AppGraphCloseTest.kt`. +- **Why:** the PR #49 review round found that a failed harness construction leaked a `Job` into the + parent scope and that a throwing `harness.close()` or `graph.close()` could skip the owning + factory close. Both are test-infrastructure defects inside E1-12 scope; production D-89 behavior + is untouched. +- **Correction of record:** the original E1-12 entry reports 30 `:shared` tests per target, which + was accurate at its time. The two harness tests added by the review rounds raise the final count + to 32 tests, 0 failures, 0 skipped on each of `:shared:testAndroidHostTest` and + `:shared:iosSimulatorArm64Test`. The original entry is otherwise unchanged. +- **Documents touched:** `docs/handoff-E1-12.md`, this log, and the E1-12 `:shared` common-test + files listed in the handoff. +- **Verification:** extended constructor test RED against the old implementation + (`[SupervisorJobImpl{Active}]` attached to the parent), GREEN after the fix; focused shared + verification passed 32 tests per target; the complete non-instrumented command passed; + `contractCheck` reports no unresolved decisions and no `PENDING` assertions; `git diff --check` + clean; the `backgroundScope` source audit still finds no direct state-holder collector launch. +- **Follow-ups / risks:** none new. PR #49 still requires human review and must not be merged by + an agent. + +### 2026-09-02 — E1-12 shared graph-test teardown made deterministic + +- **Type:** story +- **Story / Decision:** `E1-12` / — +- **Author:** Codex, on behalf of David Ruiz +- **What changed:** added a reusable `AppGraphTestHarness` that owns a child coroutine scope, + launches state-holder collectors eagerly and cancels and joins the complete scope before closing + its graph. Migrated every Kotlin caller-owned graph holder scope in `:shared` tests, removed all + direct `backgroundScope.launch` state-holder collectors and audited every test class that mounts + an `AppGraph`. +- **Why:** `runTest` cancels `backgroundScope` after the test body returns, so closing the graph in a + `finally` block could release the native SQLite driver while test-owned collectors were still + subscribed. Kotlin/Native could then abort with signal 11 instead of reporting a test result. +- **Documents touched:** `AGENTS.md`, `README.md`, `docs/BACKLOG.md`, + `docs/handoff-E1-12.md`, this log, and the E1-12 `:shared` common-test files listed in the handoff. +- **Verification:** deterministic RED proved the missing ordering; all 30 `:shared` tests pass on + Android host and `iosSimulatorArm64`; the Native suite passed 10/10 consecutive forced local runs + on Apple silicon; the complete non-instrumented command from `AGENTS.md` passed with 627 + actionable tasks; `contractCheck` reports 111 aligned decisions and ADRs with no unresolved or + pending assertions. +- **Follow-ups / risks:** the PR requires human review and repeated macOS CI evidence before merge. + Production hardening of `AppGraph.close()` against live external subscribers remains explicitly + deferred to a separate story because it would change D-89 and touch gated `core/database/**`. + ### 2026-09-02 — E1-11 parity test corrected: it proved cascade-then-cascade, not cascade-then-direct - **Type:** correction diff --git a/docs/handoff-E1-12.md b/docs/handoff-E1-12.md new file mode 100644 index 0000000..1b428f6 --- /dev/null +++ b/docs/handoff-E1-12.md @@ -0,0 +1,330 @@ +# Agent Handoff - E1-12 + +## Story + +`E1-12 - FuelEntryStateHolderTest Kotlin/Native SIGSEGV on Graph Close - S` + +## Ready Check + +- Backlog story: explicit and open in `docs/BACKLOG.md`; GitHub issue #42 contains the diagnosed + failure sequence and reproduction evidence. +- Acceptance criteria reviewed: make `:shared:iosSimulatorArm64Test` deterministic by moving every + `:shared` test-owned state-holder scope and collector under a reusable child scope that is + cancelled before its `AppGraph` closes; audit every `:shared` test that mounts an `AppGraph`; + keep the fix in test code; record the repeated-run count. +- Dependencies checked: E1-08 introduced the affected tests and is complete. E1-12 has no + dependency on E1-11; E1-11 is now merged into the `main` base at `64c91d6`. No other story is in + flight in this working tree. +- Decisions checked: D-75, D-86, D-89 and D-106 are `Accepted` and govern the Native test route, + graph separation, database-handle lifetime and graph bootstrap cancellation. The decision board + contains no `Proposed` or `Pending` rows. The story explicitly requires no new decision. +- Normative sections reviewed: `docs/SPECIFICATION.md` §8.4 and §11; + `docs/CONTRACTS.md` §11.6, §14 and §20.10; `docs/TECHNICAL_PLAN.md` §4 and §12; ADR-0090 / D-89; + `AGENTS.md` Definition of Ready, Continuous Progress Documentation, Definition of Done and Human + Review Gates. +- Expected verification: a focused RED test that deterministically proves cancellation must + precede graph close; focused `:shared:testAndroidHostTest` and `:shared:iosSimulatorArm64Test`; + repeated Apple-silicon `:shared:iosSimulatorArm64Test` runs; the complete non-instrumented command + from `AGENTS.md`; `git diff --check`; and all required pull-request checks. +- Human review gates identified before work: applies because the E1-12 backlog story explicitly + requires human review. The change must remain test-only and the agent MUST NOT merge the pull + request. +- Rule 0 acknowledged: owner conversation is Spanish (Spain); every repository artifact, branch, + commit and pull-request field is technical English. +- TDD workflow: the owner's requested `NETWORK -> GREEN -> REFACTOR -> one push -> PR` sequence is + interpreted as `RED -> GREEN -> REFACTOR -> one push -> PR`, because E1-12 introduces no network + behavior and the remaining terms exactly match the repository TDD workflow. The requested single + push after the three local commits explicitly supersedes the default push-after-each-phase rule + for this story. + +## In-Progress Checkpoint + +- Date: 2026-09-02. +- Branch and base: `story/E1-12-shared-test-graph-close-race`, based on `main` / `origin/main` at + `64c91d6` (merged PR #48). +- Current phase and latest commit: RED `ed698e0`, GREEN `55f075c`, REFACTOR `a7fb619`, the + pull-request continuity checkpoints `b23b4d2` and `a4009df`, the review-fix dispatcher + correction `27030d6`, the shared-factory hardening `fa69083`, the review-findings record + `eb8755f`, the second review round RED `5b9cb46`, GREEN `92bc607` and REFACTOR `4d86f82`, + the review documentation `9258821`, the first CI-evidence checkpoint `f62516d` and this + final CI-evidence checkpoint are complete. All phases are finished; this final + continuity-record commit is the story's final HEAD, and the last code-bearing commit is + `4d86f82`. +- Push and pull-request status: every commit, including this final checkpoint, is pushed. All + ten required checks passed on the code-identical commits `9258821`, `f62516d` and `6776810` + in runs `https://github.com/davidru85/carApp/actions/runs/33667021311`, + `https://github.com/davidru85/carApp/actions/runs/33669085132` and + `https://github.com/davidru85/carApp/actions/runs/33676703761`; the push of this final + documentation-only checkpoint triggers the same ten checks on identical code, and its run + is recorded in the PR #49 description. PR #49 is open at + `https://github.com/davidru85/carApp/pull/49` and MUST NOT be merged by the agent. +- Completed since the previous checkpoint: resolved the second review round of PR #49. + Extended `constructorThrowsWhenParentScopeHasNoTestCoroutineScheduler` to retain the parent + `Job` explicitly and assert that a failed construction leaves no orphaned child attached + (confirmed RED: `SupervisorJobImpl{Active}` remained attached). Moved the scheduler + resolution and validation in `AppGraphTestHarness` before the `scopeJob` creation so a + missing `TestCoroutineScheduler` fails before any `Job` is created. Made every teardown in the + harness tests exception-safe with nested `try/finally` so `owningFactory.close()` always + runs, and removed the obsolete `DatabaseFactory` and `DatabaseHandle` imports from + `AppGraphCloseTest.kt`. Valid harness behavior is unchanged: collectors remain children of + `scopeJob`, run eagerly through `UnconfinedTestDispatcher` and are cancelled and joined + before `graph.close()`. +- Verification evidence and known failures: the extended constructor test failed as expected + before the fix (BUILD FAILED; expected `[]` children but observed + `[SupervisorJobImpl{Active}]`). After the fix, focused `:shared:testAndroidHostTest` + harness tests passed (3/3), and `./gradlew :shared:testAndroidHostTest + :shared:iosSimulatorArm64Test :shared:ktlintCheck :shared:detekt --rerun-tasks` passed with + 32 tests and 0 failures/skips on each target. The complete non-instrumented repository + command passed with 627 actionable tasks; `contractCheck` reported no unresolved decisions + and no `PENDING` assertions; `git diff --check` is clean; the source audit still finds no + direct `backgroundScope.launch` state-holder collector and no state-holder factory receiving + `backgroundScope` under `shared/src/commonTest`. Historical GREEN/REFACTOR evidence (30 + tests per target, 10/10 forced Native runs, CI run `33625103198` with 3/3 successful + macOS shared-test executions) predates the second review round and is superseded by the + final post-review verification above. +- Open decisions or blockers: no technical decision is open. Only the mandatory human review + remains. Production hardening of `AppGraph.close()` remains explicitly deferred outside + E1-12 because it would change D-89 and touch gated `core/database/**`. +- Exact next step: owner review and merge of PR #49. +- Final CI evidence: GitHub Actions run + `https://github.com/davidru85/carApp/actions/runs/33676703761` on the pushed HEAD `6776810` + passed all ten required jobs with no reruns: `ktlint`, `detekt`, `architecture-check`, + `contract-check`, `android-assemble`, `android-instrumented-tests`, `shared-tests`, + `ios-simulator-build` (9m41s), `objc-header-golden-check` and `provider-decoupling`. The + code-identical documentation-only HEADs `9258821` and `f62516d` had already passed all ten + jobs in runs `https://github.com/davidru85/carApp/actions/runs/33667021311` and + `https://github.com/davidru85/carApp/actions/runs/33669085132`. The push of this final + documentation-only checkpoint triggers the same ten checks on the identical code. +- Flake record for run `33669085132` (all on documentation-only commits; none reachable from + the E1-12 `:shared` commonTest change, which the iOS host tests do not link): + - Attempt 1: `ViewModelLifecycleTests.testVehicleListViewModelInitialState` crashed the XCTest + runner with the E1-11-documented signature `kotlin.IllegalStateException: + AndroidxDriverConnectionPool.close() called while 1 reader connection(s) still checked out`. + - Attempt 2: `ViewModelLifecycleTests.testFuelEntryFormViewModelModeDerivations` crashed with + the same connection-pool signature (the exact test named in the E1-11 `d1cf977` instance). + - Attempt 3: `carAppUITests.VehicleAndFuelFlowUITests.testVehicleSwipeDeleteShowsConfirmationDialog` + failed on `XCTAssertTrue failed - Swipe should reveal a delete action` (UI-gesture flake). + - Attempt 4: all ten jobs passed; no code change between attempts. + This matches the E1-11 handoff expectation to re-run failed macOS jobs before merging. The + intermittent `AppGraph.close()` production defect behind the connection-pool signature remains + the explicitly deferred production-hardening follow-up recorded under Risks or Follow-ups. + +## Scope Completed + +- Added one reusable test harness that owns a child scope, launches state-holder collectors and + cancels and joins that scope before closing its graph. +- Migrated every Kotlin caller-owned `AppGraph` state-holder scope in `:shared` tests to the + harness. +- Removed all direct state-holder collector launches on `runTest.backgroundScope`. +- Audited every `:shared` test class that mounts an `AppGraph`. + +## Acceptance Evidence + +- `AppGraphTestHarnessTest.closeCancelsCollectorsBeforeClosingTheGraph` deterministically observes + `collectors-cancelled` before `graph-closed` on Android host and Kotlin/Native. +- `FuelEntryStateHolderTest` uses `AppGraphTestHarness.scope` for every state holder and + `AppGraphTestHarness.collect(...)` for every long-lived state or save-completion collector; its + `finally` blocks call only `harness.close()`. +- `AppGraphContractTest`, `VehicleFormStateHolderTest` and `VehicleListStateHolderTest` use the + harness for every Kotlin caller-owned state-holder scope and graph teardown. +- Source audit finds no direct `backgroundScope.launch` and no state-holder factory receiving + `backgroundScope` anywhere under `shared/src/commonTest`. +- `constructorThrowsWhenParentScopeHasNoTestCoroutineScheduler` proves a failed harness + construction leaves no orphaned child `Job` on the parent, and its teardown cancels the parent + `Job` even if an assertion fails. +- Final post-review counts (generated reports): 32 tests, 0 failures, 0 skipped on + `:shared:testAndroidHostTest`; 32 tests, 0 failures, 0 skipped on + `:shared:iosSimulatorArm64Test`. Historical evidence below records the pre-review 30-test + baseline and the pre-review CI executions. +- Historical (pre-review): `:shared:iosSimulatorArm64Test` passed 10 consecutive forced runs on + the local Apple-silicon host, and GitHub Actions run `33625103198` passed all 10 required jobs + with 3/3 successful macOS `shared-tests` executions and no SIGSEGV. +- Graph-mounting audit: + - `AppGraphCloseTest.kt`: no caller-owned state holder or external collector; directly verifies + idempotent direct and Swift-transitive graph close plus bootstrap cancellation. + - `AppGraphContractTest.kt`: Kotlin caller-owned holder scopes migrated to the harness. + - `AppGraphTestHarnessTest.kt`: uses the harness and observes its cancellation-before-close + contract. + - `BuildAppGraphTest.kt`: no state holder or external collector; only inspects dependency mapping + before direct graph close. + - `FuelEntryStateHolderTest.kt`: every holder scope and collector migrated to the harness. + - `SessionStateHolderTest.kt`: uses `SwiftAppGraph`, which owns and closes its holder child scope + before closing the wrapped graph under the existing D-89 contract. + - `SwiftAppGraphLifecycleTest.kt`: uses the same Swift-owned lifecycle and explicitly verifies a + released holder scope is inactive before final graph close. + - `VehicleFormStateHolderTest.kt`: every Kotlin caller-owned holder scope migrated to the harness. + - `VehicleListStateHolderTest.kt`: every Kotlin caller-owned holder scope migrated to the + harness. + +## Out of Scope / Not Done + +- Production hardening of `AppGraph.close()`, state-holder production lifecycle changes and any + `DatabaseFactory` change are explicitly deferred outside E1-12. + +## Files Changed + +- `AGENTS.md`. +- `README.md`. +- `docs/BACKLOG.md`. +- `docs/PROJECT_LOG.md`. +- `docs/handoff-E1-12.md` (new; live continuity record). +- `shared/src/commonTest/kotlin/com/ruizurraca/carapp/AppGraphCloseTest.kt`. +- `shared/src/commonTest/kotlin/com/ruizurraca/carapp/AppGraphContractTest.kt`. +- `shared/src/commonTest/kotlin/com/ruizurraca/carapp/AppGraphTestHarness.kt` (new; reusable child- + scope and ordered-teardown helper). +- `shared/src/commonTest/kotlin/com/ruizurraca/carapp/AppGraphTestHarnessTest.kt` (new; deterministic + teardown-order regression test). +- `shared/src/commonTest/kotlin/com/ruizurraca/carapp/FuelEntryStateHolderTest.kt`. +- `shared/src/commonTest/kotlin/com/ruizurraca/carapp/RecordingDatabaseFactory.kt` (new; shared + recording test double for graph-close assertions). +- `shared/src/commonTest/kotlin/com/ruizurraca/carapp/VehicleFormStateHolderTest.kt`. +- `shared/src/commonTest/kotlin/com/ruizurraca/carapp/VehicleListStateHolderTest.kt`. + +## Decisions Made + +- The owner's `NETWORK` phase label is treated as `RED`; no network phase exists for this test-only + story. This interpretation changes no technical contract or decision. +- The owner explicitly requested one push after the RED, GREEN and REFACTOR commits. This + story-specific instruction replaces the default push-after-each-phase cadence while preserving + the required commit order. +- The final continuity-record commit was amended twice and re-pushed with + `git push --force-with-lease` (each lease pinned to the exact pushed commit). Reason: the + handoff checkpoint describes the final HEAD, so every documentation push would otherwise + create a newer HEAD and self-invalidate the record in an endless loop. The amended commits + are documentation-only; the lease prevented any possibility of overwriting a foreign update, + and no other branch or commit was affected. This deviation from the default no-force-push + convention is recorded here per the SHOULD rule. +- No technical decision was introduced. The helper and test migrations implement the solution + already fixed by the E1-12 acceptance criteria while preserving D-89 unchanged. + +## Verification Run + +Historical RED/GREEN/REFACTOR evidence (first implementation round, pre-review; retained for the +TDD record and superseded by the final post-review verification below): + +- RED: `./gradlew :shared:testAndroidHostTest --tests + "com.ruizurraca.carapp.AppGraphTestHarnessTest.closeCancelsCollectorsBeforeClosingTheGraph" + --rerun-tasks` — expected BUILD FAILED; the test compiled and executed, expecting + `[collectors-cancelled, graph-closed]` but observing `[graph-closed]`. +- GREEN: `./gradlew :shared:testAndroidHostTest :shared:iosSimulatorArm64Test --rerun-tasks` — BUILD + SUCCESSFUL; 30 tests passed on each target, including 10 `FuelEntryStateHolderTest` cases and the + deterministic harness regression test. +- GREEN: `./gradlew :shared:ktlintCheck :shared:detekt` — BUILD SUCCESSFUL. +- REFACTOR stability: `for run_index in {1..10}; do ./gradlew + :shared:iosSimulatorArm64Test --rerun-tasks --quiet || exit 1; done` — 10/10 consecutive BUILD + SUCCESSFUL runs on the local Apple-silicon host; no signal or assertion failure. +- REFACTOR full repository verification: `./gradlew ktlintCheck detekt architectureCheck + contractCheck :build-logic:convention:test koverVerify :androidApp:assembleDebug + :androidApp:testDebugUnitTest testAndroidHostTest iosSimulatorArm64Test -x + :integration:firebase-auth:iosSimulatorArm64Test -x + :integration:firebase-firestore:iosSimulatorArm64Test -x + :wiring:firebase:iosSimulatorArm64Test -x :composition:ios:iosSimulatorArm64Test` — BUILD + SUCCESSFUL in 6s with 627 actionable tasks (41 executed, 586 up-to-date); `contractCheck` + output inspected: every assertion PASS, 111 decisions, 111 ADRs, no unresolved decisions and no + `PENDING` assertions. +- Historical CI: `https://github.com/davidru85/carApp/actions/runs/33625103198` — attempt 1 passed + all 10 required jobs; the macOS `shared-tests` job passed in the original run and two isolated + reruns (3/3 successful executions, no SIGSEGV). + +Final post-review verification (second review round, current HEAD): + +- RED (orphaned child job): `./gradlew :shared:testAndroidHostTest --tests + "com.ruizurraca.carapp.AppGraphTestHarnessTest.constructorThrowsWhenParentScopeHasNoTestCoroutineScheduler" + --rerun-tasks` — expected BUILD FAILED before the harness fix: the test observed + `[SupervisorJobImpl{Active}]` attached to the retained parent `Job` instead of `[]`, proving the + constructor orphaned `scopeJob` when scheduler validation failed. +- GREEN: `./gradlew :shared:testAndroidHostTest --tests + "com.ruizurraca.carapp.AppGraphTestHarnessTest" --rerun-tasks` — BUILD SUCCESSFUL; 3/3 harness + tests passed after moving scheduler validation before `scopeJob` creation. +- REFACTOR (exception-safe teardown): `./gradlew :shared:testAndroidHostTest + :shared:iosSimulatorArm64Test :shared:ktlintCheck :shared:detekt --rerun-tasks` — BUILD + SUCCESSFUL in 16s with all 143 actionable tasks executed; 32 tests, 0 failures, 0 skipped on + each target (generated-report counts). +- Full repository verification: the complete non-instrumented command from `AGENTS.md` — BUILD + SUCCESSFUL in 1s with 627 actionable tasks (38 executed, 589 up-to-date). +- `./gradlew contractCheck` output inspected: no unresolved decisions, no `PENDING` assertions. +- `git diff --check` — clean. +- Source audit repeated: no `backgroundScope.launch` state-holder collector and no state-holder + factory call receiving `backgroundScope` under `shared/src/commonTest`; `backgroundScope` appears + only as the harness `parentScope` argument. + +## Review Findings Resolution (PR #49) + +### Blocking Finding: Collector Dispatcher Regression +- Root cause: `AppGraphTestHarness.collect()` launched directly into `scope`, which inherited + `StandardTestDispatcher` from the enclosing `runTest` scope. Tests migrated to the harness + previously collected on `UnconfinedTestDispatcher(testScheduler)`. `CoroutineStart.UNDISPATCHED` + made only the initial flow collection synchronous; subsequent emissions queued on the scheduler. +- Fix: `AppGraphTestHarness` now resolves `TestCoroutineScheduler` from + `parentScope.coroutineContext` via `requireNotNull(...)` (failing loudly with + `IllegalArgumentException` if missing), instantiates `UnconfinedTestDispatcher(scheduler)` and + passes `context = collectorDispatcher` to `scope.launch(..., start = CoroutineStart.UNDISPATCHED)`. + The collector jobs remain children of `scopeJob`, preserving `cancelAndJoin()` guarantees. +- Verification: + - Step 1 (before fix): temporary assertion placed immediately after `form.confirmSave(...)` + read `saveCompletionCount` as `0`. + - Step 2 (after fix): eager collector execution verified; emission observed eagerly as `1` + (and reinforced by unit test `collectorsRunEagerlyOnUnconfinedTestDispatcher`). + - Step 3 (cleanup): temporary instrumentation removed; working tree clean. + +### Non-blocking Cleanups +- Deduplicated test doubles: collapsed `CloseRecordingDatabaseFactory` in + `AppGraphTestHarnessTest.kt` and `RecordingDatabaseFactory` in `AppGraphCloseTest.kt` into a single + shared `RecordingDatabaseFactory.kt` with an optional `onClose: () -> Unit = {}` callback. +- Hardened teardown: wrapped `harness.close()` in a nested `finally` block in + `AppGraphTestHarnessTest.kt` so the graph is closed even if assertions fail. +- Added explicit unit tests in `AppGraphTestHarnessTest.kt`: + - `constructorThrowsWhenParentScopeHasNoTestCoroutineScheduler`: proves missing scheduler throws. + - `collectorsRunEagerlyOnUnconfinedTestDispatcher`: proves emissions are observed eagerly. + +### Second Review Round (PR #49, current) + +- Finding 1 (orphaned child `Job` on failed construction): confirmed RED by extending + `constructorThrowsWhenParentScopeHasNoTestCoroutineScheduler` to retain the parent `Job` + explicitly and assert no children remain attached after the failed constructor; the assertion + observed `[SupervisorJobImpl{Active}]` against the then-current implementation. Fixed by moving + the `TestCoroutineScheduler` resolution and validation in `AppGraphTestHarness` before the + `SupervisorJob` creation, so a missing scheduler fails before any `Job` is constructed. The + test teardown now cancels the parent `Job` in its `finally` even if an assertion fails. Valid + harness behavior is unchanged: collectors remain children of `scopeJob`, run eagerly through + `UnconfinedTestDispatcher` and are cancelled and joined before `graph.close()`. +- Finding 2 (exception-safe teardown): every teardown in `AppGraphTestHarnessTest.kt` now uses + nested `try/finally` blocks so `owningFactory.close()` still runs when `harness.close()` or + `graph.close()` throws, and the obsolete `DatabaseFactory` and `DatabaseHandle` imports were + removed from `AppGraphCloseTest.kt`. +- Finding 3 (continuity record): this handoff's `In-Progress Checkpoint`, `Files Changed`, + acceptance evidence and verification sections were refreshed to the final HEAD, with final + report-derived test counts and historical evidence clearly separated from final post-review + verification. A correction entry was appended to `docs/PROJECT_LOG.md` rather than rewriting + the original story entry. +- Finding 4 (PR description): the PR #49 description was refreshed after the final push to + agree with this handoff, including the checkpoint, changed-file list, review-findings + resolution, verification results, current HEAD and latest CI evidence; the human-review + gate remains checked and the PR is not merged. + +## Contract Impact + +- No contract changes planned. E1-12 preserves D-89 and makes test teardown respect its existing + ownership contract. + +## Decision Board Impact + +- No decision changes planned. + +## Shared-Write Modules Touched + +- None. `:core:database` is explicitly out of scope. + +## Project Log Entry + +- [x] Entry appended + +## Risks or Follow-ups + +- The production question of making `AppGraph.close()` safe against live external subscribers is + deferred to a separate explicitly scoped and human-gated story if pursued. + +## Human Review Gate + +- Applies: E1-12 is explicitly human-review-required in `docs/BACKLOG.md`. The agent MUST NOT merge + its pull request. diff --git a/shared/src/commonTest/kotlin/com/ruizurraca/carapp/AppGraphCloseTest.kt b/shared/src/commonTest/kotlin/com/ruizurraca/carapp/AppGraphCloseTest.kt index 074fdbf..3527bce 100644 --- a/shared/src/commonTest/kotlin/com/ruizurraca/carapp/AppGraphCloseTest.kt +++ b/shared/src/commonTest/kotlin/com/ruizurraca/carapp/AppGraphCloseTest.kt @@ -1,8 +1,6 @@ package com.ruizurraca.carapp import com.ruizurraca.carapp.core.common.DispatcherProvider -import com.ruizurraca.carapp.core.database.DatabaseFactory -import com.ruizurraca.carapp.core.database.DatabaseHandle import com.ruizurraca.carapp.core.testing.InMemoryDatabaseFactory import com.ruizurraca.carapp.shared.testing.testAppGraphDependencies import com.ruizurraca.carapp.shared.testing.testAppProviders @@ -78,22 +76,3 @@ class AppGraphCloseTest { } } } - -private class RecordingDatabaseFactory( - private val delegate: DatabaseFactory, -) : DatabaseFactory { - var closeCalls: Int = 0 - private set - - override fun create(): DatabaseHandle { - val handle = delegate.create() - return object : DatabaseHandle { - override val database = handle.database - - override fun close() { - closeCalls += 1 - handle.close() - } - } - } -} diff --git a/shared/src/commonTest/kotlin/com/ruizurraca/carapp/AppGraphContractTest.kt b/shared/src/commonTest/kotlin/com/ruizurraca/carapp/AppGraphContractTest.kt index 6c4fff9..05b62b0 100644 --- a/shared/src/commonTest/kotlin/com/ruizurraca/carapp/AppGraphContractTest.kt +++ b/shared/src/commonTest/kotlin/com/ruizurraca/carapp/AppGraphContractTest.kt @@ -18,9 +18,13 @@ class AppGraphContractTest { isDebugBuild = true, providers = testAppProviders(dependencies), ) + val harness = AppGraphTestHarness(graph, backgroundScope) - assertIs(graph.vehicleListStateHolder(backgroundScope)) - assertIs(graph.vehicleFormStateHolder(backgroundScope, null)) - graph.close() + try { + assertIs(graph.vehicleListStateHolder(harness.scope)) + assertIs(graph.vehicleFormStateHolder(harness.scope, null)) + } finally { + harness.close() + } } } diff --git a/shared/src/commonTest/kotlin/com/ruizurraca/carapp/AppGraphTestHarness.kt b/shared/src/commonTest/kotlin/com/ruizurraca/carapp/AppGraphTestHarness.kt new file mode 100644 index 0000000..e82af53 --- /dev/null +++ b/shared/src/commonTest/kotlin/com/ruizurraca/carapp/AppGraphTestHarness.kt @@ -0,0 +1,49 @@ +package com.ruizurraca.carapp + +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.CoroutineStart +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.Job +import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.cancelAndJoin +import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.collect +import kotlinx.coroutines.launch +import kotlinx.coroutines.test.TestCoroutineScheduler +import kotlinx.coroutines.test.UnconfinedTestDispatcher + +@OptIn(ExperimentalCoroutinesApi::class) +internal class AppGraphTestHarness( + val graph: AppGraph, + parentScope: CoroutineScope, +) { + private val scheduler = + requireNotNull(parentScope.coroutineContext[TestCoroutineScheduler]) { + "AppGraphTestHarness requires parentScope to provide a TestCoroutineScheduler" + } + + private val scopeJob = SupervisorJob(parentScope.coroutineContext[Job]) + + private val collectorDispatcher = UnconfinedTestDispatcher(scheduler) + + val scope = CoroutineScope(parentScope.coroutineContext + scopeJob) + + fun collect( + flow: Flow, + collector: suspend (T) -> Unit = {}, + ): Job = + scope.launch( + context = collectorDispatcher, + start = CoroutineStart.UNDISPATCHED, + ) { + flow.collect(collector) + } + + suspend fun close() { + try { + scopeJob.cancelAndJoin() + } finally { + graph.close() + } + } +} diff --git a/shared/src/commonTest/kotlin/com/ruizurraca/carapp/AppGraphTestHarnessTest.kt b/shared/src/commonTest/kotlin/com/ruizurraca/carapp/AppGraphTestHarnessTest.kt new file mode 100644 index 0000000..e071ccd --- /dev/null +++ b/shared/src/commonTest/kotlin/com/ruizurraca/carapp/AppGraphTestHarnessTest.kt @@ -0,0 +1,126 @@ +package com.ruizurraca.carapp + +import com.ruizurraca.carapp.core.testing.InMemoryDatabaseFactory +import com.ruizurraca.carapp.shared.testing.testAppGraphDependencies +import com.ruizurraca.carapp.shared.testing.testAppProviders +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Job +import kotlinx.coroutines.awaitCancellation +import kotlinx.coroutines.flow.MutableSharedFlow +import kotlinx.coroutines.flow.flow +import kotlinx.coroutines.test.runTest +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFailsWith + +class AppGraphTestHarnessTest { + @Test + fun closeCancelsCollectorsBeforeClosingTheGraph() = + runTest { + val events = mutableListOf() + val owningFactory = InMemoryDatabaseFactory() + val dependencies = + testAppGraphDependencies( + databaseFactory = + RecordingDatabaseFactory(owningFactory) { + events += "graph-closed" + }, + ) + val harness = + AppGraphTestHarness( + graph = + buildAppGraph( + isDebugBuild = true, + providers = testAppProviders(dependencies), + ), + parentScope = backgroundScope, + ) + + try { + try { + harness.collect( + flow { + try { + awaitCancellation() + } finally { + events += "collectors-cancelled" + } + }, + ) + } finally { + try { + harness.close() + } finally { + assertEquals( + listOf("collectors-cancelled", "graph-closed"), + events, + ) + } + } + } finally { + owningFactory.close() + } + } + + @Test + fun constructorThrowsWhenParentScopeHasNoTestCoroutineScheduler() { + val parentJob = Job() + val nonTestScope = CoroutineScope(parentJob) + val owningFactory = InMemoryDatabaseFactory() + val dependencies = testAppGraphDependencies(databaseFactory = owningFactory) + val graph = + buildAppGraph( + isDebugBuild = true, + providers = testAppProviders(dependencies), + ) + + try { + assertFailsWith { + AppGraphTestHarness(graph, nonTestScope) + } + assertEquals( + emptyList(), + parentJob.children.toList(), + "a failed harness construction must not leave an orphaned child job on the parent", + ) + } finally { + try { + parentJob.cancel() + graph.close() + } finally { + owningFactory.close() + } + } + } + + @Test + fun collectorsRunEagerlyOnUnconfinedTestDispatcher() = + runTest { + val owningFactory = InMemoryDatabaseFactory() + val dependencies = testAppGraphDependencies(databaseFactory = owningFactory) + val harness = + AppGraphTestHarness( + graph = + buildAppGraph( + isDebugBuild = true, + providers = testAppProviders(dependencies), + ), + parentScope = backgroundScope, + ) + + try { + val flow = MutableSharedFlow(extraBufferCapacity = 1) + var received = 0 + harness.collect(flow) { received = it } + + flow.tryEmit(42) + assertEquals(42, received) + } finally { + try { + harness.close() + } finally { + owningFactory.close() + } + } + } +} diff --git a/shared/src/commonTest/kotlin/com/ruizurraca/carapp/FuelEntryStateHolderTest.kt b/shared/src/commonTest/kotlin/com/ruizurraca/carapp/FuelEntryStateHolderTest.kt index 8d68341..37cb288 100644 --- a/shared/src/commonTest/kotlin/com/ruizurraca/carapp/FuelEntryStateHolderTest.kt +++ b/shared/src/commonTest/kotlin/com/ruizurraca/carapp/FuelEntryStateHolderTest.kt @@ -21,17 +21,11 @@ import com.ruizurraca.carapp.core.testing.TestDispatcherProvider import com.ruizurraca.carapp.feature.fuel.presentation.FuelEntryListStateHolder import com.ruizurraca.carapp.shared.testing.testAppGraphDependencies import com.ruizurraca.carapp.shared.testing.testAppProviders -import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.ExperimentalCoroutinesApi -import kotlinx.coroutines.cancelAndJoin -import kotlinx.coroutines.flow.collect import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.flowOf import kotlinx.coroutines.flow.toList -import kotlinx.coroutines.launch import kotlinx.coroutines.test.StandardTestDispatcher -import kotlinx.coroutines.test.TestScope -import kotlinx.coroutines.test.UnconfinedTestDispatcher import kotlinx.coroutines.test.advanceUntilIdle import kotlinx.coroutines.test.runTest import kotlin.test.Test @@ -53,13 +47,17 @@ class FuelEntryStateHolderTest { FakeLocaleProvider(LocaleInfo("en-US", "US", CurrencyCode("USD"))), ) val databaseHandle = dependencies.databaseFactory.create() - val graph = - buildAppGraph( - isDebugBuild = true, - providers = - testAppProviders( - dependencies.copy(databaseFactory = fixedDatabaseFactory(databaseHandle)), + val harness = + AppGraphTestHarness( + graph = + buildAppGraph( + isDebugBuild = true, + providers = + testAppProviders( + dependencies.copy(databaseFactory = fixedDatabaseFactory(databaseHandle)), + ), ), + parentScope = backgroundScope, ) try { @@ -73,7 +71,7 @@ class FuelEntryStateHolderTest { persisted, ) } finally { - graph.close() + harness.close() } } @@ -109,16 +107,21 @@ class FuelEntryStateHolderTest { fun newFormUsesExactClockVehicleOdometerAndSupportedLocaleCurrency() = runTest { val now = Instant.parse("2026-08-31T06:42:19.123Z") - val graph = - buildFuelGraph( - clock = FakeAppClock(now), - localeProvider = FakeLocaleProvider(LocaleInfo("en-US", "US", CurrencyCode("USD"))), + val harness = + AppGraphTestHarness( + graph = + buildFuelGraph( + clock = FakeAppClock(now), + localeProvider = FakeLocaleProvider(LocaleInfo("en-US", "US", CurrencyCode("USD"))), + ), + parentScope = backgroundScope, ) + val graph = harness.graph try { - val vehicleId = createVehicle(graph, backgroundScope, initialOdometerKm = 12_345L) - val holder = graph.fuelEntryFormStateHolder(backgroundScope, vehicleId, entryId = null) - backgroundScope.launch(UnconfinedTestDispatcher(testScheduler)) { holder.state.collect() } + val vehicleId = createVehicle(harness, initialOdometerKm = 12_345L) + val holder = graph.fuelEntryFormStateHolder(harness.scope, vehicleId, entryId = null) + harness.collect(holder.state) val state = holder.state.first { value -> value.odometerKm == 12_345L } assertEquals(now.toEpochMilliseconds(), state.dateEpochMillis) @@ -128,27 +131,32 @@ class FuelEntryStateHolderTest { assertFalse(state.hasMissedEntries) assertNull(state.message) } finally { - graph.close() + harness.close() } } @Test fun unsupportedLocaleCurrencyFallsBackToEur() = runTest { - val graph = - buildFuelGraph( - localeProvider = FakeLocaleProvider(LocaleInfo("ja-JP", "JP", CurrencyCode("JPY"))), + val harness = + AppGraphTestHarness( + graph = + buildFuelGraph( + localeProvider = FakeLocaleProvider(LocaleInfo("ja-JP", "JP", CurrencyCode("JPY"))), + ), + parentScope = backgroundScope, ) + val graph = harness.graph try { - val vehicleId = createVehicle(graph, backgroundScope, initialOdometerKm = 1L) - val holder = graph.fuelEntryFormStateHolder(backgroundScope, vehicleId, entryId = null) - backgroundScope.launch(UnconfinedTestDispatcher(testScheduler)) { holder.state.collect() } + val vehicleId = createVehicle(harness, initialOdometerKm = 1L) + val holder = graph.fuelEntryFormStateHolder(harness.scope, vehicleId, entryId = null) + harness.collect(holder.state) advanceUntilIdle() assertEquals("EUR", holder.state.value.currencyCode) } finally { - graph.close() + harness.close() } } @@ -164,41 +172,44 @@ class FuelEntryStateHolderTest { SettingsDatabaseAccess(databaseHandle.database).upsertSettings( SettingsDatabaseRow("GBP", "KM", "LITER", analyticsEnabled = false), ) - val graph = - buildAppGraph( - isDebugBuild = true, - providers = - testAppProviders( - dependencies.copy(databaseFactory = fixedDatabaseFactory(databaseHandle)), + val harness = + AppGraphTestHarness( + graph = + buildAppGraph( + isDebugBuild = true, + providers = + testAppProviders( + dependencies.copy(databaseFactory = fixedDatabaseFactory(databaseHandle)), + ), ), + parentScope = backgroundScope, ) + val graph = harness.graph val holder = graph.fuelEntryFormStateHolder( - backgroundScope, + harness.scope, vehicleId = "00000000-0000-4000-8000-000000000099", entryId = null, ) - val collector = - backgroundScope.launch(UnconfinedTestDispatcher(testScheduler)) { holder.state.collect() } + harness.collect(holder.state) try { assertEquals("GBP", holder.state.first { it.currencyCode == "GBP" }.currencyCode) } finally { - holder.close() - collector.cancelAndJoin() - graph.close() + harness.close() } } @Test fun litersAndPriceDeriveTotalCostWhileTyping() = runTest { - val graph = buildFuelGraph() + val harness = AppGraphTestHarness(buildFuelGraph(), backgroundScope) + val graph = harness.graph try { - val vehicleId = createVehicle(graph, backgroundScope, initialOdometerKm = 100L) - val holder = graph.fuelEntryFormStateHolder(backgroundScope, vehicleId, entryId = null) - backgroundScope.launch(UnconfinedTestDispatcher(testScheduler)) { holder.state.collect() } + val vehicleId = createVehicle(harness, initialOdometerKm = 100L) + val holder = graph.fuelEntryFormStateHolder(harness.scope, vehicleId, entryId = null) + harness.collect(holder.state) holder.setLitersScaled(45_123L) holder.setPricePerLiterScaled(1_789L) @@ -207,19 +218,20 @@ class FuelEntryStateHolderTest { assertEquals(8_073L, derivedState.totalCostMinor) assertNull(derivedState.message) } finally { - graph.close() + harness.close() } } @Test fun invalidLiveMoneyClearsDerivedValueAndWaitsUntilSaveToPublishError() = runTest { - val graph = buildFuelGraph() + val harness = AppGraphTestHarness(buildFuelGraph(), backgroundScope) + val graph = harness.graph try { - val vehicleId = createVehicle(graph, backgroundScope, initialOdometerKm = 100L) - val holder = graph.fuelEntryFormStateHolder(backgroundScope, vehicleId, entryId = null) - backgroundScope.launch(UnconfinedTestDispatcher(testScheduler)) { holder.state.collect() } + val vehicleId = createVehicle(harness, initialOdometerKm = 100L) + val holder = graph.fuelEntryFormStateHolder(harness.scope, vehicleId, entryId = null) + harness.collect(holder.state) holder.setLitersScaled(40_000L) holder.setPricePerLiterScaled(1_000_000L) @@ -242,24 +254,25 @@ class FuelEntryStateHolderTest { ?.code, ) } finally { - graph.close() + harness.close() } } @Test fun inconsistentPartialEntryRequiresConfirmationThenPublishesBothIndicators() = runTest { - val graph = buildFuelGraph() + val harness = AppGraphTestHarness(buildFuelGraph(), backgroundScope) + val graph = harness.graph try { - val vehicleId = createVehicle(graph, backgroundScope, initialOdometerKm = 100L) - val list = graph.fuelEntryListStateHolder(backgroundScope, vehicleId) - val form = graph.fuelEntryFormStateHolder(backgroundScope, vehicleId, entryId = null) + val vehicleId = createVehicle(harness, initialOdometerKm = 100L) + val list = graph.fuelEntryListStateHolder(harness.scope, vehicleId) + val form = graph.fuelEntryFormStateHolder(harness.scope, vehicleId, entryId = null) var saveCompletionCount = 0 - backgroundScope.launch(UnconfinedTestDispatcher(testScheduler)) { list.state.collect() } - backgroundScope.launch(UnconfinedTestDispatcher(testScheduler)) { form.state.collect() } - backgroundScope.launch(UnconfinedTestDispatcher(testScheduler)) { - form.observeSaveCompletions().collect { saveCompletionCount += 1 } + harness.collect(list.state) + harness.collect(form.state) + harness.collect(form.observeSaveCompletions()) { + saveCompletionCount += 1 } form.setOdometerKm(50L) @@ -296,23 +309,24 @@ class FuelEntryStateHolderTest { assertEquals(SyncStatus.Idle, publishedState.syncStatus) assertEquals(1, saveCompletionCount) } finally { - graph.close() + harness.close() } } @Test fun listPublishesWeightedSummaryAndReliabilityAfterThreeFullTanks() = runTest { - val graph = buildFuelGraph() + val harness = AppGraphTestHarness(buildFuelGraph(), backgroundScope) + val graph = harness.graph try { - val vehicleId = createVehicle(graph, backgroundScope, initialOdometerKm = 100L) - val list = graph.fuelEntryListStateHolder(backgroundScope, vehicleId) - backgroundScope.launch(UnconfinedTestDispatcher(testScheduler)) { list.state.collect() } + val vehicleId = createVehicle(harness, initialOdometerKm = 100L) + val list = graph.fuelEntryListStateHolder(harness.scope, vehicleId) + harness.collect(list.state) - saveFullEntry(graph, backgroundScope, list, vehicleId, odometerKm = 100L, expectedCount = 1) - saveFullEntry(graph, backgroundScope, list, vehicleId, odometerKm = 500L, expectedCount = 2) - saveFullEntry(graph, backgroundScope, list, vehicleId, odometerKm = 900L, expectedCount = 3) + saveFullEntry(harness, list, vehicleId, odometerKm = 100L, expectedCount = 1) + saveFullEntry(harness, list, vehicleId, odometerKm = 500L, expectedCount = 2) + saveFullEntry(harness, list, vehicleId, odometerKm = 900L, expectedCount = 3) val state = list.state.first { value -> @@ -327,20 +341,21 @@ class FuelEntryStateHolderTest { assertTrue(state.isConsumptionReliable) assertEquals(SyncStatus.Idle, state.syncStatus) } finally { - graph.close() + harness.close() } } @Test fun deleteRequiresConfirmationAndRemovesTheEntryFromTheReactiveList() = runTest { - val graph = buildFuelGraph() + val harness = AppGraphTestHarness(buildFuelGraph(), backgroundScope) + val graph = harness.graph try { - val vehicleId = createVehicle(graph, backgroundScope, initialOdometerKm = 100L) - val list = graph.fuelEntryListStateHolder(backgroundScope, vehicleId) - backgroundScope.launch(UnconfinedTestDispatcher(testScheduler)) { list.state.collect() } - saveFullEntry(graph, backgroundScope, list, vehicleId, odometerKm = 100L, expectedCount = 1) + val vehicleId = createVehicle(harness, initialOdometerKm = 100L) + val list = graph.fuelEntryListStateHolder(harness.scope, vehicleId) + harness.collect(list.state) + saveFullEntry(harness, list, vehicleId, odometerKm = 100L, expectedCount = 1) val populatedState = list.state.first { state -> state.entries.isNotEmpty() } val entryId = populatedState.entries.single().id @@ -359,17 +374,16 @@ class FuelEntryStateHolderTest { assertTrue(deletedState.entries.isEmpty()) } finally { - graph.close() + harness.close() } } - private suspend fun TestScope.createVehicle( - graph: AppGraph, - scope: CoroutineScope, + private suspend fun createVehicle( + harness: AppGraphTestHarness, initialOdometerKm: Long, ): String { - val holder = graph.vehicleFormStateHolder(scope, vehicleId = null) - scope.launch(UnconfinedTestDispatcher(testScheduler)) { holder.state.collect() } + val holder = harness.graph.vehicleFormStateHolder(harness.scope, vehicleId = null) + harness.collect(holder.state) holder.setName("Roadster") holder.setInitialOdometerKm(initialOdometerKm) holder.save() @@ -378,16 +392,15 @@ class FuelEntryStateHolderTest { return requireNotNull(state.savedVehicleId) } - private suspend fun TestScope.saveFullEntry( - graph: AppGraph, - scope: CoroutineScope, + private suspend fun saveFullEntry( + harness: AppGraphTestHarness, list: FuelEntryListStateHolder, vehicleId: String, odometerKm: Long, expectedCount: Int, ) { - val holder = graph.fuelEntryFormStateHolder(scope, vehicleId, entryId = null) - scope.launch(UnconfinedTestDispatcher(testScheduler)) { holder.state.collect() } + val holder = harness.graph.fuelEntryFormStateHolder(harness.scope, vehicleId, entryId = null) + harness.collect(holder.state) holder.setOdometerKm(odometerKm) holder.setLitersScaled(40_000L) holder.setPricePerLiterScaled(1_500L) diff --git a/shared/src/commonTest/kotlin/com/ruizurraca/carapp/RecordingDatabaseFactory.kt b/shared/src/commonTest/kotlin/com/ruizurraca/carapp/RecordingDatabaseFactory.kt new file mode 100644 index 0000000..3ee044c --- /dev/null +++ b/shared/src/commonTest/kotlin/com/ruizurraca/carapp/RecordingDatabaseFactory.kt @@ -0,0 +1,25 @@ +package com.ruizurraca.carapp + +import com.ruizurraca.carapp.core.database.DatabaseFactory +import com.ruizurraca.carapp.core.database.DatabaseHandle + +internal class RecordingDatabaseFactory( + private val delegate: DatabaseFactory, + private val onClose: () -> Unit = {}, +) : DatabaseFactory { + var closeCalls: Int = 0 + private set + + override fun create(): DatabaseHandle { + val handle = delegate.create() + return object : DatabaseHandle { + override val database = handle.database + + override fun close() { + closeCalls += 1 + onClose() + handle.close() + } + } + } +} diff --git a/shared/src/commonTest/kotlin/com/ruizurraca/carapp/VehicleFormStateHolderTest.kt b/shared/src/commonTest/kotlin/com/ruizurraca/carapp/VehicleFormStateHolderTest.kt index 68f602c..860c64f 100644 --- a/shared/src/commonTest/kotlin/com/ruizurraca/carapp/VehicleFormStateHolderTest.kt +++ b/shared/src/commonTest/kotlin/com/ruizurraca/carapp/VehicleFormStateHolderTest.kt @@ -46,9 +46,10 @@ class VehicleFormStateHolderTest { ), ), ) + val harness = AppGraphTestHarness(graph, backgroundScope) try { - val holder = graph.vehicleFormStateHolder(backgroundScope, vehicleId = null) + val holder = graph.vehicleFormStateHolder(harness.scope, vehicleId = null) holder.setName("Roadster") holder.save() @@ -72,7 +73,7 @@ class VehicleFormStateHolderTest { assertEquals(0L, vehicle.deleted) assertEquals(null, vehicle.deletedAt) } finally { - graph.close() + harness.close() } } @@ -93,9 +94,10 @@ class VehicleFormStateHolderTest { ), ), ) + val harness = AppGraphTestHarness(graph, backgroundScope) try { - val holder = graph.vehicleFormStateHolder(backgroundScope, vehicleId = null) + val holder = graph.vehicleFormStateHolder(harness.scope, vehicleId = null) holder.setName("Roadster") holder.save() @@ -130,7 +132,7 @@ class VehicleFormStateHolderTest { Json.parseToJsonElement(outbox.payload).jsonObject.keys, ) } finally { - graph.close() + harness.close() } } @@ -161,9 +163,10 @@ class VehicleFormStateHolderTest { ), ), ) + val harness = AppGraphTestHarness(graph, backgroundScope) try { - val holder = graph.vehicleFormStateHolder(backgroundScope, vehicleId = null) + val holder = graph.vehicleFormStateHolder(harness.scope, vehicleId = null) holder.setName("Roadster") holder.save() @@ -175,7 +178,7 @@ class VehicleFormStateHolderTest { assertEquals("00000000-0000-4000-8000-000000000001", call.second.entityId.value) assertEquals(1, call.second.schemaVersion) } finally { - graph.close() + harness.close() } } @@ -202,9 +205,10 @@ class VehicleFormStateHolderTest { ), ), ) + val harness = AppGraphTestHarness(graph, backgroundScope) try { - val holder = graph.vehicleFormStateHolder(backgroundScope, vehicleId = null) + val holder = graph.vehicleFormStateHolder(harness.scope, vehicleId = null) holder.setName("Roadster") holder.save() holder.state.first { state -> !state.isSaving } @@ -214,7 +218,7 @@ class VehicleFormStateHolderTest { assertEquals("VEHICLE", json.getValue("entityType").jsonPrimitive.content) assertEquals(EntityType.VEHICLE, snapshot.entityType) } finally { - graph.close() + harness.close() } } @@ -237,9 +241,10 @@ class VehicleFormStateHolderTest { ), ), ) + val harness = AppGraphTestHarness(graph, backgroundScope) try { - val holder = graph.vehicleFormStateHolder(backgroundScope, vehicleId = null) + val holder = graph.vehicleFormStateHolder(harness.scope, vehicleId = null) holder.setName("Roadster") holder.save() @@ -261,7 +266,7 @@ class VehicleFormStateHolderTest { ).awaitAsOneOrNull(), ) } finally { - graph.close() + harness.close() } } @@ -284,9 +289,10 @@ class VehicleFormStateHolderTest { ), ), ) + val harness = AppGraphTestHarness(graph, backgroundScope) try { - val holder = graph.vehicleFormStateHolder(backgroundScope, vehicleId = null) + val holder = graph.vehicleFormStateHolder(harness.scope, vehicleId = null) holder.setName("Offline Roadster") holder.save() @@ -309,7 +315,7 @@ class VehicleFormStateHolderTest { ) assertEquals(emptyList(), remote.pushCalls) } finally { - graph.close() + harness.close() } } diff --git a/shared/src/commonTest/kotlin/com/ruizurraca/carapp/VehicleListStateHolderTest.kt b/shared/src/commonTest/kotlin/com/ruizurraca/carapp/VehicleListStateHolderTest.kt index 64cd4e9..3bc441c 100644 --- a/shared/src/commonTest/kotlin/com/ruizurraca/carapp/VehicleListStateHolderTest.kt +++ b/shared/src/commonTest/kotlin/com/ruizurraca/carapp/VehicleListStateHolderTest.kt @@ -47,10 +47,11 @@ class VehicleListStateHolderTest { ), ), ) + val harness = AppGraphTestHarness(graph, backgroundScope) try { - val list = graph.vehicleListStateHolder(backgroundScope) - val form = graph.vehicleFormStateHolder(backgroundScope, vehicleId = null) + val list = graph.vehicleListStateHolder(harness.scope) + val form = graph.vehicleFormStateHolder(harness.scope, vehicleId = null) form.setName("Roadster") form.save() @@ -70,7 +71,7 @@ class VehicleListStateHolderTest { publishedState.vehicles, ) } finally { - graph.close() + harness.close() } } @@ -93,9 +94,10 @@ class VehicleListStateHolderTest { ), ), ) + val harness = AppGraphTestHarness(graph, backgroundScope) try { - val list = graph.vehicleListStateHolder(backgroundScope) + val list = graph.vehicleListStateHolder(harness.scope) list.refresh() list.state.first { state -> !state.isLoading } @@ -125,7 +127,7 @@ class VehicleListStateHolderTest { remote.pullCalls.single(), ) } finally { - graph.close() + harness.close() } }