From 2360829e83fd6bbc9f8a0a5b568d90edb77e93f1 Mon Sep 17 00:00:00 2001 From: davotoula Date: Thu, 30 Jul 2026 18:10:24 +0200 Subject: [PATCH] Code review: - refactor(location): dedupe test fixtures and simplify the gate internals - test(location): assert the release, not just its ledger side-effect - test(location): cover the LocationState -> LocationFlow -> RefCountedSession seam --- .../amethyst/service/location/LocationFlow.kt | 79 +++--- .../location/LocationProviderLadder.kt | 19 +- .../service/location/LocationState.kt | 106 ++++---- .../resourceusage/RefCountedSession.kt | 17 +- .../service/location/LocationFlowTest.kt | 36 +-- .../location/LocationLedgerCompositionTest.kt | 243 ++++++++++++++++++ .../location/LocationProviderLadderTest.kt | 21 +- .../service/location/MockLocationManager.kt | 50 ++++ 8 files changed, 429 insertions(+), 142 deletions(-) create mode 100644 amethyst/src/test/java/com/vitorpamplona/amethyst/service/location/LocationLedgerCompositionTest.kt create mode 100644 amethyst/src/test/java/com/vitorpamplona/amethyst/service/location/MockLocationManager.kt diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/location/LocationFlow.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/location/LocationFlow.kt index 3bc0f40ae2..0fd483cf2a 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/location/LocationFlow.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/location/LocationFlow.kt @@ -60,13 +60,12 @@ import kotlinx.coroutines.launch * the release on that path; see `releasesTheRegistrationWhenTheSeedThrows` in * `LocationFlowTest`. (In principle a collector cancelling mid-seed would also * unwind past `awaitClose` the same way, but that path could not be - * reproduced — `callbackFlow`'s `send` is buffered and returns without - * suspending, so it never observes the cancellation.) + * reproduced — `callbackFlow`'s buffer is empty at this point, so the single + * seed send returns without suspending and never observes the cancellation.) */ class LocationFlow( private val locationManager: LocationManager, private val sdkInt: Int = Build.VERSION.SDK_INT, - private val hasFine: Boolean = false, ) { @SuppressLint("MissingPermission") fun get( @@ -84,51 +83,24 @@ class LocationFlow( // One binder call, reused for both the ladder filter and the seed. val providers = locationManager.allProviders - val candidates = LocationProviderLadder.chooseProviders(sdkInt, hasFine) { it in providers } + val candidates = LocationProviderLadder.chooseProviders(sdkInt) { it in providers } - var registered: String? = null - for (provider in candidates) { - try { - locationManager.requestLocationUpdates( - provider, - minTimeMs, - minDistanceM, - locationCallback, - Looper.getMainLooper(), - ) - registered = provider - break - } catch (e: SecurityException) { - Log.w("LocationFlow", "Provider $provider refused the update request", e) - } - } - - if (registered == null) { - throw SecurityException("No usable location provider. Candidates: $candidates") - } + val registered = + candidates.firstOrNull { requestUpdates(it, minTimeMs, minDistanceM, locationCallback) } + ?: throw SecurityException("No usable location provider. Candidates: $candidates") Log.i("LocationFlow") { "Listening on $registered every ${minTimeMs}ms / ${minDistanceM}m" } onListening?.invoke(true) - // Everything after the acquire runs under try/finally, not under - // awaitClose. freshestLastKnownLocation() just below can throw a - // non-cancellation exception and unwind before awaitClose is ever - // reached — proven by releasesTheRegistrationWhenTheSeedThrows in - // LocationFlowTest, which fails if the release moves into - // awaitClose. Cleanup parked inside awaitClose would then never - // run — the registration would leak and the refcount would stick - // at >= 1 for the life of the process, so location.ms would accrue - // forever with nothing listening. (In principle a collector - // cancelling mid-seed would unwind the same way, but that path - // could not be reproduced: callbackFlow's send is buffered and - // returns without suspending, so it never observes the - // cancellation.) The finally covers both cases regardless. + // Cleanup lives in this finally, not in awaitClose — see the class + // KDoc, and `releasesTheRegistrationWhenTheSeedThrows` in + // LocationFlowTest, which fails if it moves. try { // Seeded after registration so the no-provider path throws // without having emitted anything; seeding first would show the // consumer Success -> LackPermission on a device with no // compatible provider. - freshestLastKnownLocation(providers)?.let { + freshestLastKnownLocation(candidates)?.let { Log.d("LocationFlow") { "Last known location is $it" } send(it) } @@ -141,10 +113,35 @@ class LocationFlow( } } + /** True when the registration was accepted; false when the provider refused it. */ + @SuppressLint("MissingPermission") + private fun requestUpdates( + provider: String, + minTimeMs: Long, + minDistanceM: Float, + locationCallback: LocationListener, + ): Boolean = + try { + locationManager.requestLocationUpdates( + provider, + minTimeMs, + minDistanceM, + locationCallback, + Looper.getMainLooper(), + ) + true + } catch (e: SecurityException) { + Log.w("LocationFlow", "Provider $provider refused the update request", e) + false + } + /** - * The freshest cached fix across every provider. Permission-checked per - * provider like the update request is, so each lookup is guarded — on a - * device where a provider refuses us, the others should still seed. + * The freshest cached fix across the providers the ladder deemed usable. + * Sweeping every provider the device reports instead would, on the + * coarse-only legacy path, pay a guaranteed-to-throw binder call per + * fine-only provider on every flow start. Still guarded per provider, like + * the update request is — on a device where one refuses us, the others + * should still seed. */ @SuppressLint("MissingPermission") private fun freshestLastKnownLocation(providers: List): Location? = diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/location/LocationProviderLadder.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/location/LocationProviderLadder.kt index 278d57f866..fb9ae6d8a4 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/location/LocationProviderLadder.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/location/LocationProviderLadder.kt @@ -36,10 +36,13 @@ import android.os.Build * Below API 31, `gps`, `passive` and `fused` required `ACCESS_FINE_LOCATION`; * only `network` accepted `ACCESS_COARSE_LOCATION`. Approximate location, which * lets a coarse-only app request any provider and receive a fuzzed result, is an - * Android 12 change. Amethyst declares coarse only, so [hasFine] is always false - * in production — it is a parameter so the function is total over the permission - * axis and both sides of the API branch are testable, not because fine access is - * anticipated. + * Android 12 change. Amethyst declares coarse only, so the legacy branch is + * unconditional below API 31. + * + * Adding `ACCESS_FINE_LOCATION` later would **not** widen this on its own — the + * branch below has no permission input, so pre-31 devices would keep getting + * `network` alone and silently lose the precision the new permission was granted + * for. Whoever adds it must widen the condition here too. * * Returns the ordered candidate list rather than a single choice so the caller * can fall through to the next rung if a registration is refused. An empty list @@ -60,15 +63,9 @@ object LocationProviderLadder { fun chooseProviders( sdkInt: Int, - hasFine: Boolean, exists: (String) -> Boolean, ): List { - val ladder = - if (sdkInt >= Build.VERSION_CODES.S || hasFine) { - FULL_LADDER - } else { - COARSE_ONLY_LEGACY_LADDER - } + val ladder = if (sdkInt >= Build.VERSION_CODES.S) FULL_LADDER else COARSE_ONLY_LEGACY_LADDER return ladder.filter(exists) } diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/location/LocationState.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/location/LocationState.kt index 12dcfa7e60..82ad1bed00 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/location/LocationState.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/location/LocationState.kt @@ -62,7 +62,7 @@ import kotlinx.coroutines.flow.transformLatest */ class LocationState( context: Context, - scope: CoroutineScope, + private val scope: CoroutineScope, private val isForeground: StateFlow, /** * Resource-ledger hook: true while location updates are actively @@ -88,9 +88,16 @@ class LocationState( /** * How long to keep listening after the last activity stops, so a * one-second app switch doesn't destroy and rebuild the registration. - * Matches the `WhileSubscribed` window below, and is the same intent. + * Same intent as [SUBSCRIPTION_STOP_TIMEOUT_MS], on the other axis. */ const val BACKGROUND_GRACE_MS: Long = 5_000L + + /** + * How long `stateIn` keeps the upstream alive after the last collector + * leaves, so a screen rotation or a tab switch doesn't rebuild the + * registration either. + */ + const val SUBSCRIPTION_STOP_TIMEOUT_MS: Long = 5_000L } sealed class LocationResult { @@ -107,11 +114,12 @@ class LocationState( private var hasLocationPermission = MutableStateFlow(false) - // Volatile: R1 below reads these to decide whether to emit Loading, from a - // different coroutine than the onEach that writes them. - @Volatile private var latestLocation: LocationResult = LocationResult.Loading + // Read by R1 below to decide whether to emit Loading, from a different + // coroutine than the onEach that writes it — hence a StateFlow rather than + // a plain field. + private val latestLocation = MutableStateFlow(LocationResult.Loading) - @Volatile private var latestPreciseLocation: LocationResult = LocationResult.Loading + private val latestPreciseLocation = MutableStateFlow(LocationResult.Loading) fun setLocationPermission(newValue: Boolean) { if (newValue != hasLocationPermission.value) { @@ -152,60 +160,59 @@ class LocationState( }.distinctUntilChanged() @OptIn(ExperimentalCoroutinesApi::class) - private fun geohashFlow( + private fun buildGeohashStateFlow( tag: String, charsCount: Int, minTimeMs: Long, minDistanceM: Float, - latest: () -> LocationResult, - setLatest: (LocationResult) -> Unit, - ): Flow = - gate.transformLatest { state -> - when (state) { - // Deliberately does NOT write to the cache. Today's code emits - // LackPermission without touching latestLocation, and wiping it - // here would cost a Loading emission — and so an empty-feed - // flash — on every permission flap, which is the regression R1 - // exists to prevent. Consumers already see LackPermission from - // the StateFlow; the cache is internal and only decides whether - // Loading is emitted. - Gate.NoPermission -> emit(LocationResult.LackPermission) + cache: MutableStateFlow, + ): StateFlow = + gate + .transformLatest { state -> + when (state) { + // Deliberately does NOT write to the cache. Today's code emits + // LackPermission without touching the cache, and wiping it + // here would cost a Loading emission — and so an empty-feed + // flash — on every permission flap, which is the regression R1 + // exists to prevent. Consumers already see LackPermission from + // the StateFlow; the cache is internal and only decides whether + // Loading is emitted. + Gate.NoPermission -> emit(LocationResult.LackPermission) - // Emit nothing: stateIn keeps the last value, so every - // synchronous .value reader still sees the last known geohash - // while the OS registration is released. - Gate.Paused -> Unit + // Emit nothing: stateIn keeps the last value, so every + // synchronous .value reader still sees the last known geohash + // while the OS registration is released. + Gate.Paused -> Unit - Gate.Listen -> { - // Only when there is nothing cached. Emitting Loading on - // every foreground return would flash the "Around Me" feed - // empty, because AroundMeFeedFlow.convert maps anything - // that is not Success to an empty geotag set. - if (latest() !is LocationResult.Success) emit(LocationResult.Loading) + Gate.Listen -> { + // Only when there is nothing cached. Emitting Loading on + // every foreground return would flash the "Around Me" feed + // empty, because AroundMeFeedFlow.convert maps anything + // that is not Success to an empty geotag set. + if (cache.value !is LocationResult.Success) emit(LocationResult.Loading) - emitAll( - locationSource(minTimeMs, minDistanceM) - .map { LocationResult.Success(it.toGeoHash(charsCount)) as LocationResult } - .onEach { setLatest(it) } - .catch { e -> - Log.w(tag, "Exception in the flow", e) - setLatest(LocationResult.LackPermission) - emit(LocationResult.LackPermission) - }, - ) + emitAll( + locationSource(minTimeMs, minDistanceM) + .map { LocationResult.Success(it.toGeoHash(charsCount)) as LocationResult } + .onEach { cache.value = it } + .catch { e -> + Log.w(tag, "Exception in the flow", e) + cache.value = LocationResult.LackPermission + emit(LocationResult.LackPermission) + }, + ) + } } - } - } + }.stateIn(scope, SharingStarted.WhileSubscribed(SUBSCRIPTION_STOP_TIMEOUT_MS), cache.value) val geohashStateFlow: StateFlow by lazy { - geohashFlow( + buildGeohashStateFlow( tag = "GeohashStateFlow", charsCount = GeohashPrecision.KM_5_X_5.digits, minTimeMs = COARSE_MIN_TIME, minDistanceM = COARSE_MIN_DISTANCE, - latest = { latestLocation }, - setLatest = { latestLocation = it }, - ).stateIn(scope, SharingStarted.WhileSubscribed(5000), latestLocation) + cache = latestLocation, + ) } /** @@ -221,13 +228,12 @@ class LocationState( * app ever requests `ACCESS_FINE_LOCATION`. */ val preciseGeohashStateFlow: StateFlow by lazy { - geohashFlow( + buildGeohashStateFlow( tag = "PreciseGeohashStateFlow", charsCount = GeohashChannelLevel.BUILDING.chars, minTimeMs = PRECISE_MIN_TIME, minDistanceM = PRECISE_MIN_DISTANCE, - latest = { latestPreciseLocation }, - setLatest = { latestPreciseLocation = it }, - ).stateIn(scope, SharingStarted.WhileSubscribed(5000), latestPreciseLocation) + cache = latestPreciseLocation, + ) } } diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/resourceusage/RefCountedSession.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/resourceusage/RefCountedSession.kt index f73e4996a4..4ef51cc3cc 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/resourceusage/RefCountedSession.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/resourceusage/RefCountedSession.kt @@ -34,17 +34,14 @@ package com.vitorpamplona.amethyst.service.resourceusage * off with a holder still active. * * Takes the setter as a lambda rather than a [SessionTimeIntegrator] because - * that is all it needs — and because constructing a real integrator drags in a - * [ResourceUsageAccountant] and a store file to observe one boolean. + * that is all it needs, and so tests need no accountant or store file. * - * Reports **transitions only**, not every call. A 1 -> 2 acquire would otherwise - * re-enter [SessionTimeIntegrator.setActive] with the session already open, - * splitting one segment into two. That happens to be arithmetically harmless - * (`account()` adds each piece, and the pieces are contiguous), and it does not - * inflate a `*.starts` counter either, because [SessionTimeIntegrator] already - * guards its starts increment on `prev == null`. Transition-only is simply the - * contract the name implies, and it keeps the class honest for a future caller - * that reacts to the callback rather than integrating it. + * Reports **transitions only**, not every call — the contract the name implies, + * and what keeps the class honest for a future caller that reacts to the + * callback rather than integrating it. (For [SessionTimeIntegrator] specifically + * a 1 -> 2 re-entry would have been harmless: it splits one segment into two + * contiguous pieces that `account()` re-adds, and its starts increment is + * already guarded on `prev == null`.) * * Releases must be paired with acquires. This class cannot tell an unpaired * release from a real one, so callers guarantee the pairing; see `LocationFlow`, diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/service/location/LocationFlowTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/location/LocationFlowTest.kt index 5329691f00..153a011f3f 100644 --- a/amethyst/src/test/java/com/vitorpamplona/amethyst/service/location/LocationFlowTest.kt +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/location/LocationFlowTest.kt @@ -38,25 +38,10 @@ import org.junit.Test @OptIn(ExperimentalCoroutinesApi::class) class LocationFlowTest { - /** - * A LocationManager that reports [providers] and refuses [denied] with a - * SecurityException, mimicking the pre-API-31 fine-location requirement. - */ private fun manager( providers: List, denied: Set = emptySet(), - ): LocationManager { - val lm = mockk(relaxed = true) - every { lm.allProviders } returns providers - every { lm.getLastKnownLocation(any()) } returns null - every { - lm.requestLocationUpdates(any(), any(), any(), any(), any()) - } answers { - val provider = firstArg() - if (provider in denied) throw SecurityException("denied: $provider") - } - return lm - } + ) = mockLocationManager(providers, denied) @Test fun firesNeitherEdgeWhenNoProviderExists() = @@ -180,6 +165,25 @@ class LocationFlowTest { verify(exactly = 0) { lm.getLastKnownLocation(any()) } } + @Test + fun seedsOnlyFromTheProvidersTheLadderDeemedUsable() = + runTest { + // On the coarse-only legacy path the ladder narrows to `network`, + // and gps/passive/fused would each throw SecurityException. Sweeping + // every provider the device reports would pay those guaranteed + // throws — stack trace plus an eagerly-interpolated Log.w — on every + // flow start, i.e. on every foreground return. + val lm = manager(providers = listOf("fused", "network", "gps", "passive")) + val job = launch { LocationFlow(lm, sdkInt = 30).get(60_000L, 500f).collect { } } + + runCurrent() + + verify(exactly = 1) { lm.getLastKnownLocation("network") } + verify(exactly = 0) { lm.getLastKnownLocation(neq("network")) } + + job.cancelAndJoin() + } + @Test fun registersOnExactlyOneProvider() = runTest { diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/service/location/LocationLedgerCompositionTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/location/LocationLedgerCompositionTest.kt new file mode 100644 index 0000000000..3f8b0eaf31 --- /dev/null +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/location/LocationLedgerCompositionTest.kt @@ -0,0 +1,243 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.amethyst.service.location + +import android.content.Context +import android.location.LocationListener +import android.location.LocationManager +import com.vitorpamplona.amethyst.service.resourceusage.RefCountedSession +import io.mockk.every +import io.mockk.mockk +import io.mockk.verify +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.cancelAndJoin +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.launch +import kotlinx.coroutines.test.UnconfinedTestDispatcher +import kotlinx.coroutines.test.advanceTimeBy +import kotlinx.coroutines.test.advanceUntilIdle +import kotlinx.coroutines.test.runTest +import org.junit.Assert.assertEquals +import org.junit.Test + +/** + * Wires the *real* [LocationState] -> [LocationFlow] -> [RefCountedSession] + * seam together, exercising [LocationState]'s **default** `locationSource` + * rather than overriding it the way [LocationStateTest] and [LocationFlowTest] + * do. Each of those files mocks the other layer out, so neither can catch a + * regression in the composition itself — e.g. a refcount that gets pinned + * open (or closed) because `onListening` fires in an order or multiplicity + * the two independent [LocationState] flows didn't anticipate. That failure + * mode is unrecoverable at runtime: a pinned-open refcount means + * `location.ms` accrues in the resource-usage ledger forever, with nothing + * actually listening. + * + * Same dispatcher note as [LocationStateTest]: `runTest {}`'s default + * `StandardTestDispatcher` does not drive this `combine` + `transformLatest` + + * `stateIn(WhileSubscribed)` + `backgroundScope.launch { collect }` chain to + * completion via `advanceUntilIdle()` alone, so every test here runs on + * `UnconfinedTestDispatcher`. + */ +@OptIn(ExperimentalCoroutinesApi::class) +class LocationLedgerCompositionTest { + /** A [LocationManager] that reports two providers and never denies registration. */ + private fun locationManager(): LocationManager = mockLocationManager(providers = listOf("fused", "network")) + + /** A [Context] whose `LOCATION_SERVICE` lookup returns [locationManager]. */ + private fun contextWithLocationService(locationManager: LocationManager): Context { + val context = mockk(relaxed = true) + every { context.getSystemService(Context.LOCATION_SERVICE) } returns locationManager + return context + } + + /** + * Wires a real [LocationState] to a real [RefCountedSession], standing in + * for the resource-usage ledger session. Deliberately does NOT override + * `locationSource` — that is the whole point of this file. + * + * Exposes [locationManager] so a test can `verify` against it directly — + * e.g. that exactly one of two concurrent registrations was torn down — + * rather than inferring release from the ledger alone, which cannot tell + * "released" from "never released" on its own (see + * `bothFlowsActiveThenOneStopsKeepsTheLedgerOpen`). + */ + private class Harness( + scope: CoroutineScope, + context: Context, + val locationManager: LocationManager, + ) { + /** Stand-in for `SessionTimeIntegrator.setActive`, i.e. the ledger. */ + val ledger = mutableListOf() + private val refCount = RefCountedSession { ledger.add(it) } + val foreground = MutableStateFlow(true) + val state = + LocationState( + context = context, + scope = scope, + isForeground = foreground, + onListening = { refCount.setActive(it) }, + ) + } + + private fun harness(scope: CoroutineScope): Harness { + val locationManager = locationManager() + return Harness(scope, contextWithLocationService(locationManager), locationManager) + } + + /** + * The interleaving [RefCountedSession] exists for. Two independent + * [LocationState] flows both register with the OS; the ledger must open + * once, not twice. Stopping one of the two must NOT close the ledger, + * because the other is still listening — this half is exactly what a + * per-flow (rather than refcounted) `onListening` -> ledger wiring would + * get wrong, and what a per-layer test (mocking `locationSource`, or + * driving `RefCountedSession` directly with synthetic booleans) cannot + * see, because it never lets two real [LocationFlow] registrations race + * each other through the shared hook. + * + * Asserts its premise, not just its consequence. The ledger staying at + * `[true]` after the precise flow stops is also what you'd see if that + * flow's registration never released at all — `onListening` deleted from + * [LocationFlow]'s `finally`, cleanup moved somewhere unreached, or the + * [LocationState.SUBSCRIPTION_STOP_TIMEOUT_MS] teardown simply hadn't + * elapsed. The + * `removeUpdates` verify below rules out the OS-registration side of + * that; cancelling the coarse flow too and checking the full `[true, + * false]` sequence at the end rules out the `onListening` side, since + * `removeUpdates` alone fires unconditionally in the `finally` and would + * not by itself notice `onListening` going missing. + */ + @Test + fun bothFlowsActiveThenOneStopsKeepsTheLedgerOpen() = + runTest(UnconfinedTestDispatcher()) { + val harness = harness(backgroundScope) + harness.state.setLocationPermission(true) + + val preciseJob = backgroundScope.launch { harness.state.preciseGeohashStateFlow.collect { } } + val coarseJob = backgroundScope.launch { harness.state.geohashStateFlow.collect { } } + advanceUntilIdle() + + assertEquals( + "both flows registering with the OS must open the ledger exactly once, not twice", + listOf(true), + harness.ledger, + ) + + preciseJob.cancelAndJoin() + // The cancelled collector alone doesn't tear down the upstream + // registration — WhileSubscribed keeps it alive for a grace + // period first. Advance past it so the release below is + // actually observable, not just "not yet due". + advanceTimeBy(LocationState.SUBSCRIPTION_STOP_TIMEOUT_MS + 1) + advanceUntilIdle() + + // Exactly one of the two OS registrations should have been torn + // down at this point — the precise one — while the coarse one is + // still live. + verify(exactly = 1) { harness.locationManager.removeUpdates(any()) } + + assertEquals( + "the coarse flow is still listening; stopping the precise one alone must not close the ledger", + listOf(true), + harness.ledger, + ) + + // Stop the coarse flow too, so the ledger closing here proves + // the release path genuinely works end-to-end for this test's + // own harness — not merely that the assertions above never + // exercised it. + coarseJob.cancelAndJoin() + advanceTimeBy(LocationState.SUBSCRIPTION_STOP_TIMEOUT_MS + 1) + advanceUntilIdle() + + verify(exactly = 2) { harness.locationManager.removeUpdates(any()) } + + assertEquals( + "both flows stopping must close the ledger exactly once, proving the precise flow's earlier release was real", + listOf(true, false), + harness.ledger, + ) + } + + /** + * The full foreground -> background cycle with both flows registered. + * The ledger must see exactly `[true, false]`: one open when the first + * flow registers (the second joining must not re-open it), one close + * once BOTH flows have released (not one close per flow, and not before + * the background grace period each flow independently waits out). + */ + @Test + fun fullCycleWithBothFlowsOpensAndClosesTheLedgerExactlyOnce() = + runTest(UnconfinedTestDispatcher()) { + val harness = harness(backgroundScope) + harness.state.setLocationPermission(true) + + backgroundScope.launch { harness.state.geohashStateFlow.collect { } } + backgroundScope.launch { harness.state.preciseGeohashStateFlow.collect { } } + advanceUntilIdle() + + assertEquals(listOf(true), harness.ledger) + + harness.foreground.value = false + // Real grace-period wait, not a test artefact: settledForeground + // delays BACKGROUND_GRACE_MS before the gate reaches Paused for + // either flow. See LocationStateTest for the worked example this + // follows. + advanceTimeBy(LocationState.BACKGROUND_GRACE_MS + 1) + advanceUntilIdle() + + assertEquals( + "backgrounding must close the ledger exactly once, not once per flow, and not before the grace period", + listOf(true, false), + harness.ledger, + ) + } + + /** + * After a full open/close cycle, returning to the foreground must reopen + * the ledger exactly once — not once per flow, and not a second time on + * top of a close that never actually happened. + */ + @Test + fun returningToForegroundReopensTheLedgerExactlyOnce() = + runTest(UnconfinedTestDispatcher()) { + val harness = harness(backgroundScope) + harness.state.setLocationPermission(true) + + backgroundScope.launch { harness.state.geohashStateFlow.collect { } } + backgroundScope.launch { harness.state.preciseGeohashStateFlow.collect { } } + advanceUntilIdle() + + harness.foreground.value = false + advanceTimeBy(LocationState.BACKGROUND_GRACE_MS + 1) + advanceUntilIdle() + + harness.foreground.value = true + advanceUntilIdle() + + assertEquals( + "a return to foreground after a full close must reopen the ledger exactly once", + listOf(true, false, true), + harness.ledger, + ) + } +} diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/service/location/LocationProviderLadderTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/location/LocationProviderLadderTest.kt index 5928e9f5bd..9f804817ca 100644 --- a/amethyst/src/test/java/com/vitorpamplona/amethyst/service/location/LocationProviderLadderTest.kt +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/location/LocationProviderLadderTest.kt @@ -30,7 +30,7 @@ class LocationProviderLadderTest { fun modernDevicePrefersFusedThenFallsBackInOrder() { assertEquals( listOf("fused", "network", "gps", "passive"), - LocationProviderLadder.chooseProviders(sdkInt = 31, hasFine = false) { it in all }, + LocationProviderLadder.chooseProviders(sdkInt = 31) { it in all }, ) } @@ -40,25 +40,18 @@ class LocationProviderLadderTest { assertEquals( listOf("network", "passive"), - LocationProviderLadder.chooseProviders(sdkInt = 37, hasFine = false) { it in present }, + LocationProviderLadder.chooseProviders(sdkInt = 37) { it in present }, ) } @Test fun coarseOnlyBelowApi31GetsNetworkOnly() { // gps, passive and fused all required ACCESS_FINE_LOCATION before - // Android 12 (see Hypothesis H1 in the design spec). + // Android 12 (see Hypothesis H1 in the design spec), and Amethyst + // declares ACCESS_COARSE_LOCATION only. assertEquals( listOf("network"), - LocationProviderLadder.chooseProviders(sdkInt = 30, hasFine = false) { it in all }, - ) - } - - @Test - fun fineBelowApi31GetsTheFullLadder() { - assertEquals( - listOf("fused", "network", "gps", "passive"), - LocationProviderLadder.chooseProviders(sdkInt = 26, hasFine = true) { it in all }, + LocationProviderLadder.chooseProviders(sdkInt = 30) { it in all }, ) } @@ -68,7 +61,7 @@ class LocationProviderLadderTest { assertEquals( emptyList(), - LocationProviderLadder.chooseProviders(sdkInt = 28, hasFine = false) { it in present }, + LocationProviderLadder.chooseProviders(sdkInt = 28) { it in present }, ) } @@ -76,7 +69,7 @@ class LocationProviderLadderTest { fun noProvidersAtAllGetsNothing() { assertEquals( emptyList(), - LocationProviderLadder.chooseProviders(sdkInt = 37, hasFine = false) { false }, + LocationProviderLadder.chooseProviders(sdkInt = 37) { false }, ) } } diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/service/location/MockLocationManager.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/location/MockLocationManager.kt new file mode 100644 index 0000000000..95dba139b1 --- /dev/null +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/location/MockLocationManager.kt @@ -0,0 +1,50 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.amethyst.service.location + +import android.location.LocationListener +import android.location.LocationManager +import io.mockk.every +import io.mockk.mockk + +/** + * A [LocationManager] that reports [providers], has no cached fix, and refuses + * [denied] with a `SecurityException` — mimicking the pre-API-31 fine-location + * requirement. + * + * Shared by [LocationFlowTest] and [LocationLedgerCompositionTest] so the mock's + * surface tracks the binder calls [LocationFlow] actually makes in one place. + */ +internal fun mockLocationManager( + providers: List, + denied: Set = emptySet(), +): LocationManager { + val lm = mockk(relaxed = true) + every { lm.allProviders } returns providers + every { lm.getLastKnownLocation(any()) } returns null + every { + lm.requestLocationUpdates(any(), any(), any(), any(), any()) + } answers { + val provider = firstArg() + if (provider in denied) throw SecurityException("denied: $provider") + } + return lm +}