From 4bdac51db6fa424cee4ea553c98cc05596c4febe Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Tue, 27 Oct 2026 19:20:34 +0100 Subject: [PATCH 1/3] collection: Nav3 deferred transactions From 0d30809057a88e6c59520803808e19e827bc3718 Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Wed, 4 Nov 2026 09:26:41 +0100 Subject: [PATCH 2/3] ref(android): Refactor .navigation3.lifecycle ahead of staging Nav3 transactions (JAVA-785) (#6399) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Refactor .navigation3.lifecycle ahead of staging nav3 transactions when they're requested by a nav root without a lease. (Staging transactions is required in order to better support ViewPager2 pages and other scenarios that pre-build their back stacks before their nav roots are displayed.) The refactor lets us separate three responsibilities: - Lease coordination: NavLeaseCoordinator observes registered owners’ lifecycles and decides which SentryNavEffect may publish navigation state. - State publication: Each effect’s NavScopes publishes and withdraws its navigation data and transaction together when ownership changes. - Transaction management: Each effect gets a transaction handle that manages starting, finishing, binding, and restoring its transaction. The coordinator no longer retains transactions itself. Behavior is unchanged save for one corner case: when a SentryNavEffect regains the lease, its unfinished transaction is now also re-bound if the scope holds a finished transaction. Before, it was only re-bound into an empty scope. The update matches the rule for starting a transaction, and in practice shouldn't occur, since a finished SentryTracer removes itself from the scope. --- .../compose/navigation3/BackStackConverter.kt | 2 +- .../navigation3/BackStackEntryMapper.kt | 2 +- .../compose/navigation3/BackStackObserver.kt | 77 +- .../CoordinatedBackStackObserver.kt | 3 +- .../lifecycle/NavLeaseCoordinator.kt | 638 +++++----- .../compose/navigation3/lifecycle/NavScope.kt | 156 --- .../navigation3/lifecycle/NavScopes.kt | 438 +++---- .../navigation3/lifecycle/NavTransaction.kt | 71 ++ .../compose/navigation3/lifecycle/README.md | 24 +- .../navigation3/lifecycle/StagedNavData.kt | 16 +- .../lifecycle/StagedNavTransaction.kt | 143 +++ .../CoordinatedBackStackObserverTest.kt | 28 +- .../lifecycle/NavLeaseCoordinatorTest.kt | 1049 +++++++++-------- .../navigation3/lifecycle/NavScopeTest.kt | 452 ++++--- .../navigation3/lifecycle/NavScopesTest.kt | 472 ++++++-- .../lifecycle/NavTransactionTest.kt | 125 ++ .../lifecycle/StagedNavTransactionTest.kt | 398 +++++++ 17 files changed, 2623 insertions(+), 1471 deletions(-) delete mode 100644 sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavScope.kt create mode 100644 sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavTransaction.kt create mode 100644 sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavTransaction.kt create mode 100644 sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavTransactionTest.kt create mode 100644 sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavTransactionTest.kt diff --git a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/BackStackConverter.kt b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/BackStackConverter.kt index 6100c706202..4bd0bd24e57 100644 --- a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/BackStackConverter.kt +++ b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/BackStackConverter.kt @@ -450,7 +450,7 @@ internal data class NormalizedSentryBackStackEntry( internal fun List.serialize(): List> = map(NormalizedSentryBackStackEntry::serialize) -/** Why host-provided arguments were unavailable in emitted navigation data. */ +/** Why host-provided arguments were unavailable in emitted nav state. */ internal enum class ArgumentDropReason( val serializedValue: String, val exhaustsUpdateBudget: Boolean = false, diff --git a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/BackStackEntryMapper.kt b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/BackStackEntryMapper.kt index 7455cfda5ff..d26336d1066 100644 --- a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/BackStackEntryMapper.kt +++ b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/BackStackEntryMapper.kt @@ -78,7 +78,7 @@ public class SentryBackStackEntry( * * If [map] throws, returns `null`, or returns a blank [name][SentryBackStackEntry.name], Sentry * records the destination as "/unknown". Doing so signals that name extraction needs to be fixed - * while avoiding misleading gaps in navigation data. + * while avoiding misleading gaps in nav state. * * For instance, if a user navigates from `/home -> /detail -> /settings`, but the mapper for * `/detail` throws, the back stack record will be `/home -> /unknown -> /settings` rather than diff --git a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/BackStackObserver.kt b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/BackStackObserver.kt index 8e2806686e5..2c4fd1fb14d 100644 --- a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/BackStackObserver.kt +++ b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/BackStackObserver.kt @@ -2,12 +2,9 @@ package io.sentry.compose.navigation3 import io.sentry.Breadcrumb import io.sentry.Hint -import io.sentry.ITransaction import io.sentry.SentryIntegrationPackageStorage -import io.sentry.SentryLevel.DEBUG import io.sentry.SentryLevel.INFO import io.sentry.SentryOptions -import io.sentry.SpanStatus import io.sentry.TransactionContext import io.sentry.TransactionOptions import io.sentry.compose.navigation3.BackStackConverter.RetentionPolicy @@ -16,6 +13,7 @@ import io.sentry.compose.navigation3.PreparedChange.BackStackHasSameTop import io.sentry.compose.navigation3.PreparedChange.BackStackIsEmpty import io.sentry.compose.navigation3.lifecycle.INavScope import io.sentry.compose.navigation3.lifecycle.INavScopes +import io.sentry.compose.navigation3.lifecycle.INavTransaction import io.sentry.compose.navigation3.lifecycle.NAVIGATION_CONTEXT_KEY import io.sentry.protocol.App import io.sentry.protocol.TransactionNameSource @@ -59,7 +57,7 @@ internal class BackStackObserver( private val backStackConverter = BackStackConverter(entryMapper, scopes.options.logger) - private val navTransaction = NavTransaction(scopes) + private val navTransactions = NavTransactions(scopes) private val navBreadcrumbs = NavBreadcrumbs(scopes) private val navScreen = NavScreen() private val navContext = NavContext(scopes, options) @@ -83,7 +81,7 @@ internal class BackStackObserver( } /** - * Updates Sentry nav data based on the provided [backStack]. + * Updates Sentry nav state based on the provided [backStack]. * * **Data generated** * @@ -113,7 +111,7 @@ internal class BackStackObserver( } /** - * Clears the nav data this observer recorded. + * Clears the nav state this observer recorded. * * Call it when observation ends so that no stale screen, nav context, or nav transaction outlives * it. @@ -194,10 +192,10 @@ internal class BackStackObserver( navBreadcrumbs.emit(fromEntry = previousTop, toEntry = currentBackStack.topEntryNormalized) } - navTransaction.stop(scope) + navTransactions.stop(scope) if (areNavigationTransactionsEnabled) { - navTransaction + navTransactions .start( scope, currentTop.name, @@ -231,7 +229,7 @@ internal class BackStackObserver( } private fun tearDown(scope: INavScope) { - navTransaction.stop(scope) + navTransactions.stop(scope) navContext.clear(scope) navScreen.clear(scope) clearPreviousTop() @@ -277,32 +275,22 @@ private data class BackStackData( ) /** A helper class for managing nav transactions. */ -private class NavTransaction(private val scopes: INavScopes) { +private class NavTransactions(private val scopes: INavScopes) { private companion object { private const val TRANSACTION_ORIGIN = "auto.navigation.nav3" } - private var activeNavTransaction: ITransaction? = null - - /** Starts an idle navigation transaction, or no-ops if another transaction is already active. */ + /** + * Starts an idle navigation transaction, or no-ops if another transaction is already active. + * + * [Stop][stop] the current navigation transaction first. See [INavScope.startNavTransaction]. + */ fun start( scope: INavScope, name: String, arguments: Map, - ): ITransaction? { - scope.clearFinishedTransaction() - - if (scope.transaction != null) { - scopes.options.logger.log( - DEBUG, - "Nav3 transaction for route %s won't be created because another transaction is active.", - name, - ) - - return null - } - + ): INavTransaction? { val transactionOptions = TransactionOptions().also { it.isWaitForChildren = true @@ -314,36 +302,25 @@ private class NavTransaction(private val scopes: INavScopes) { } val transaction = - scopes.startTransaction( + scope.startNavTransaction( TransactionContext(name, TransactionNameSource.ROUTE, NAVIGATION_OP), transactionOptions, - ) + ) ?: return null - // Not an error: INavScopes.startTransaction may decline to start a transaction by design. - if (transaction.isNoOp) { - return null + if (arguments.isNotEmpty()) { + transaction.setData("arguments", arguments) } - activeNavTransaction = transaction - - transaction.apply { - if (arguments.isNotEmpty()) { - setData("arguments", arguments) - } - } - - scope.bindTransaction(transaction) - return transaction } - /** Finishes and unsets the active navigation transaction, if one exists. */ + /** + * Finishes and unsets the active navigation transaction, if one exists. + * + * See [INavScope.finishNavTransaction]. + */ fun stop(scope: INavScope) { - val transaction = activeNavTransaction ?: return - val status = transaction.status ?: SpanStatus.OK - transaction.finish(status) - scope.unbindTransaction(transaction) - activeNavTransaction = null + scope.finishNavTransaction() } } @@ -377,15 +354,15 @@ private class NavContext(private val scopes: INavScopes, private val options: Se * proper values to the transaction context instead. */ fun updateTransaction( - transaction: ITransaction, + transaction: INavTransaction, scope: INavScope, backStack: BackStackData, ) { if (scopes.options.isEnableScreenTracking) { - val appContext = transaction.contexts.app ?: scope.app ?: App() + val appContext = scope.app ?: App() appContext.viewNames = listOf(backStack.topEntryNormalized.name) - transaction.contexts.setApp(appContext) + transaction.setApp(appContext) } if (options.captureBackStack && backStack.capturedEntriesNormalized.isNotEmpty()) { diff --git a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/CoordinatedBackStackObserver.kt b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/CoordinatedBackStackObserver.kt index 8cf9c6e1be3..a34a214decc 100644 --- a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/CoordinatedBackStackObserver.kt +++ b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/CoordinatedBackStackObserver.kt @@ -22,7 +22,8 @@ import io.sentry.compose.navigation3.lifecycle.NavScopes * **Thread safety** * * This class is ***not*** thread-safe. Invocations should be confined to the same thread as the - * [LifecycleEventObserver] callbacks implemented by [NavScopes]. + * [LifecycleEventObserver] callbacks through which the [NavLeaseCoordinator] observes the attached + * lifecycle. */ internal class CoordinatedBackStackObserver( scopes: IScopes, diff --git a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinator.kt b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinator.kt index 87b0f8f75ba..9f8a0f87fde 100644 --- a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinator.kt +++ b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinator.kt @@ -1,30 +1,149 @@ package io.sentry.compose.navigation3.lifecycle import androidx.lifecycle.Lifecycle +import androidx.lifecycle.Lifecycle.Event.ON_DESTROY +import androidx.lifecycle.Lifecycle.Event.ON_PAUSE +import androidx.lifecycle.Lifecycle.Event.ON_RESUME +import androidx.lifecycle.Lifecycle.Event.ON_START +import androidx.lifecycle.Lifecycle.Event.ON_STOP +import androidx.lifecycle.LifecycleEventObserver import androidx.lifecycle.LifecycleOwner import io.sentry.IScope -import io.sentry.ITransaction -import io.sentry.SpanStatus +import io.sentry.IScopes /** * Manages a lease in order to determine which [NavScopes] instance can bind nav transactions and * publish nav data to a shared [IScope]. * - * `NavScopes` instances register as [candidates][Candidate] for lease ownership via - * [registerCandidate]. + * `NavScopes` instances register as [candidates][Candidate] for lease ownership via [register]. + * Each candidate observes its owner's lifecycle until it's disposed. * - * Note: "Owner" below refers to the `LifecycleOwner` whose lifecycle events the candidate's - * `NavScopes` relays. + * Note: "Owner" below refers to the `LifecycleOwner` whose lifecycle the candidate observes. * - * **Exception semantics** + * **A fundamental tradeoff** + * + * During `LifecycleOwner` overlap, the nav transaction and scoped nav data can reflect + * **_either_**: + * + * 1. the outgoing owner that's still visible, or + * 2. the incoming owner that may crash before it becomes visible, + * + * but not both. This class opts for (2) so that the incoming destination populates crash context + * and any work performed is associated with it† – the idea being that start-up logic is more likely + * to cause a crash or produce interesting spans than an outgoing destination's steady state. + * + * † Associating work with the incoming transaction means spans produced by the outgoing owner may + * appear as if they were produced by the incoming owner (or vice versa, were our policy reversed). + * A possible solution would be to implement a parent span delivery system similar to + * `LocalSentrySpan`, but that's a task for another day. + * + * **Lease handoff policy** + * + * The lease is acquired preemptively by an incoming owner's `NavScopes` instance when it applies + * its first [back stack update][onBackStackUpdate].† + * + * A displaced owner remains eligible for lease restoration until it + * [stops][Lifecycle.Event.ON_STOP]. It gets the lease back when it + * [resumes][Lifecycle.Event.ON_RESUME], or sooner if the new owner leaves or pauses while the + * displaced owner is still visible. If restored, the displaced owner's nav data and unfinished + * transaction are reinstated. + * + * † There are some exceptions, e.g., the leaseholder is resumed and the incoming owner isn't (e.g., + * a preloaded ViewPager2 Fragment page next to the current one). Such an owner acquires the lease + * when it resumes instead. + * + * The following diagram illustrates how handoff works: + * ``` + * Time + * │ + * ▼ + * + * NavScopes A + Lifecycle A Shared state NavScopes B + Lifecycle B + * ───────────────────────── ──────────── ───────────────────────── + * + * attach() Navigation lease: none + * │ + * Initial back stack update + * │ + * ├── acquire lease ────────► Navigation lease: A + * ├── create & bind txA ────► Scope transaction: txA + * ├── publish nav data ─────► Published data: A + * │ + * ON_RESUME A is eligible for restoration + * │ + * ON_PAUSE + * │ + * ├────────────────────────► Navigation lease: A (no other resumed owner) + * │ A remains eligible for restoration + * │ + * │ attach() + * │ │ + * │ Navigation lease: A ◄────────────────────┤ + * │ Scope transaction: txA │ + * │ Published data: A │ + * │ B registered, awaiting initial back │ + * │ stack update │ + * │ │ + * │ Initial back stack update + * │ │ + * │ Suspend & unbind txA ◄───────────────────┤ + * │ Navigation lease: B │ + * │ Published data: B │ + * │ Scope transaction: txB │ + * │ (rollback point) │ + * │ │ + * │ ON_RESUME + * │ │ + * │ B eligible for restoration ◄─────────────┤ + * │ │ + * ON_STOP │ + * │ │ + * ├── retain txA; timers continue │ + * ├────────────────────────► A no longer eligible for restoration │ + * │ │ + * ON_DESTROY │ + * │ │ + * ├── finish retained txA if unfinished │ + * └────────────────────────► A no longer tracked │ + * ON_PAUSE + * │ + * Navigation lease: B (no other resumed ◄─┤ + * owner) │ + * │ + * ON_STOP + * │ + * Navigation lease: B (no other resumed ◄─┤ + * owner); B keeps recording navigation │ + * B no longer eligible for restoration │ + * │ + * ON_DESTROY + * │ + * Finish txB if unfinished; ◄──────────────┤ + * unbind if still bound │ + * Clear B's remaining screen/back stack │ + * Retain recorded nav events │ + * Navigation lease: none │ + * ▼ + * + * Alternative at rollback point: B disposes before A stops + * Unbind txB ◄─────────────── dispose B ───┤ + * Navigation lease: A │ + * Republish A's staged data │ + * Restore txA if unfinished and scope free │ + * Finish txB if unfinished │ + * ``` + * + * **Exception policy** * * Methods in this class with an `IScope` parameter can throw arbitrary exceptions†, which propagate - * to the caller. Each method updates its own bookkeeping before calling into the scope, so the - * coordinator remains usable if the exception is caught. + * to the caller. Each method updates its own bookkeeping before calling into the scope or a + * candidate's [LeasedNavState], so the coordinator remains usable if the exception is caught. + * Candidates call these methods from [configureScope][IScopes.configureScope] callbacks, whose + * error handling catches the exceptions. * * † E.g., `IScope` can throw arbitrary exceptions when scope mutators notify registered observers – - * including the host app – in an unguarded loop, or when `finish()` is called on a retained nav - * transaction, which can trigger the host app's event processors. + * including the host app – in an unguarded loop, or when starting a nav transaction triggers the + * host app's callbacks. * * **Thread safety** * @@ -36,195 +155,47 @@ internal class NavLeaseCoordinator { private val candidates = mutableListOf() - /** Candidate responsible for the most recently published nav data in the shared scope. */ - private var publishedDataCandidate: Candidate? = null + /** Candidate whose nav state was most recently published to the shared scope. */ + private var publisher: Candidate? = null private var leaseHolder: Candidate? = null /** * Counter that orders candidates by when this coordinator most recently observed them in resumed * state. - * - * Each resume, including registration of an already-resumed owner, assigns the next value to that - * candidate's [Candidate.resumeOrder]. This lets fallback selection prefer the most recently - * resumed eligible candidate without relying on clock timestamps. Zero is reserved for candidates - * that have not been observed resumed and therefore cannot be chosen as fallbacks. */ private var resumeOrder: Long = 0 /** - * Registers [owner] as a candidate for the nav lease. No-ops if `owner` instance is already - * registered or [lifecycleState] is [destroyed][Lifecycle.State.DESTROYED]. - * - * [onPublishData] publishes `owner`'s staged nav data to the given scope when `owner` acquires - * the lease. [onClearPublishedData] clears `owner`'s published nav data from the given scope when - * its data is the most recently published and either another owner acquires the lease or `owner` - * is unregistered. + * Registers a candidate for the nav lease that observes [lifecycle] until it's + * [disposed][NavLeaseRegistration.dispose] or `lifecycle` is destroyed. Returns `null` if + * `lifecycle` is already [destroyed][Lifecycle.State.DESTROYED]. */ - fun registerCandidate( - owner: Any, - lifecycleState: Lifecycle.State, - onPublishData: (IScope) -> Unit, - onClearPublishedData: (IScope) -> Unit, - ) { - if (findCandidate(owner) != null) { - return - } - + fun register( + lifecycle: Lifecycle, + scopes: IScopes, + navState: LeasedNavState, + ): NavLeaseRegistration? { + val lifecycleState = lifecycle.currentState if (lifecycleState == Lifecycle.State.DESTROYED) { - return + return null } val initialResumeOrder = if (lifecycleState == Lifecycle.State.RESUMED) ++resumeOrder else 0 - candidates += - Candidate(owner, onPublishData, onClearPublishedData, lifecycleState, initialResumeOrder) - } - - /** Records that [owner] started. */ - fun startCandidate(owner: Any) { - findCandidate(owner)?.start() - } - - /** - * Records that [owner] resumed. - * - * Restores the lease to `owner` if it already applied a back stack update and [scope] is - * non-null. - * - * A `null` scope (e.g., the SDK is disabled) still records the lifecycle transition so the - * candidate remains eligible for fallback once a scope becomes available. - * - * Can throw arbitrary exceptions. See "Exception semantics" in the class KDoc. - */ - fun resumeCandidate(owner: Any, scope: IScope?) { - findCandidate(owner)?.let { - it.resume(++resumeOrder) - - if (scope != null && it.hasAppliedInitialBackStackUpdate) { - transferLeaseTo(it, scope) - } - } - } - - /** - * Records that [owner] paused. - * - * Hands the lease to another owner only if that owner is resumed (e.g., a Fragment whose dialog - * is being dismissed); otherwise keeps it. A paused leaseholder may still be the topmost visible - * owner (e.g., beneath a translucent window or in picture-in-picture mode), and any other paused - * owner is beneath it. A displaced owner gets the lease back when it resumes. - * - * A `null` scope (e.g., the SDK is disabled) still records the lifecycle transition so the - * candidate remains eligible for fallback once a scope becomes available. - * - * Can throw arbitrary exceptions. See "Exception semantics" in the class KDoc. - */ - fun pauseCandidate(owner: Any, scope: IScope?) { - findCandidate(owner)?.let { - it.pause() - - if (leaseHolder === it) { - mostRecentlyResumedCandidate(excluding = it, resumedOnly = true)?.let { next -> - handOffLease(next, scope) - } - } - } - } - - /** - * Records that [owner] stopped. - * - * As with pausing, hands the lease to another owner only if that owner is resumed; otherwise the - * stopped owner keeps it. Keeping it lets a lone owner go on recording navigation that happens - * while it isn't resumed (e.g., while the app is in the background, or after it restarts but - * before it resumes), including starting nav transactions. - * - * A paused owner doesn't get the lease: when the leaseholder stops before it, the app is - * typically going to the background (e.g., a translucent leaseholder stops before the owner - * beneath it), and that owner isn't what the user last saw. A paused owner that comes back to the - * foreground gets the lease when it resumes. - * - * A `null` scope (e.g., the SDK is disabled) still records the lifecycle transition so the - * candidate remains eligible for fallback once a scope becomes available. - * - * Can throw arbitrary exceptions. See "Exception semantics" in the class KDoc. - */ - fun stopCandidate(owner: Any, scope: IScope?) { - findCandidate(owner)?.let { - it.stop() - - if (leaseHolder === it) { - mostRecentlyResumedCandidate(resumedOnly = true)?.let { next -> handOffLease(next, scope) } - } - } - } - - /** - * Stops tracking [owner] (e.g., because it was destroyed or its effect left the composition). - * No-ops if `owner` instance isn't registered. - * - * Finishes and unbinds `owner`'s nav transaction, and clears its published nav data if that data - * is the most recently published. If `owner` holds the lease, hands it to the eligible candidate - * most recently observed resumed, even if that candidate is now only started or paused, or - * releases it if there's none. - * - * A `null` scope (e.g., the SDK is disabled) still removes the candidate, but its transaction is - * left to time out, its published data is left in place, and the lease is released rather than - * handed off. - * - * Can throw arbitrary exceptions. See "Exception semantics" in the class KDoc. Coordinator - * references to `owner` are released before any scope call, so they're released even if scope - * cleanup or lease handoff throws. The remaining scope cleanup is then skipped. - */ - fun unregisterCandidate(owner: Any, scope: IScope?) { - val candidate = findCandidate(owner) ?: return - val transaction = candidate.transaction - val wasPublisher = publishedDataCandidate === candidate - val wasLeaseHolder = leaseHolder === candidate - - candidates.remove(candidate) - candidate.transaction = null - if (wasPublisher) { - publishedDataCandidate = null - } - if (wasLeaseHolder) { - leaseHolder = null - } - - // No need to finish the transaction if we don't have a scope (e.g., the SDK is disabled), as - // the transaction can't be sent and it will time out anyway. Not finishing it has the added - // advantage of keeping SDK work inside `configureScope`'s error handling. - if (scope == null) { - return - } + val candidate = Candidate(lifecycle, scopes, navState, lifecycleState, initialResumeOrder) + candidates += candidate - finishTransaction(transaction, scope) - if (wasPublisher) { - candidate.onClearPublishedData(scope) - } - if (wasLeaseHolder) { - transferLeaseTo(mostRecentlyResumedCandidate(), scope) - } + lifecycle.addObserver(candidate) + return candidate } - fun hasLease(owner: Any): Boolean = leaseHolder?.owner === owner - /** - * Records that the registered [owner] is about to apply a back-stack update and acquires the nav - * lease if the owner's lifecycle permits it. + * Records that [candidate] is about to apply a back-stack update and acquires the nav lease if + * the owner's lifecycle permits it. * - * An owner that isn't resumed doesn't take the lease from a resumed leaseholder, as it isn't the - * one the user is interacting with (e.g., a preloaded ViewPager2 page next to the current one). - * Instead, it acquires the lease when it resumes. - * - * If the lease transfers, ownership and transaction binding are updated before the candidate's - * staged nav data is published. Publication completes before this method returns, so the caller - * can then apply the back-stack update. Later calls for the same [owner] do nothing. - * - * Can throw arbitrary exceptions. See "Exception semantics" in the class KDoc. + * Later calls for the same [candidate] do nothing. */ - fun onBackStackUpdate(owner: Any, scope: IScope) { - val candidate = findCandidate(owner) ?: return + private fun onBackStackUpdate(candidate: Candidate, scope: IScope) { if (candidate.hasAppliedInitialBackStackUpdate) { return } @@ -232,6 +203,10 @@ internal class NavLeaseCoordinator { candidate.hasAppliedInitialBackStackUpdate = true val holder = leaseHolder + + // An owner that isn't resumed doesn't take the lease from a resumed leaseholder, as it isn't + // the one the user is interacting with (e.g., a preloaded ViewPager2 page next to the current + // page). Instead, it acquires the lease when it resumes. val wouldDisplaceResumedHolder = holder != null && holder.state == Lifecycle.State.RESUMED && @@ -242,49 +217,66 @@ internal class NavLeaseCoordinator { } } - /** Whether [transaction] is a nav transaction retained for any registered owner. */ - fun isNavTransaction(transaction: ITransaction): Boolean = candidates.any { - it.transaction === transaction + /** + * Records that [candidate] resumed. + * + * Restores the lease to `candidate` if it already applied a back stack update and [scope] is + * non-null. + */ + private fun resumeCandidate(candidate: Candidate, scope: IScope?) { + // A null scope (e.g., the SDK is disabled) still records the lifecycle transition so that the + // candidate remains eligible for fallback once the scope becomes available. + candidate.resume(++resumeOrder) + + if (scope != null && candidate.hasAppliedInitialBackStackUpdate) { + transferLeaseTo(candidate, scope) + } } /** - * Records [transaction] as the nav transaction of [owner], which must hold the lease. Ignored - * otherwise, as only the leaseholder starts nav transactions. + * Records that [candidate] paused. * - * If [owner] loses the lease, the transaction is unbound but retained: it's rebound if [owner] - * regains the lease while it's still unfinished and the scope is free, and finished when [owner] - * is [unregistered][unregisterCandidate]. It's forgotten once [owner] retires it; see - * [releaseTransaction]. + * Hands the lease to another owner only if that owner is resumed (e.g., a Fragment whose dialog + * is being dismissed); otherwise keeps it. */ - fun retainTransaction(owner: Any, transaction: ITransaction) { - val candidate = leaseHolder ?: return - if (candidate.owner !== owner) { - return + private fun pauseCandidate(candidate: Candidate, scope: IScope?) { + candidate.pause() + + // A paused leaseholder may still be the topmost visible owner (e.g., beneath a translucent + // window or picture-in-picture mode), and any other paused owner is beneath it. A displaced + // owner will acquire the lease again when it resumes. + if (leaseHolder === candidate) { + mostRecentlyResumedCandidate(excluding = candidate, resumedOnly = true)?.let { next -> + handOffLease(next, scope) + } } - candidate.transaction = transaction } /** - * Forgets [transaction] as the nav transaction of [owner], if it's the one retained. Called when - * [owner] retires the transaction (e.g., it navigated away), so it's never rebound later. + * Records that [candidate] stopped. * - * Retiring is distinct from losing the lease: a transaction that's still waiting for child spans - * isn't [finished][ITransaction.isFinished] yet, so without this, regaining the lease would - * rebind a transaction whose destination [owner] has already left. + * As with pausing, hands the lease to another owner only if that owner is resumed; otherwise the + * stopped owner keeps it. Keeping it lets a lone owner go on recording navigation that happens + * while it isn't resumed (e.g., while the app is in the background, or after it restarts but + * before it resumes), including starting nav transactions. */ - fun releaseTransaction(owner: Any, transaction: ITransaction) { - val candidate = findCandidate(owner) ?: return - if (candidate.transaction === transaction) { - candidate.transaction = null + private fun stopCandidate(candidate: Candidate, scope: IScope?) { + candidate.stop() + + // A paused owner doesn't get the lease. When the leaseholder stops before it, the app is + // typically going to the background (e.g., a translucent leaseholder stops before the owner + // beneath it), and that owner isn't what the user last saw. A paused owner that comes back to + // the foreground will acquire the lease when it resumes. + if (leaseHolder === candidate) { + mostRecentlyResumedCandidate(resumedOnly = true)?.let { next -> handOffLease(next, scope) } } } - private fun findCandidate(owner: Any): Candidate? = candidates.firstOrNull { it.owner === owner } - /** - * The eligible candidate most recently observed resumed, other than [excluding]. With - * [resumedOnly], only candidates that are resumed right now are considered; otherwise, started - * and paused ones are too. + * The eligible candidate most recently observed to be in a resumed state, other than [excluding]. + * + * If [resumedOnly] is set to `true`, only candidates that are resumed right now are considered; + * if `false`, started and paused candidates are also considered. */ private fun mostRecentlyResumedCandidate( excluding: Candidate? = null, @@ -312,94 +304,74 @@ internal class NavLeaseCoordinator { } } - /** - * Hands the lease to [candidate], or releases it given `null`. - * - * Maintains the invariant that, while a lease is held, the leaseholder is also the candidate - * recorded as having published the shared scope's nav data. The invariant holds even when a - * publish or clear callback throws part way through the handoff. See "Exception semantics" in the - * class KDoc. - */ + /** Hands the lease to [candidate], or releases it given `null`. */ private fun transferLeaseTo(candidate: Candidate?, scope: IScope) { if (leaseHolder === candidate) { return } - if (candidate != null) { - // Unbind another owner's transaction without finishing it, so it can be restored if that - // owner returns. If this owner's retained transaction has since finished, discard it - // instead of binding a finished transaction to the shared scope. - scope.withTransaction { current -> - if (current != null && candidates.any { it !== candidate && it.transaction === current }) { - scope.clearTransaction() - } - } - - val transaction = candidate.transaction - if (transaction?.isFinished == true) { - candidate.transaction = null - clearTransactionFromScope(transaction, scope) - } else if (transaction != null) { - // The navigation lease doesn't give this owner control over an unrelated transaction - // already bound to the shared scope. Restore the owner's retained nav transaction only if - // the scope is free or already holds that same transaction. - scope.withTransaction { current -> - if (current == null || current === transaction) { - scope.transaction = transaction - } - } - } - } - - val previousPublisher = publishedDataCandidate + val previousPublisher = publisher leaseHolder = candidate if (candidate != null) { // Record the candidate as the publisher before calling into the scope: it owns the scope's - // nav data from the moment it takes the lease. If clearing the previous owner's data then - // throws, the next handoff still clears the candidate's data rather than never clearing it. - // - // Clear the previous publisher's nav data before publishing the candidate's, so the - // previous owner's values don't linger where the candidate has nothing staged to replace - // them. - publishedDataCandidate = candidate + // nav state from the moment it takes the lease. If unpublishing the previous owner's state + // then throws, the next handoff still unpublishes the candidate's state rather than never + // unpublishing it. + publisher = candidate if (previousPublisher != null && previousPublisher !== candidate) { - previousPublisher.onClearPublishedData(scope) + previousPublisher.navState.unpublishFrom(scope) } - candidate.onPublishData(scope) + candidate.navState.publishTo(scope) } } /** - * Finishes [transaction] if it's unfinished, and unbinds it from [scope]. + * Stops tracking [candidate] (e.g., because it was destroyed). * - * Callers must forget the transaction first. + * Unpublishes `candidate`'s nav state from [scope] if it's the most recently published. If + * `candidate` holds the lease, hands it to the eligible candidate most recently observed resumed, + * even if that candidate is now only started or paused, or releases it if there's none. */ - private fun finishTransaction(transaction: ITransaction?, scope: IScope) { - if (transaction == null) { - return + private fun unregisterCandidate(candidate: Candidate, scope: IScope?) { + val wasPublisher = publisher === candidate + val wasLeaseHolder = leaseHolder === candidate + + // Remove candidate before any scope calls to ensure it's released even if cleanup or handoff + // throws. + candidates.remove(candidate) + + if (wasPublisher) { + publisher = null } - if (!transaction.isFinished) { - transaction.finish(transaction.status ?: SpanStatus.OK) + if (wasLeaseHolder) { + leaseHolder = null } - clearTransactionFromScope(transaction, scope) - } - private fun clearTransactionFromScope(transaction: ITransaction, scope: IScope) { - scope.withTransaction { current -> - if (current === transaction) { - scope.clearTransaction() - } + // A null scope (e.g., the SDK is disabled) still removes the candidate, but its published nav + // state is left in place and the lease is released rather than handed off. + if (scope == null) { + return + } + + if (wasPublisher) { + candidate.navState.unpublishFrom(scope) + } + if (wasLeaseHolder) { + transferLeaseTo(mostRecentlyResumedCandidate(), scope) } } - /** A [LifecycleOwner] that has registered itself as a candidate for lease ownership. */ - private class Candidate( - val owner: Any, - val onPublishData: (IScope) -> Unit, - val onClearPublishedData: (IScope) -> Unit, + /** + * A [LifecycleOwner] registered as a candidate for lease ownership. Relays its owner's lifecycle + * events to the coordinator until it's [disposed][dispose]. + */ + private inner class Candidate( + private val lifecycle: Lifecycle, + private val scopes: IScopes, + val navState: LeasedNavState, initialState: Lifecycle.State, initialResumeOrder: Long, - ) { + ) : NavLeaseRegistration, LifecycleEventObserver { /** The candidate's lifecycle state, as last observed. */ var state: Lifecycle.State = initialState @@ -407,15 +379,13 @@ internal class NavLeaseCoordinator { /** * Whether the candidate has been observed STOPPED since - * [registration][NavLeaseCoordinator.registerCandidate]. + * [registration][NavLeaseCoordinator.register]. */ private var wasStopped: Boolean = false - var transaction: ITransaction? = null - /** * See [NavLeaseCoordinator.resumeOrder]. Non-zero once the candidate has been observed RESUMED - * since [registration][NavLeaseCoordinator.registerCandidate]. + * since [registration][NavLeaseCoordinator.register]. */ var resumeOrder: Long = initialResumeOrder private set @@ -424,14 +394,9 @@ internal class NavLeaseCoordinator { /** * Whether the candidate's lifecycle permits its first back stack update since - * [registration][NavLeaseCoordinator.registerCandidate] to acquire the nav lease, including - * taking it from another owner that isn't resumed: it's resumed, or it has neither resumed nor - * stopped since registration (i.e., it's still coming up for the first time). A paused or - * restarted candidate is returning to the screen rather than arriving on it. - * - * This checks the candidate's own lifecycle only; it does not indicate whether the candidate - * has already applied its first update, nor account for a resumed leaseholder (see - * [NavLeaseCoordinator.onBackStackUpdate]). + * [registration][NavLeaseCoordinator.register] to acquire the nav lease: it's resumed, or it + * has neither resumed nor stopped since registration (i.e., it's still coming up for the first + * time). */ val canAcquireLeaseOnFirstUpdate: Boolean get() = state == Lifecycle.State.RESUMED || (resumeOrder == 0L && !wasStopped) @@ -457,5 +422,112 @@ internal class NavLeaseCoordinator { state = Lifecycle.State.CREATED wasStopped = true } + + private var isDisposed = false + + override val hasLease: Boolean + get() = leaseHolder === this + + override fun onStateChanged(source: LifecycleOwner, event: Lifecycle.Event) { + when (event) { + ON_START -> start() + ON_RESUME -> withScopeOrNull { resumeCandidate(this, it) } + ON_PAUSE -> withScopeOrNull { pauseCandidate(this, it) } + ON_STOP -> withScopeOrNull { stopCandidate(this, it) } + ON_DESTROY -> dispose() + else -> Unit + } + } + + override fun onBackStackUpdate() { + if (isDisposed) { + return + } + scopes.configureScope { scope -> this@NavLeaseCoordinator.onBackStackUpdate(this, scope) } + } + + override fun dispose() { + if (isDisposed) { + return + } + isDisposed = true + lifecycle.removeObserver(this) + + withScopeOrNull { scope -> + try { + unregisterCandidate(this, scope) + } finally { + navState.onUnregistered(scope) + } + } + } + + /** + * Runs [block] with the real scope, or with `null` if [configureScope][IScopes.configureScope] + * skips its callback (e.g., the SDK is disabled), so coordinator bookkeeping still happens. + * + * A callback that ran but threw is not retried, as retrying could let its exception escape into + * the host app. + */ + private fun withScopeOrNull(block: (IScope?) -> Unit) { + var callbackInvoked = false + scopes.configureScope { scope -> + callbackInvoked = true + block(scope) + } + + if (!callbackInvoked) { + block(null) + } + } } } + +/** + * A candidate's registration with a [NavLeaseCoordinator], starting from + * [NavLeaseCoordinator.register] and continuing until it's [disposed][dispose] or its lifecycle is + * destroyed. + */ +internal interface NavLeaseRegistration { + + /** Whether the candidate holds the nav lease. */ + val hasLease: Boolean + + /** + * Announces that the candidate is about to apply a back stack update, so it can acquire the nav + * lease if it's eligible to. Does nothing once the candidate is unregistered. + */ + fun onBackStackUpdate() + + /** + * Stops observing the candidate's lifecycle and unregisters it: releases the lease, unpublishes + * the candidate's nav state, and then calls [LeasedNavState.onUnregistered], even if + * unregistering throws. Does nothing once the candidate is unregistered. + */ + fun dispose() +} + +/** + * The nav state a [NavLeaseCoordinator] candidate publishes to the shared [IScope] while it holds + * the nav lease. + * + * Nav transactions and nav data each occupy a single, mutually-exclusive slot in the `IScope`, so + * only one candidate's nav state can be published at a time. Unpublishing nav state removes it from + * the scope but keeps it staged, so the candidate can publish it again if it regains the lease. + */ +internal interface LeasedNavState { + + /** Publishes staged nav state to [scope]. */ + fun publishTo(scope: IScope) + + /** Removes published nav state from [scope], keeping it staged so it can be republished. */ + fun unpublishFrom(scope: IScope) + + /** + * Called when the lease candidate is unregistered, after its nav state has been unpublished. + * + * The provided [scope] is the scope the candidate was unregistered against, or `null` if there + * was none (e.g., the SDK is disabled). + */ + fun onUnregistered(scope: IScope?) +} diff --git a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavScope.kt b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavScope.kt deleted file mode 100644 index f67c0b9b32d..00000000000 --- a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavScope.kt +++ /dev/null @@ -1,156 +0,0 @@ -package io.sentry.compose.navigation3.lifecycle - -import io.sentry.IScope -import io.sentry.ITransaction -import io.sentry.PropagationContext -import io.sentry.protocol.App -import io.sentry.protocol.Contexts - -/** - * An [INavScope] implementation that writes to and reads from [delegate] only when [owner] holds - * the nav lease; otherwise it falls back to [stagedNavData]. - */ -internal class NavScope( - private val delegate: IScope, - private val stagedNavData: StagedNavData, - private val coordinator: NavLeaseCoordinator, - private val owner: Any, -) : INavScope { - - private val hasLease: Boolean - get() = coordinator.hasLease(owner) - - override var screen: String? - get() = if (hasLease) delegate.screen else stagedNavData.screen - set(value) { - stagedNavData.stageScreen(value) - if (hasLease) { - stagedNavData.publishScreen(delegate) - } - } - - override fun setNavigationContext(value: Any?) { - stagedNavData.stageNavigationContext(value) - if (hasLease) { - stagedNavData.publishNavigationContext(delegate) - } - } - - /** - * Copies the whole of [IScope.getContexts] in order to return a copy of its [App]. - * - * [App]'s copy constructor isn't public, so an `App` can't be copied on its own. Going through - * [Contexts] deep-copies every context on the scope and discards all of it but the `App`. - * - * Called once per navigation that changes the top of the back stack, so the cost is bounded. - * Don't add call sites without reading [INavScope.app] first. - */ - override val app: App? - get() = Contexts(delegate.contexts).app - - /** - * The transaction bound to the scope. Without the lease, another owner's nav transaction isn't - * reported, so [owner]'s attempt to start a nav transaction reaches [NavScopes.startTransaction], - * which declines it and defers the propagation context rotation it implies. Non-nav transactions - * (e.g., the host's) are always reported, so they still block nav transactions as usual. - */ - override val transaction: ITransaction? - get() = - delegate.transaction?.takeUnless { current -> - !hasLease && coordinator.isNavTransaction(current) - } - - override fun bindTransaction(transaction: ITransaction) { - if (!hasLease) { - return - } - delegate.withTransaction { current -> - if (current == null) { - delegate.transaction = transaction - } - } - } - - override fun unbindTransaction(transaction: ITransaction) { - // Coordinator is called before clearTransaction() because the latter notifies the host app's - // scope observer(s), which could throw. Clearing the transaction first would cause the - // coordinator to retain a stale transaction. - coordinator.releaseTransaction(owner, transaction) - delegate.withTransaction { current -> - if (current === transaction) { - delegate.clearTransaction() - } - } - } - - override fun clearFinishedTransaction() { - if (!hasLease) { - return - } - delegate.withTransaction { current -> - if (current?.isFinished == true) { - delegate.clearTransaction() - } - } - } - - override fun rotatePropagationContext() { - if (hasLease) { - delegate.withPropagationContext { delegate.propagationContext = PropagationContext() } - } else { - stagedNavData.deferPropagationContextRotation() - } - } -} - -/** - * The subset of the [IScope] interface used to record navigation data. - * - * Protects against accidental misuse of what would otherwise be a partially-implemented `IScope` - * interface. New operations against the underlying `IScope` instance must be added here. Operations - * not directly delegable to `IScope` should ***not*** be added. - */ -internal interface INavScope { - - var screen: String? - - /** - * A copy of the scope's app context, if any. - * - * A copy rather than the live instance, because callers mutate the result before binding it to a - * transaction, and mutating the scope's own [App] would corrupt shared scope state. - * - * Implementations may have to copy more than the [App] to produce it, so treat this as expensive: - * read it once where it's needed rather than per frame or in a loop. See [NavScope.app]. - */ - val app: App? - - /** - * The transaction bound to the scope, if any, as far as starting a nav transaction is concerned: - * see [NavScope.transaction]. - */ - val transaction: ITransaction? - - /** Binds [transaction] to the scope if no transaction is bound to it. */ - fun bindTransaction(transaction: ITransaction) - - /** - * Unbinds [transaction] from the scope if it's the bound transaction, and ensures it won't be - * bound to the scope again. - */ - fun unbindTransaction(transaction: ITransaction) - - /** Unbinds the scope's transaction if it has finished. */ - fun clearFinishedTransaction() - - /** - * Sets or, given `null`, clears the value stored under [NAVIGATION_CONTEXT_KEY]. - * - * Implementations should clear via [IScope.setContexts] rather than [IScope.removeContexts], as - * the latter doesn't notify scope observers and therefore doesn't persist the removal to disk. - */ - fun setNavigationContext(value: Any?) - - /** Replaces the scope's propagation context with a new one. */ - fun rotatePropagationContext() -} diff --git a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopes.kt b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopes.kt index 92fdb80b52f..323656c8df9 100644 --- a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopes.kt +++ b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopes.kt @@ -1,23 +1,17 @@ package io.sentry.compose.navigation3.lifecycle import androidx.lifecycle.Lifecycle -import androidx.lifecycle.Lifecycle.Event.ON_DESTROY -import androidx.lifecycle.Lifecycle.Event.ON_PAUSE -import androidx.lifecycle.Lifecycle.Event.ON_RESUME -import androidx.lifecycle.Lifecycle.Event.ON_START -import androidx.lifecycle.Lifecycle.Event.ON_STOP -import androidx.lifecycle.LifecycleEventObserver -import androidx.lifecycle.LifecycleOwner import io.sentry.Breadcrumb import io.sentry.Hint import io.sentry.IScope import io.sentry.IScopes -import io.sentry.ITransaction -import io.sentry.NoOpTransaction +import io.sentry.PropagationContext import io.sentry.SentryLevel import io.sentry.SentryOptions import io.sentry.TransactionContext import io.sentry.TransactionOptions +import io.sentry.protocol.App +import io.sentry.protocol.Contexts /** The key under which nav context (i.e., the captured back stack) is stored in a scope. */ internal const val NAVIGATION_CONTEXT_KEY: String = "Navigation" @@ -28,14 +22,14 @@ internal const val NAVIGATION_CONTEXT_KEY: String = "Navigation" * `SentryNavEffect` exists.) * * Needed because all `SentryNavEffect`s mutate a single [IScope] instance by default and will - * clobber or interleave one another's updates unless managed properly: + * clobber or interleave one another's updates unless properly managed: * ``` * LifecycleOwner (e.g., Activity or Fragment) * ↓ owns * SentryNavEffect * ↓ * CoordinatedBackStackObserver (wrapper) - * ├── BackStackObserver — computes nav data and nav transactions + * ├── BackStackObserver — computes nav data and requests nav transactions * └── NavScopes — controls if nav data + txns reach the real scope * ↓ * NavLeaseCoordinator — arbitrates ownership across all observers @@ -43,151 +37,18 @@ internal const val NAVIGATION_CONTEXT_KEY: String = "Navigation" * shared IScope * ``` * - * **Limitations** - * - * Multi-effect support is restricted to the scenarios documented in the - * [Navigation for Android docs](https://docs.sentry.io/platforms/android/integrations/navigation/#supported-setups). - * - * This class implements the policy that underlies those scenarios. It assumes that, at any moment, - * the user is interacting with at most one registered [LifecycleOwner] in the host app, and that - * the `LifecycleOwner` being interacted with is the one that was most recently resumed. - * - * Concretely, a host app can safely use more than one `SentryNavEffect` only if: - * - * 1. each `SentryNavEffect` is called in the same composable as the `NavDisplay` it observes; - * 2. no `SentryNavEffect` is called from inside a `NavDisplay`, e.g., from a nav entry, scene, or - * entry decorator†; - * 3. each Activity or Fragment calls at most one `SentryNavEffect` from its own content (sibling - * Fragments may each have their own effect, but not alongside one in the Activity hosting them); - * and - * 4. the user can interact with only one of those Activities or Fragments at a time. - * - * † Meant to ensure each effect's `LocalLifecycleOwner` is an Activity or a Fragment and not a - * `NavDisplay`-provided owner. `NavDisplay` provides its own `LocalLifecycleOwner` to the entries - * and scenes it renders. That owner drops to STARTED state during transitions and while overlays - * are shown. An effect called inside a `NavDisplay` would therefore (wrongly) gain and lose the nav - * lease this class manages as navigation animates, rather than as its Activity or Fragment comes - * and goes. - * - * **A fundamental tradeoff** - * - * During `LifecycleOwner` overlap, the nav transaction and scoped nav data can reflect - * **_either_**: - * - * 1. the outgoing owner that's still visible, or - * 2. the incoming owner that may crash before it becomes visible, - * - * but not both. This class opts for (2) so that the incoming destination populates crash context - * and any work performed is associated with it† – the idea being that start-up logic is more likely - * to cause a crash or produce interesting spans than an outgoing destination's steady state. - * - * † Associating work with the incoming transaction means spans produced by the outgoing owner may - * appear as if they were produced by the incoming owner (or vice versa, were our policy reversed). - * A possible solution would be to implement a parent span delivery system similar to - * `LocalSentrySpan`, but that's a task for another day. - * - * **Only leaseholders publish nav state or start nav transactions** + * **Only leaseholders start nav transactions or publish nav data** * * This class works by only allowing `NavScopes` instances holding a lease to start nav transactions - * and publish their navigation data to the shared `IScope`. - * - * `NavScopes` instances without a lease write to their own copy of [stagedNavData], which lets them - * preserve their latest updates without overwriting another instance's published data. When an - * instance acquires the lease, it publishes its staged data to the shared `IScope`. - * - * **Lease handoff policy** + * and publish their navigation data to the shared `IScope`. (The lease policy is implemented by + * [NavLeaseCoordinator].) * - * The lease is acquired preemptively by an incoming owner's `NavScopes` instance when it applies - * its first [back stack update][onBackStackUpdate].† - * - * A displaced owner remains eligible for lease restoration until it [stops][ON_STOP]. It gets the - * lease back when it [resumes][ON_RESUME], or sooner if the new owner leaves or pauses while the - * displaced owner is still visible. If restored, the displaced owner's nav data and unfinished - * transaction are reinstated. - * - * † There are some exceptions, e.g., the leaseholder is resumed and the incoming owner isn't (e.g., - * a preloaded ViewPager2 Fragment page next to the current one). Such an owner acquires the lease - * when it resumes instead. - * - * The following diagram illustrates how handoff works: - * ``` - * Time - * │ - * ▼ - * - * SentryNavEffect A Shared state SentryNavEffect B - * ───────────────── ──────────── ───────────────── - * - * attach() Navigation lease: none - * │ - * Initial back stack update - * │ - * ├── acquire lease ────────► Navigation lease: A - * ├── create & bind txA ────► Scope transaction: txA - * ├── publish nav data ─────► Published data: A - * │ - * ON_RESUME A is eligible for restoration - * │ - * ON_PAUSE - * │ - * ├────────────────────────► Navigation lease: A (no other resumed owner) - * │ A remains eligible for restoration - * │ - * │ attach() - * │ │ - * │ Navigation lease: A ◄────────────────────┤ - * │ Scope transaction: txA │ - * │ Published data: A │ - * │ B registered, awaiting initial back │ - * │ stack update │ - * │ │ - * │ Initial back stack update - * │ │ - * │ Suspend & unbind txA ◄───────────────────┤ - * │ Navigation lease: B │ - * │ Published data: B │ - * │ Scope transaction: txB │ - * │ (rollback point) │ - * │ │ - * │ ON_RESUME - * │ │ - * │ B eligible for restoration ◄─────────────┤ - * │ │ - * ON_STOP │ - * │ │ - * ├── retain txA; timers continue │ - * ├────────────────────────► A no longer eligible for restoration │ - * │ │ - * ON_DESTROY │ - * │ │ - * ├── finish retained txA if unfinished │ - * └────────────────────────► A no longer tracked │ - * ON_PAUSE - * │ - * Navigation lease: B (no other resumed ◄─┤ - * owner) │ - * │ - * ON_STOP - * │ - * Navigation lease: B (no other resumed ◄─┤ - * owner); B keeps recording navigation │ - * B no longer eligible for restoration │ - * │ - * ON_DESTROY - * │ - * Finish txB if unfinished; ◄──────────────┤ - * unbind if still bound │ - * Clear B's remaining screen/back stack │ - * Retain recorded breadcrumbs │ - * Navigation lease: none │ - * ▼ - * - * Alternative at rollback point: B disposes before A stops - * Finish & unbind txB ◄────── dispose B ───┤ - * Navigation lease: A │ - * Restore txA if unfinished and scope free │ - * Republish A's staged data │ - * ``` + * `NavScopes` instances without a lease write to their own copy of [stagedNavData], and keep their + * unfinished nav transaction in [stagedNavTransaction], which lets them preserve their latest + * updates without overwriting another instance's transaction or published data. When an instance + * acquires the lease, it writes its staged data to the shared `IScope` and binds its unfinished + * transaction again (subject to any publishing conditions, e.g., the transaction stays unbound if + * another transaction is already bound to the shared `IScope`). * * **Usage** * @@ -210,166 +71,251 @@ internal const val NAVIGATION_CONTEXT_KEY: String = "Navigation" internal class NavScopes( private val delegate: IScopes, private val coordinator: NavLeaseCoordinator, -) : INavScopes, LifecycleEventObserver { +) : INavScopes { override val options: SentryOptions get() = delegate.options - private val owner: Any = Any() private var attachedLifecycle: Lifecycle? = null + + /** This instance's registration with the [coordinator], while it's attached. */ + private var registration: NavLeaseRegistration? = null + + private val stagedNavTransaction = StagedNavTransaction(delegate) private val stagedNavData = StagedNavData() + private val navStatePublisher = NavStatePublisher() + /** * Registers this [NavScopes] instance with [lifecycle]. No-ops if `lifecycle` has already been * attached or is [destroyed][Lifecycle.Event.ON_DESTROY]. * - * Attaching to a second, different lifecycle is unsupported; it's logged and ignored. + * Attaching to a second, different lifecycle no-ops. */ fun attach(lifecycle: Lifecycle) { val currentLifecycle = attachedLifecycle - if (currentLifecycle != null && currentLifecycle !== lifecycle) { - delegate.options.logger.log( - SentryLevel.WARNING, - "NavScopes is already attached to a different lifecycle; ignoring the new lifecycle.", - ) - return - } - if (currentLifecycle === lifecycle) { + if (currentLifecycle != null) { + if (currentLifecycle !== lifecycle) { + delegate.options.logger.log( + SentryLevel.WARNING, + "NavScopes is already attached to a different lifecycle; ignoring the new lifecycle.", + ) + } + + // No-op if already attached to a lifecycle instance (whether the same or a different one). return } + if (lifecycle.currentState == Lifecycle.State.DESTROYED) { return } attachedLifecycle = lifecycle - - coordinator.registerCandidate( - owner = owner, - lifecycleState = lifecycle.currentState, - onPublishData = stagedNavData::publishTo, - onClearPublishedData = stagedNavData::clearPublishedStateFrom, - ) - - lifecycle.addObserver(this) - } - - override fun onStateChanged(source: LifecycleOwner, event: Lifecycle.Event) { - when (event) { - ON_START -> coordinator.startCandidate(owner) - ON_RESUME -> withScopeOrNull { coordinator.resumeCandidate(owner, it) } - ON_PAUSE -> withScopeOrNull { coordinator.pauseCandidate(owner, it) } - ON_STOP -> withScopeOrNull { coordinator.stopCandidate(owner, it) } - ON_DESTROY -> dispose() - else -> Unit - } + registration = coordinator.register(lifecycle, delegate, navStatePublisher) } /** * Announces that a back stack update is about to be applied, so this instance can acquire the nav - * lease if it's eligible to. + * lease if it's eligible. * - * Must be called before each update. If [configureScope][IScopes.configureScope] skips its - * callback (e.g., the SDK is disabled), the update itself can't be applied either, so nothing is - * recorded and the next update applied is treated as this instance's first. + * Must be called before each update. */ fun onBackStackUpdate() { - delegate.configureScope { scope -> coordinator.onBackStackUpdate(owner, scope) } + registration?.onBackStackUpdate() } override fun configureScope(callback: (INavScope) -> Unit) { - delegate.configureScope { scope -> - callback(NavScope(scope, stagedNavData, coordinator, owner)) - } - } - - /** - * Starts a nav transaction, provided this instance holds the nav lease. - * - * Without the lease, returns a no-op transaction and defers the propagation context rotation a - * new transaction would have implied until the lease is acquired. A transaction started without - * the lease couldn't be bound to the scope, so its destination's spans would attach to the - * leaseholder's bound transaction instead, and the empty transaction would be dropped. - * - * The [coordinator] retains the leaseholder's transaction, so it can be restored if this instance - * loses the lease and regains it while the transaction is still unfinished. - */ - override fun startTransaction( - transactionContext: TransactionContext, - transactionOptions: TransactionOptions, - ): ITransaction { - if (!coordinator.hasLease(owner)) { - stagedNavData.deferPropagationContextRotation() - return NoOpTransaction.getInstance() - } - - return delegate.startTransaction(transactionContext, transactionOptions).also { transaction -> - if (!transaction.isNoOp) { - coordinator.retainTransaction(owner, transaction) - } - } + delegate.configureScope { scope -> callback(NavScope(scope)) } } - /** Records [breadcrumb] immediately; breadcrumbs only append, so they aren't lease-protected. */ override fun addBreadcrumb(breadcrumb: Breadcrumb, hint: Hint) { delegate.addBreadcrumb(breadcrumb, hint) } /** - * Releases the lease, clears remaining published state, and finishes this instance's transaction. - * - * Also stops observing the attached [LifecycleOwner] and cleans up memory references. + * Releases the lease (if held), unpublishes this instance's nav state, and finishes its nav + * transaction. */ fun dispose() { - attachedLifecycle?.removeObserver(this) + registration?.dispose() + stagedNavTransaction.forget() attachedLifecycle = null + registration = null + } - // Clear nav state, unbind the nav transaction, and avoid leaking the LifecycleOwner. - withScopeOrNull { coordinator.unregisterCandidate(owner, it) } + /** + * Publishes this instance's [stagedNavTransaction] and [stagedNavData] to the shared [IScope] + * while it holds the nav lease, as directed by the [coordinator]. + */ + private inner class NavStatePublisher : LeasedNavState { + + override fun publishTo(scope: IScope) { + stagedNavData.publishTo(scope) + stagedNavTransaction.publishTo(scope) + } + + override fun unpublishFrom(scope: IScope) { + stagedNavTransaction.unpublishFrom(scope) + stagedNavData.clearPublishedStateFrom(scope) + } + + override fun onUnregistered(scope: IScope?) { + // Unregistering on destroy detaches this instance, as dispose() does. + attachedLifecycle = null + registration = null + + if (scope != null) { + stagedNavTransaction.finish(scope) + } else { + // Without a scope (e.g., the SDK is disabled), the transaction can't be sent and will time + // out anyway. Not finishing it immediately lets us ensure the error-handling policy from + // IScopes.configureScope covers all SDK work. + stagedNavTransaction.forget() + } + } } /** - * Runs [block] with the real scope, or with `null` if [configureScope][IScopes.configureScope] - * skips its callback (e.g., the SDK is disabled), so coordinator bookkeeping still happens. - * - * A callback that ran but threw is not retried, as retrying could let its exception escape into - * the host app. + * An [INavScope] that maintains a [stagedNavTransaction] and [stagedNavData], and only writes to + * or reads from the [delegate IScope][IScope] when this instance holds the nav lease. */ - private fun withScopeOrNull(block: (IScope?) -> Unit) { - var callbackInvoked = false - delegate.configureScope { scope -> - callbackInvoked = true - block(scope) + private inner class NavScope(private val delegate: IScope) : INavScope { + + override var screen: String? + get() = if (hasLease) delegate.screen else stagedNavData.screen + set(value) { + stagedNavData.stageScreen(value) + if (hasLease) { + stagedNavData.publishScreen(delegate) + } + } + + /** + * Copies the whole of [IScope.getContexts] in order to return a copy of its [App]. + * + * [App]'s copy constructor isn't public, so an `App` can't be copied on its own. Going through + * [Contexts] deep-copies every context on the scope and discards all of it but the `App`. + */ + override val app: App? + get() = Contexts(delegate.contexts).app + + private val hasLease: Boolean + get() = registration?.hasLease == true + + override fun setNavigationContext(value: Any?) { + stagedNavData.stageNavigationContext(value) + if (hasLease) { + stagedNavData.publishNavigationContext(delegate) + } + } + + override fun rotatePropagationContext() { + if (hasLease) { + delegate.withPropagationContext { delegate.propagationContext = PropagationContext() } + } else { + stagedNavData.deferPropagationContextRotation() + } + } + + /** + * Starts and binds a nav transaction with the nav lease (see [StagedNavTransaction]). Callers + * must [finish][finishNavTransaction] the previous nav transaction first. + * + * Without the lease, starts none and defers the propagation context rotation a new transaction + * would have implied until the lease is acquired. A transaction started without the lease + * couldn't be bound to the scope, so its destination's spans would attach to the leaseholder's + * bound transaction instead, and the empty transaction would be dropped. + */ + override fun startNavTransaction( + transactionContext: TransactionContext, + transactionOptions: TransactionOptions, + ): INavTransaction? { + return if (hasLease) { + stagedNavTransaction.start(delegate, transactionContext, transactionOptions) + } else { + stagedNavData.deferPropagationContextRotation() + null + } } - if (!callbackInvoked) { - block(null) + override fun finishNavTransaction() { + stagedNavTransaction.finish(delegate) } } } /** - * The subset of the [IScopes] interface used to record navigation data. + * The subset of the [IScopes] interface used to record nav state. * * Protects against accidental misuse of what would otherwise be a partially-implemented `IScopes` * interface. New operations against the underlying `IScopes` instance must be added here. * Operations not directly delegable to `IScopes` should ***not*** be added. */ -internal interface INavScopes { +internal sealed interface INavScopes { val options: SentryOptions - /** Runs [callback] with the current [INavScope], if the SDK provides a scope. */ + /** + * Runs [callback] with the current [INavScope]. + * + * No-ops if the SDK doesn't provide a scope (e.g., the SDK is disabled). + */ fun configureScope(callback: (INavScope) -> Unit) + fun addBreadcrumb(breadcrumb: Breadcrumb, hint: Hint) +} + +/** + * The scope operations used to record nav data and nav transactions. + * + * Protects against accidental misuse of what would otherwise be a partially-implemented `IScope` + * interface. New operations against the underlying `IScope` instance must be added here. Operations + * not directly delegable to `IScope` should ***not*** be added. The exception is the nav + * transaction lifecycle ([startNavTransaction] and [finishNavTransaction]), which depends on the + * nav lease and so has no `IScope` equivalent. + */ +internal sealed interface INavScope { + + var screen: String? + + /** + * A copy of the scope's app context, if any. + * + * A copy rather than the live instance, because callers mutate the result before binding it to a + * transaction, and mutating the scope's own [App] would corrupt shared scope state. + * + * Implementations may have to copy more than the [App] to produce it, so treat this as expensive: + * read it once where it's needed rather than per frame or in a loop. + */ + val app: App? + + /** + * Sets or, given `null`, clears the value stored under [NAVIGATION_CONTEXT_KEY]. + * + * Implementations should clear via [IScope.setContexts] rather than [IScope.removeContexts], as + * the latter doesn't notify scope observers and therefore doesn't persist the removal to disk. + */ + fun setNavigationContext(value: Any?) + + /** Replaces the scope's propagation context with a new one. */ + fun rotatePropagationContext() + /** - * Starts a transaction, or returns a no-op transaction if none can be started (e.g., tracing is - * disabled, or the implementation declines to start one; see [NavScopes.startTransaction]). + * Starts a nav transaction and binds it to the scope, unless another unfinished transaction is + * already bound to it or the nav lease isn't held. Returns `null` if no transaction was started. + * + * (Note: Callers must first [finish][finishNavTransaction] the nav transaction most recently + * started, if any. Otherwise, it blocks the new one while it's bound.) */ - fun startTransaction( + fun startNavTransaction( transactionContext: TransactionContext, transactionOptions: TransactionOptions, - ): ITransaction + ): INavTransaction? - fun addBreadcrumb(breadcrumb: Breadcrumb, hint: Hint) + /** + * Finishes the nav transaction most recently started via [startNavTransaction] and unbinds it + * from the shared [IScope]. + */ + fun finishNavTransaction() } diff --git a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavTransaction.kt b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavTransaction.kt new file mode 100644 index 00000000000..cfbfd05f8fb --- /dev/null +++ b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavTransaction.kt @@ -0,0 +1,71 @@ +package io.sentry.compose.navigation3.lifecycle + +import io.sentry.ITransaction +import io.sentry.SpanStatus +import io.sentry.protocol.App + +/** + * An [INavTransaction] handle backed by a started [ITransaction]. + * + * **Thread safety** + * + * This class is ***not*** thread-safe. Apply the thread safety policy from [NavScopes]. + */ +internal class NavTransaction +private constructor( + /** The backing transaction. */ + val transaction: ITransaction +) : INavTransaction { + + val name: String + get() = transaction.name + + override fun setData(key: String, value: Any) { + transaction.setData(key, value) + } + + override fun setContext(key: String, value: Any) { + transaction.setContext(key, value) + } + + override fun setApp(app: App) { + transaction.contexts.setApp(app) + } + + /** + * Finishes the backing transaction with its own status, or [OK][SpanStatus.OK] if it has none, + * and then passes it to [onFinished]. + */ + fun finish(onFinished: (ITransaction) -> Unit) { + if (!transaction.isFinished) { + transaction.finish(transaction.status ?: SpanStatus.OK) + } + onFinished(transaction) + } + + companion object { + /** A handle backed by the already-started [transaction]. */ + fun started(transaction: ITransaction): NavTransaction = NavTransaction(transaction) + } +} + +/** + * The subset of the [ITransaction] interface used to record nav data on a nav transaction. + * + * Protects against accidental misuse of what would otherwise be a partially-implemented + * `ITransaction` interface. New operations against the underlying `ITransaction` instance must be + * added here. Operations not directly delegable to `ITransaction` should ***not*** be added. + * + * Starting and finishing a nav transaction are lease-aware, so they go through [INavScope] instead. + */ +internal sealed interface INavTransaction { + + /** See [ITransaction.setData]. */ + fun setData(key: String, value: Any) + + /** See [ITransaction.setContext]. */ + fun setContext(key: String, value: Any) + + /** Sets the transaction's app context. See [ITransaction.getContexts]. */ + fun setApp(app: App) +} diff --git a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/README.md b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/README.md index 7d697812ecf..e003af99061 100644 --- a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/README.md +++ b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/README.md @@ -3,10 +3,32 @@ Package contains code for coordinating multiple `SentryNavEffect`s and their access to a shared `IScope`, so that overlapping effects (e.g., during Activity or Fragment transitions) don't clobber one another's screen names, nav context, or nav transactions. -See the `NavScopes` KDoc for the lease policy that decides which effect may write to the scope. +See the `NavLeaseCoordinator` KDoc for the lease policy that decides which effect may write to the scope. ## Encapsulation policy This package is meant to encapsulate knowledge about the possibility of multiple `SentryNavEffect`s and the shared mutable state issues it raises. Addressing those concerns is complicated. Confining them here makes them easier to reason about and easier to swap for alternative logic should we someday need to. + +## Limitations + +Support for multiple `SentryNavEffect`s is restricted to the scenarios documented in the +[Navigation for Android docs](https://docs.sentry.io/platforms/android/integrations/navigation/#supported-setups). + +`NavLeaseCoordinator` implements the policy that underlies those scenarios. It assumes that, at any moment, the user is interacting with at +most one registered `LifecycleOwner` in the host app, and that the `LifecycleOwner` being interacted with is the one that was most recently +resumed. + +Concretely, a host app can safely use more than one `SentryNavEffect` only if: + +1. each `SentryNavEffect` is called in the same composable as the `NavDisplay` it observes; +2. no `SentryNavEffect` is called from inside a `NavDisplay`, e.g., from a nav entry, scene, or entry decorator†; +3. each Activity or Fragment calls at most one `SentryNavEffect` from its own content (sibling Fragments may each have their own effect, but + not alongside one in the Activity hosting them); and +4. the user can interact with only one of those Activities or Fragments at a time. + +† Meant to ensure each effect's `LocalLifecycleOwner` is an Activity or a Fragment and not a `NavDisplay`-provided owner. `NavDisplay` +provides its own `LocalLifecycleOwner` to the entries and scenes it renders. That owner drops to STARTED state during transitions and while +overlays are shown. An effect called inside a `NavDisplay` would therefore (wrongly) gain and lose the nav lease `NavLeaseCoordinator` +manages as navigation animates, rather than as its Activity or Fragment comes and goes. diff --git a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavData.kt b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavData.kt index a32cc74d506..affa1caff61 100644 --- a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavData.kt +++ b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavData.kt @@ -4,17 +4,22 @@ import io.sentry.IScope import io.sentry.PropagationContext /** - * Holds navigation state staged for publication. + * Holds nav data (i.e., the screen name and nav context) staged for publication to a shared + * [IScope]. * - * Only state that occupies a single slot in the shared [IScope] is staged. Nav events (i.e., - * breadcrumbs) only append to the scope, so they can't overwrite another instance's data and are - * recorded immediately, irrespective of lease ownership. + * Only state that occupies a single slot in the shared `IScope` is staged. Nav events (i.e., + * breadcrumbs) are excluded as they only append to the scope and are recorded immediately, + * irrespective of lease ownership. * * Propagation context isn't staged either, since an earlier propagation context mustn't overwrite * the scope's current one. Instead, a rotation requested without the lease, or implied by a nav * transaction that couldn't be started without it, is [deferred][deferPropagationContextRotation] * and performed once when the lease is acquired. As a result, any destination reached in the * meantime still gets a fresh trace. + * + * **Thread safety** + * + * This class is ***not*** thread-safe. Apply the thread safety policy from [NavScopes]. */ internal class StagedNavData { @@ -57,8 +62,7 @@ internal class StagedNavData { * rotation. * * Only staged values are published, so values the host app or other integrations set (e.g., a - * screen name while screen tracking is off) survive. Values published by the previous leaseholder - * are cleared separately, via its [clearPublishedStateFrom]. + * screen name while screen tracking is off) survive. */ fun publishTo(realScope: IScope) { if (hasScreenUpdate) { diff --git a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavTransaction.kt b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavTransaction.kt new file mode 100644 index 00000000000..793aad39142 --- /dev/null +++ b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavTransaction.kt @@ -0,0 +1,143 @@ +package io.sentry.compose.navigation3.lifecycle + +import io.sentry.IScope +import io.sentry.IScopes +import io.sentry.ITransaction +import io.sentry.SentryLevel +import io.sentry.TransactionContext +import io.sentry.TransactionOptions + +/** + * Holds a nav transaction staged for publication to a shared [IScope]. + * + * A started transaction that's [unpublished][unpublishFrom] is unbound but keeps running, so that + * it can be bound again if it's republished while unfinished. Once [finished][finish], a + * transaction is forgotten and never bound again. + * + * Starting or publishing a nav transaction never displaces an unfinished transaction bound to the + * scope by someone else (e.g., the host app). + * + * **Thread safety** + * + * This class is ***not*** thread-safe. Apply the thread safety policy from [NavScopes]. + */ +internal class StagedNavTransaction(private val scopes: IScopes) { + + /** The nav transaction most recently started, unless it has been finished since then. */ + private var navTransaction: NavTransaction? = null + + /** + * Starts a nav transaction from [context] and [options] and binds it to [scope], unless another + * unfinished transaction is already bound to `scope`, in which case none is started, as it + * couldn't be bound. Returns `null` if no transaction was started (including when [scopes] + * returned a no-op transaction, e.g., because the SDK is disabled or ignores the transaction's + * origin), in which case the instance's current nav transaction, if any, is kept. + * + * **Warning**: [Finish][finish] the current nav transaction before calling this method, as this + * method replaces the current transaction without finishing it. + */ + fun start( + scope: IScope, + context: TransactionContext, + options: TransactionOptions, + ): NavTransaction? { + if (!scope.isFreeFor(context.name, transaction = null)) { + return null + } + val transaction = + scopes.startTransaction(context, options).takeUnless { it.isNoOp } ?: return null + + // Record the transaction before binding it: binding notifies the host app's scope observers, + // which can throw, and an unrecorded transaction might never be finished. + val started = NavTransaction.started(transaction) + navTransaction = started + + bind(scope, transaction) + return started + } + + /** + * Binds the nav transaction to [scope]. A transaction that has finished in the meantime is + * forgotten and unbound instead. + * + * If another unfinished transaction is bound to [scope], the transaction stays unbound, so it can + * be bound the next time it's published. + */ + fun publishTo(scope: IScope) { + val navTransaction = navTransaction ?: return + val started = navTransaction.transaction + if (started.isFinished) { + this.navTransaction = null + unbind(scope, started) + } else if (scope.isFreeFor(navTransaction.name, started)) { + bind(scope, started) + } + } + + /** + * Unbinds the nav transaction from [scope], if it's bound, without finishing it, so it can be + * [published][publishTo] again. + */ + fun unpublishFrom(scope: IScope) { + navTransaction?.let { unbind(scope, it.transaction) } + } + + /** Finishes the nav transaction and unbinds it from [scope], if it's bound. */ + fun finish(scope: IScope) { + val navTransaction = navTransaction ?: return + // Forget the transaction before unbinding it: unbinding notifies the host app's scope + // observers, which can throw, and a transaction left behind would be bound again later. + this.navTransaction = null + navTransaction.finish(onFinished = { unbind(scope, it) }) + } + + /** + * Forgets the nav transaction without finishing it, e.g., when there's no scope to unbind it + * from. It will be left to time out. + */ + fun forget() { + navTransaction = null + } + + /** + * Reports whether the receiver's transaction slot can hold [transaction], or a new transaction + * given `null` (see [canHold]). + */ + private fun IScope.isFreeFor(name: String, transaction: ITransaction?): Boolean { + if (canHold(current = this.transaction, transaction)) { + return true + } + + scopes.options.logger.log( + SentryLevel.DEBUG, + "Nav3 transaction for route %s is blocked by another active transaction.", + name, + ) + return false + } + + /** Binds [transaction] to [scope] if the scope's transaction slot can hold it (see [canHold]). */ + private fun bind(scope: IScope, transaction: ITransaction) { + scope.withTransaction { current -> + if (current !== transaction && canHold(current, transaction)) { + scope.transaction = transaction + } + } + } + + /** + * Whether a transaction slot holding [current] can hold [transaction], or a new transaction given + * `null`: it's empty, holds a finished transaction, or already holds `transaction`. + */ + private fun canHold(current: ITransaction?, transaction: ITransaction?): Boolean = + current == null || current.isFinished || current === transaction + + /** Unbinds [transaction] from [scope] if it's the bound transaction. */ + private fun unbind(scope: IScope, transaction: ITransaction) { + scope.withTransaction { current -> + if (current === transaction) { + scope.clearTransaction() + } + } + } +} diff --git a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/CoordinatedBackStackObserverTest.kt b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/CoordinatedBackStackObserverTest.kt index 08c2003fbbf..c742d3dd5b2 100644 --- a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/CoordinatedBackStackObserverTest.kt +++ b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/CoordinatedBackStackObserverTest.kt @@ -4,6 +4,7 @@ import androidx.lifecycle.Lifecycle import androidx.lifecycle.LifecycleOwner import androidx.lifecycle.LifecycleRegistry import com.google.common.truth.Truth.assertThat +import io.sentry.IScope import io.sentry.IScopes import io.sentry.Scope import io.sentry.ScopeCallback @@ -11,6 +12,7 @@ import io.sentry.SentryOptions import io.sentry.SentryTracer import io.sentry.TransactionContext import io.sentry.TransactionOptions +import io.sentry.compose.navigation3.lifecycle.LeasedNavState import io.sentry.compose.navigation3.lifecycle.NAVIGATION_CONTEXT_KEY import io.sentry.compose.navigation3.lifecycle.NavLeaseCoordinator import org.junit.Test @@ -145,11 +147,11 @@ class CoordinatedBackStackObserverTest { /** Registers another owner that takes the nav lease and then pauses, keeping the lease. */ fun givenPausedLeaseholder() { - val other = Any() - coordinator.registerCandidate(other, Lifecycle.State.RESUMED, {}, {}) - coordinator.onBackStackUpdate(other, scope) - coordinator.pauseCandidate(other, scope) - check(coordinator.hasLease(other)) + val otherLifecycle = resumedLifecycle() + val other = checkNotNull(coordinator.register(otherLifecycle, scopes, NoOpLeasedNavState)) + other.onBackStackUpdate() + otherLifecycle.currentState = Lifecycle.State.STARTED + check(other.hasLease) } fun getSut(lifecycle: Lifecycle = resumedLifecycle()): CoordinatedBackStackObserver = @@ -164,11 +166,27 @@ class CoordinatedBackStackObserverTest { ) .apply { attach(lifecycle) } + /** + * Owners of the lifecycles created by [resumedLifecycle]. `LifecycleRegistry` holds its owner + * weakly and throws once it's collected, so the fixture keeps them alive for the test. + */ + private val lifecycleOwners = mutableListOf() + fun resumedLifecycle(): LifecycleRegistry = object : LifecycleOwner { override val lifecycle = LifecycleRegistry.createUnsafe(this).apply { currentState = Lifecycle.State.RESUMED } } + .also(lifecycleOwners::add) .lifecycle } + + /** Nav state for a competing owner that has nothing to publish. */ + private object NoOpLeasedNavState : LeasedNavState { + override fun publishTo(scope: IScope) = Unit + + override fun unpublishFrom(scope: IScope) = Unit + + override fun onUnregistered(scope: IScope?) = Unit + } } diff --git a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinatorTest.kt b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinatorTest.kt index 44986848b5b..9a521312f68 100644 --- a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinatorTest.kt +++ b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinatorTest.kt @@ -1,868 +1,931 @@ package io.sentry.compose.navigation3.lifecycle import androidx.lifecycle.Lifecycle +import androidx.lifecycle.Lifecycle.State.CREATED +import androidx.lifecycle.Lifecycle.State.DESTROYED +import androidx.lifecycle.Lifecycle.State.RESUMED +import androidx.lifecycle.Lifecycle.State.STARTED +import androidx.lifecycle.LifecycleOwner +import androidx.lifecycle.LifecycleRegistry import com.google.common.truth.Truth.assertThat import io.sentry.IScope -import io.sentry.ITransaction +import io.sentry.IScopes import io.sentry.Scope +import io.sentry.ScopeCallback import io.sentry.SentryOptions -import io.sentry.SpanStatus import org.junit.Assert.assertThrows import org.junit.Test import org.mockito.kotlin.any -import org.mockito.kotlin.doThrow +import org.mockito.kotlin.doAnswer import org.mockito.kotlin.mock -import org.mockito.kotlin.never -import org.mockito.kotlin.verify -import org.mockito.kotlin.verifyNoInteractions import org.mockito.kotlin.whenever @Suppress("LargeClass") class NavLeaseCoordinatorTest { - private val owner = Any() - private val incoming = Any() - private val incumbent = Any() - private val previous = Any() - - private val transaction = mock() - private val unrelatedTransaction = mock() + // region registration @Test - fun `a registered created owner acquires the nav lease on its first back stack update`() { + fun `register() - returns no registration for a destroyed lifecycle and doesn't observe it`() { val fixture = Fixture.create() val navLeaseCoordinator = fixture.navLeaseCoordinator - fixture.register(owner, Lifecycle.State.CREATED) + val owner = TestOwner(DESTROYED) - navLeaseCoordinator.onBackStackUpdate(owner, fixture.scope) + val registration = + navLeaseCoordinator.register(owner.lifecycle, fixture.scopes, fixture.navState("owner")) - assertThat(navLeaseCoordinator.hasLease(owner)).isTrue() - assertThat(fixture.published).containsExactly(owner) + assertThat(registration).isNull() + assertThat(owner.lifecycle.observerCount).isEqualTo(0) } @Test - fun `registering the same owner twice preserves its first publication callback`() { + fun `separate registrations for the same lifecycle don't share the nav lease`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - fixture.register(owner, Lifecycle.State.CREATED) + val owner = TestOwner(RESUMED) + val first = fixture.acquireLease("first", owner) - navLeaseCoordinator.registerCandidate( - owner, - Lifecycle.State.RESUMED, - { fixture.published += "duplicate" }, - { fixture.cleared += "duplicate" }, - ) - navLeaseCoordinator.onBackStackUpdate(owner, fixture.scope) + val second = fixture.acquireLease("second", owner) - assertThat(fixture.published).containsExactly(owner) + assertThat(first.hasLease).isFalse() + assertThat(second.hasLease).isTrue() } - @Test - fun `registering a destroyed owner does not make it eligible for the nav lease`() { - val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - - fixture.register(owner, Lifecycle.State.DESTROYED) - navLeaseCoordinator.onBackStackUpdate(owner, fixture.scope) + // endregion - assertThat(navLeaseCoordinator.hasLease(owner)).isFalse() - assertThat(fixture.published).isEmpty() - } + // region acquiring the nav lease on the first back stack update @Test - fun `equal but distinct owners do not share the nav lease`() { + fun `a created owner acquires the nav lease on its first back stack update`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - val registeredOwner = String(charArrayOf('a')) - val equalOwner = String(charArrayOf('a')) - fixture.acquireLease(registeredOwner) + val registration = fixture.register("owner", TestOwner(CREATED)) - val result = navLeaseCoordinator.hasLease(equalOwner) + registration.onBackStackUpdate() - assertThat(result).isFalse() - assertThat(navLeaseCoordinator.hasLease(registeredOwner)).isTrue() + assertThat(registration.hasLease).isTrue() + assertThat(fixture.published).containsExactly("owner") } @Test fun `starting a created owner leaves it eligible to acquire the nav lease on its first back stack update`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - fixture.register(owner, Lifecycle.State.CREATED) + val owner = TestOwner(CREATED) + val registration = fixture.register("owner", owner) - navLeaseCoordinator.startCandidate(owner) - navLeaseCoordinator.onBackStackUpdate(owner, fixture.scope) + owner.lifecycle.currentState = STARTED + registration.onBackStackUpdate() - assertThat(navLeaseCoordinator.hasLease(owner)).isTrue() + assertThat(registration.hasLease).isTrue() } @Test fun `restarting a stopped owner does not take the nav lease on its first back stack update`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - fixture.register(owner, Lifecycle.State.CREATED) - navLeaseCoordinator.stopCandidate(owner, fixture.scope) - fixture.acquireLease(incumbent) + val owner = TestOwner(STARTED) + val registration = fixture.register("owner", owner) + owner.lifecycle.currentState = CREATED + val incumbentOwner = TestOwner(RESUMED) + val incumbent = fixture.acquireLease("incumbent", incumbentOwner) + // The incumbent keeps the lease, but no longer blocks a first update from taking it. + incumbentOwner.lifecycle.currentState = STARTED - navLeaseCoordinator.startCandidate(owner) - navLeaseCoordinator.onBackStackUpdate(owner, fixture.scope) + owner.lifecycle.currentState = STARTED + registration.onBackStackUpdate() - assertThat(navLeaseCoordinator.hasLease(incumbent)).isTrue() - assertThat(fixture.published).containsExactly(incumbent) + assertThat(incumbent.hasLease).isTrue() + assertThat(fixture.published).containsExactly("incumbent") } @Test fun `resuming an owner before its first back stack update leaves the nav lease unassigned`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - fixture.register(owner, Lifecycle.State.CREATED) + val owner = TestOwner(CREATED) + val registration = fixture.register("owner", owner) - navLeaseCoordinator.resumeCandidate(owner, fixture.scope) + owner.lifecycle.currentState = RESUMED - assertThat(navLeaseCoordinator.hasLease(owner)).isFalse() + assertThat(registration.hasLease).isFalse() assertThat(fixture.published).isEmpty() } @Test fun `resuming a stopped owner after its first back stack update transfers the nav lease`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - fixture.acquireLease(incumbent) - fixture.register(owner, Lifecycle.State.CREATED) - navLeaseCoordinator.stopCandidate(owner, fixture.scope) - navLeaseCoordinator.onBackStackUpdate(owner, fixture.scope) + fixture.acquireLease("incumbent", TestOwner(RESUMED)) + val owner = TestOwner(STARTED) + val registration = fixture.register("owner", owner) + owner.lifecycle.currentState = CREATED + registration.onBackStackUpdate() - navLeaseCoordinator.resumeCandidate(owner, fixture.scope) + owner.lifecycle.currentState = RESUMED - assertThat(navLeaseCoordinator.hasLease(owner)).isTrue() - assertThat(fixture.published).containsExactly(incumbent, owner).inOrder() + assertThat(registration.hasLease).isTrue() + assertThat(fixture.published).containsExactly("incumbent", "owner").inOrder() } @Test fun `a paused owner cannot acquire the nav lease on its first back stack update`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - fixture.register(owner, Lifecycle.State.RESUMED) - navLeaseCoordinator.pauseCandidate(owner, fixture.scope) + val owner = TestOwner(RESUMED) + val registration = fixture.register("owner", owner) + owner.lifecycle.currentState = STARTED - navLeaseCoordinator.onBackStackUpdate(owner, fixture.scope) + registration.onBackStackUpdate() - assertThat(navLeaseCoordinator.hasLease(owner)).isFalse() + assertThat(registration.hasLease).isFalse() assertThat(fixture.published).isEmpty() } @Test fun `an owner that isn't resumed doesn't take the nav lease from a resumed leaseholder`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - fixture.acquireLease(incumbent) + val incumbent = fixture.acquireLease("incumbent", TestOwner(RESUMED)) - fixture.acquireLease(owner, state = Lifecycle.State.STARTED) + fixture.acquireLease("owner", TestOwner(STARTED)) - assertThat(navLeaseCoordinator.hasLease(incumbent)).isTrue() - assertThat(fixture.published).containsExactly(incumbent) + assertThat(incumbent.hasLease).isTrue() + assertThat(fixture.published).containsExactly("incumbent") } @Test fun `an owner kept from the nav lease by a resumed leaseholder acquires it when it resumes`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - fixture.acquireLease(incumbent) - fixture.acquireLease(owner, state = Lifecycle.State.STARTED) + fixture.acquireLease("incumbent", TestOwner(RESUMED)) + val owner = TestOwner(STARTED) + val registration = fixture.acquireLease("owner", owner) + + owner.lifecycle.currentState = RESUMED + + assertThat(registration.hasLease).isTrue() + assertThat(fixture.published).containsExactly("incumbent", "owner").inOrder() + } + + /** + * `NavScopes` relies on this: it starts a nav transaction deferred without the lease when the + * lease is acquired, which must not coincide with a back stack update that starts a transaction + * of its own. + */ + @Test + fun `an owner kept from the nav lease on its first back stack update doesn't take it on a later one`() { + val fixture = Fixture.create() + val incumbentOwner = TestOwner(RESUMED) + val incumbent = fixture.acquireLease("incumbent", incumbentOwner) + val registration = fixture.acquireLease("owner", TestOwner(STARTED)) + // The incumbent keeps the lease, but no longer blocks a first update from taking it. + incumbentOwner.lifecycle.currentState = STARTED - navLeaseCoordinator.resumeCandidate(owner, fixture.scope) + registration.onBackStackUpdate() - assertThat(navLeaseCoordinator.hasLease(owner)).isTrue() - assertThat(fixture.published).containsExactly(incumbent, owner).inOrder() + assertThat(incumbent.hasLease).isTrue() + assertThat(fixture.published).containsExactly("incumbent") } @Test fun `an owner that isn't resumed takes the nav lease from a paused leaseholder`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - fixture.acquireLease(incumbent) - navLeaseCoordinator.pauseCandidate(incumbent, fixture.scope) + val incumbentOwner = TestOwner(RESUMED) + fixture.acquireLease("incumbent", incumbentOwner) + incumbentOwner.lifecycle.currentState = STARTED - fixture.acquireLease(owner, state = Lifecycle.State.CREATED) + val registration = fixture.acquireLease("owner", TestOwner(CREATED)) - assertThat(navLeaseCoordinator.hasLease(owner)).isTrue() + assertThat(registration.hasLease).isTrue() } @Test fun `an owner that isn't resumed takes the nav lease from a stopped leaseholder`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - fixture.acquireLease(incumbent) - navLeaseCoordinator.pauseCandidate(incumbent, fixture.scope) - navLeaseCoordinator.stopCandidate(incumbent, fixture.scope) + val incumbentOwner = TestOwner(RESUMED) + fixture.acquireLease("incumbent", incumbentOwner) + incumbentOwner.lifecycle.currentState = CREATED - fixture.acquireLease(owner, state = Lifecycle.State.CREATED) + val registration = fixture.acquireLease("owner", TestOwner(CREATED)) - assertThat(navLeaseCoordinator.hasLease(owner)).isTrue() - assertThat(fixture.published).containsExactly(incumbent, owner).inOrder() + assertThat(registration.hasLease).isTrue() + assertThat(fixture.published).containsExactly("incumbent", "owner").inOrder() } @Test fun `a resumed owner takes the nav lease from a resumed leaseholder`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - fixture.acquireLease(incumbent) + fixture.acquireLease("incumbent", TestOwner(RESUMED)) - fixture.acquireLease(owner) + val registration = fixture.acquireLease("owner", TestOwner(RESUMED)) - assertThat(navLeaseCoordinator.hasLease(owner)).isTrue() + assertThat(registration.hasLease).isTrue() } @Test - fun `acquiring the nav lease clears the previous owner's published nav data before publishing`() { + fun `acquiring the nav lease unpublishes the previous publisher's nav state before publishing its own`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator val calls = mutableListOf() - navLeaseCoordinator.registerCandidate( - previous, - Lifecycle.State.RESUMED, - onPublishData = { calls += "publish previous" }, - onClearPublishedData = { calls += "clear previous" }, - ) - navLeaseCoordinator.onBackStackUpdate(previous, fixture.scope) - navLeaseCoordinator.registerCandidate( - incoming, - Lifecycle.State.RESUMED, - onPublishData = { calls += "publish incoming" }, - onClearPublishedData = { calls += "clear incoming" }, + fixture.acquireLease( + "previous", + TestOwner(RESUMED), + navState = + fixture.navState( + "previous", + onPublishTo = { calls += "publish previous" }, + onUnpublishFrom = { calls += "unpublish previous" }, + ), ) + val incoming = + fixture.register( + "incoming", + TestOwner(RESUMED), + fixture.navState( + "incoming", + onPublishTo = { calls += "publish incoming" }, + onUnpublishFrom = { calls += "unpublish incoming" }, + ), + ) - navLeaseCoordinator.onBackStackUpdate(incoming, fixture.scope) + incoming.onBackStackUpdate() assertThat(calls) - .containsExactly("publish previous", "clear previous", "publish incoming") + .containsExactly("publish previous", "unpublish previous", "publish incoming") .inOrder() } @Test - fun `regaining the nav lease doesn't clear the owner's own published nav data`() { + fun `regaining the nav lease doesn't unpublish the owner's own published nav state`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - fixture.acquireLease(previous) - fixture.acquireLease(owner) - navLeaseCoordinator.pauseCandidate(owner, null) - fixture.cleared.clear() + fixture.acquireLease("previous", TestOwner(RESUMED)) + val owner = TestOwner(RESUMED) + val registration = fixture.acquireLease("owner", owner) + fixture.withoutScope { owner.lifecycle.currentState = STARTED } + fixture.unpublished.clear() - navLeaseCoordinator.resumeCandidate(owner, fixture.scope) + owner.lifecycle.currentState = RESUMED - assertThat(navLeaseCoordinator.hasLease(owner)).isTrue() - assertThat(fixture.published).containsExactly(previous, owner, owner).inOrder() - assertThat(fixture.cleared).isEmpty() + assertThat(registration.hasLease).isTrue() + assertThat(fixture.published).containsExactly("previous", "owner", "owner").inOrder() + assertThat(fixture.unpublished).isEmpty() } @Test - fun `releasing the nav lease leaves the owner's published nav data in place`() { + fun `releasing the nav lease leaves the owner's published nav state in place`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - fixture.acquireLease(previous) - fixture.acquireLease(owner) - fixture.cleared.clear() + fixture.acquireLease("previous", TestOwner(RESUMED)) + val owner = TestOwner(RESUMED) + val registration = fixture.acquireLease("owner", owner) + fixture.unpublished.clear() - navLeaseCoordinator.pauseCandidate(owner, null) + fixture.withoutScope { owner.lifecycle.currentState = STARTED } - assertThat(navLeaseCoordinator.hasLease(owner)).isFalse() - assertThat(fixture.cleared).isEmpty() + assertThat(registration.hasLease).isFalse() + assertThat(fixture.unpublished).isEmpty() } /** - * The coordinator records the incoming owner as the publisher before calling into the scope, so a - * clear callback that throws mid-handoff doesn't strand the incoming owner's nav data: the next - * handoff still clears it. + * The coordinator records the incoming owner as the publisher before calling into the scope, so + * unpublishing that throws mid-handoff doesn't strand the incoming owner's nav state: the next + * handoff still unpublishes it. */ @Test - fun `if a clear attempt throws during a handoff, the next handoff clears the new leaseholder's nav data`() { + fun `if unpublishing throws during a handoff, the next handoff unpublishes the new leaseholder's nav state`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - val failure = IllegalStateException("clear failed") - var failClear = true - val firstOwner = previous - val secondOwner = incoming - val thirdOwner = owner + val failure = IllegalStateException("unpublish failed") fixture.acquireLease( - firstOwner, - onClearPublishedData = { - if (failClear) { - failClear = false - throw failure - } - fixture.cleared += firstOwner - }, + "first", + TestOwner(RESUMED), + navState = fixture.navState("first", onUnpublishFrom = { throw failure }), ) - fixture.register(secondOwner, Lifecycle.State.RESUMED) + val second = fixture.register("second", TestOwner(RESUMED)) - val thrown = - assertThrows(IllegalStateException::class.java) { - navLeaseCoordinator.onBackStackUpdate(secondOwner, fixture.scope) - } + val thrown = assertThrows(IllegalStateException::class.java) { second.onBackStackUpdate() } assertThat(thrown).isSameInstanceAs(failure) - assertThat(navLeaseCoordinator.hasLease(secondOwner)).isTrue() + assertThat(second.hasLease).isTrue() - fixture.acquireLease(thirdOwner) + fixture.acquireLease("third", TestOwner(RESUMED)) - assertThat(fixture.cleared).containsExactly(secondOwner) + assertThat(fixture.unpublished).containsExactly("second") } @Test - fun `later back stack updates do not republish nav data or reclaim the nav lease`() { + fun `later back stack updates do not republish nav state or reclaim the nav lease`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - val first = Any() - val second = Any() - fixture.acquireLease(first) - fixture.acquireLease(second, state = Lifecycle.State.RESUMED) + val first = fixture.acquireLease("first", TestOwner(RESUMED)) + val second = fixture.acquireLease("second", TestOwner(RESUMED)) + + first.onBackStackUpdate() + + assertThat(second.hasLease).isTrue() + assertThat(fixture.published).containsExactly("first", "second").inOrder() + } + + @Test + fun `a back stack update skipped by configureScope doesn't acquire the nav lease`() { + val fixture = Fixture.create() + val registration = fixture.register("owner", TestOwner(CREATED)) + + fixture.withoutScope { registration.onBackStackUpdate() } + + assertThat(registration.hasLease).isFalse() + assertThat(fixture.published).isEmpty() + } + + @Test + fun `a back stack update skipped by configureScope doesn't count as the first back stack update`() { + val fixture = Fixture.create() + val registration = fixture.register("owner", TestOwner(CREATED)) + fixture.withoutScope { registration.onBackStackUpdate() } - navLeaseCoordinator.onBackStackUpdate(first, fixture.scope) + registration.onBackStackUpdate() - assertThat(navLeaseCoordinator.hasLease(second)).isTrue() - assertThat(fixture.published).containsExactly(first, second).inOrder() + assertThat(registration.hasLease).isTrue() + assertThat(fixture.published).containsExactly("owner") } + // endregion + + // region pausing + @Test fun `pausing the leaseholder hands the nav lease to the most recently resumed owner that's still resumed`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - val older = Any() - val recent = Any() - fixture.acquireLease(older) - fixture.acquireLease(recent) - fixture.acquireLease(incoming, state = Lifecycle.State.RESUMED) + fixture.acquireLease("older", TestOwner(RESUMED)) + val recent = fixture.acquireLease("recent", TestOwner(RESUMED)) + val incomingOwner = TestOwner(RESUMED) + fixture.acquireLease("incoming", incomingOwner) - navLeaseCoordinator.pauseCandidate(incoming, fixture.scope) + incomingOwner.lifecycle.currentState = STARTED - assertThat(navLeaseCoordinator.hasLease(recent)).isTrue() - assertThat(fixture.published.last()).isSameInstanceAs(recent) + assertThat(recent.hasLease).isTrue() + assertThat(fixture.published.last()).isEqualTo("recent") } @Test fun `pausing the leaseholder keeps the nav lease when other eligible owners are only paused`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - fixture.acquireLease(owner) - navLeaseCoordinator.pauseCandidate(owner, fixture.scope) - fixture.acquireLease(incoming) + val owner = TestOwner(RESUMED) + fixture.acquireLease("owner", owner) + owner.lifecycle.currentState = STARTED + val incomingOwner = TestOwner(RESUMED) + val incoming = fixture.acquireLease("incoming", incomingOwner) - navLeaseCoordinator.pauseCandidate(incoming, fixture.scope) + incomingOwner.lifecycle.currentState = STARTED - assertThat(navLeaseCoordinator.hasLease(incoming)).isTrue() - assertThat(fixture.published).containsExactly(owner, incoming).inOrder() + assertThat(incoming.hasLease).isTrue() + assertThat(fixture.published).containsExactly("owner", "incoming").inOrder() } @Test fun `a displaced paused owner regains the nav lease when it resumes`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - fixture.acquireLease(owner) - navLeaseCoordinator.pauseCandidate(owner, fixture.scope) - fixture.acquireLease(incoming) - navLeaseCoordinator.pauseCandidate(incoming, fixture.scope) + val owner = TestOwner(RESUMED) + val registration = fixture.acquireLease("owner", owner) + owner.lifecycle.currentState = STARTED + val incomingOwner = TestOwner(RESUMED) + fixture.acquireLease("incoming", incomingOwner) + incomingOwner.lifecycle.currentState = STARTED - navLeaseCoordinator.resumeCandidate(owner, fixture.scope) + owner.lifecycle.currentState = RESUMED - assertThat(navLeaseCoordinator.hasLease(owner)).isTrue() - assertThat(fixture.published).containsExactly(owner, incoming, owner).inOrder() + assertThat(registration.hasLease).isTrue() + assertThat(fixture.published).containsExactly("owner", "incoming", "owner").inOrder() } @Test fun `pausing the leaseholder skips a resumed owner without a back stack update`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - val eligible = Any() - val uninitialized = Any() - fixture.acquireLease(eligible) - fixture.register(uninitialized, Lifecycle.State.RESUMED) - fixture.acquireLease(incoming, state = Lifecycle.State.RESUMED) + val eligible = fixture.acquireLease("eligible", TestOwner(RESUMED)) + val uninitialized = fixture.register("uninitialized", TestOwner(RESUMED)) + val incomingOwner = TestOwner(RESUMED) + fixture.acquireLease("incoming", incomingOwner) - navLeaseCoordinator.pauseCandidate(incoming, fixture.scope) + incomingOwner.lifecycle.currentState = STARTED - assertThat(navLeaseCoordinator.hasLease(eligible)).isTrue() - assertThat(navLeaseCoordinator.hasLease(uninitialized)).isFalse() + assertThat(eligible.hasLease).isTrue() + assertThat(uninitialized.hasLease).isFalse() } @Test fun `pausing the only eligible owner keeps the nav lease`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - fixture.acquireLease(owner) + val owner = TestOwner(RESUMED) + val registration = fixture.acquireLease("owner", owner) - navLeaseCoordinator.pauseCandidate(owner, fixture.scope) + owner.lifecycle.currentState = STARTED - assertThat(navLeaseCoordinator.hasLease(owner)).isTrue() - assertThat(fixture.published).containsExactly(owner) + assertThat(registration.hasLease).isTrue() + assertThat(fixture.published).containsExactly("owner") } @Test fun `pausing the only eligible owner without a scope keeps the nav lease`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - fixture.acquireLease(owner) + val owner = TestOwner(RESUMED) + val registration = fixture.acquireLease("owner", owner) - navLeaseCoordinator.pauseCandidate(owner, null) + fixture.withoutScope { owner.lifecycle.currentState = STARTED } - assertThat(navLeaseCoordinator.hasLease(owner)).isTrue() + assertThat(registration.hasLease).isTrue() } @Test - fun `resuming a paused leaseholder does not republish its nav data`() { + fun `resuming a paused leaseholder does not republish its nav state`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - fixture.acquireLease(owner) - navLeaseCoordinator.pauseCandidate(owner, fixture.scope) + val owner = TestOwner(RESUMED) + val registration = fixture.acquireLease("owner", owner) + owner.lifecycle.currentState = STARTED - navLeaseCoordinator.resumeCandidate(owner, fixture.scope) + owner.lifecycle.currentState = RESUMED - assertThat(navLeaseCoordinator.hasLease(owner)).isTrue() - assertThat(fixture.published).containsExactly(owner) + assertThat(registration.hasLease).isTrue() + assertThat(fixture.published).containsExactly("owner") } + // endregion + + // region stopping + + /** + * A real lifecycle pauses before it stops, so the leaseholder here is an owner that acquired the + * lease without ever resuming, and the other owner resumes without a scope, which records the + * resume without restoring its lease. + */ @Test fun `stopping the leaseholder restores a resumed owner`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - fixture.acquireLease(previous) - fixture.acquireLease(owner, state = Lifecycle.State.RESUMED) + val previousOwner = TestOwner(RESUMED) + val previous = fixture.acquireLease("previous", previousOwner) + previousOwner.lifecycle.currentState = STARTED + val owner = TestOwner(STARTED) + fixture.acquireLease("owner", owner) + fixture.withoutScope { previousOwner.lifecycle.currentState = RESUMED } - navLeaseCoordinator.stopCandidate(owner, fixture.scope) + owner.lifecycle.currentState = CREATED - assertThat(navLeaseCoordinator.hasLease(previous)).isTrue() - assertThat(fixture.published.last()).isSameInstanceAs(previous) + assertThat(previous.hasLease).isTrue() + assertThat(fixture.published.last()).isEqualTo("previous") } @Test fun `stopping the only eligible owner keeps the nav lease`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - fixture.acquireLease(owner) - navLeaseCoordinator.pauseCandidate(owner, fixture.scope) + val owner = TestOwner(RESUMED) + val registration = fixture.acquireLease("owner", owner) + owner.lifecycle.currentState = STARTED - navLeaseCoordinator.stopCandidate(owner, fixture.scope) + owner.lifecycle.currentState = CREATED - assertThat(navLeaseCoordinator.hasLease(owner)).isTrue() - assertThat(fixture.published).containsExactly(owner) + assertThat(registration.hasLease).isTrue() + assertThat(fixture.published).containsExactly("owner") } @Test fun `stopping the only eligible owner without a scope keeps the nav lease`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - fixture.acquireLease(owner) - navLeaseCoordinator.pauseCandidate(owner, null) + val owner = TestOwner(RESUMED) + val registration = fixture.acquireLease("owner", owner) + fixture.withoutScope { owner.lifecycle.currentState = STARTED } - navLeaseCoordinator.stopCandidate(owner, null) + fixture.withoutScope { owner.lifecycle.currentState = CREATED } - assertThat(navLeaseCoordinator.hasLease(owner)).isTrue() + assertThat(registration.hasLease).isTrue() } @Test fun `stopping the leaseholder doesn't hand the nav lease to a paused owner`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - fixture.acquireLease(previous) - navLeaseCoordinator.pauseCandidate(previous, fixture.scope) - fixture.acquireLease(owner) - navLeaseCoordinator.pauseCandidate(owner, fixture.scope) + val previousOwner = TestOwner(RESUMED) + val previous = fixture.acquireLease("previous", previousOwner) + previousOwner.lifecycle.currentState = STARTED + val owner = TestOwner(RESUMED) + val registration = fixture.acquireLease("owner", owner) + owner.lifecycle.currentState = STARTED - navLeaseCoordinator.stopCandidate(owner, fixture.scope) + owner.lifecycle.currentState = CREATED - assertThat(navLeaseCoordinator.hasLease(owner)).isTrue() - assertThat(navLeaseCoordinator.hasLease(previous)).isFalse() - assertThat(fixture.published).containsExactly(previous, owner).inOrder() - assertThat(fixture.cleared).containsExactly(previous) + assertThat(registration.hasLease).isTrue() + assertThat(previous.hasLease).isFalse() + assertThat(fixture.published).containsExactly("previous", "owner").inOrder() + assertThat(fixture.unpublished).containsExactly("previous") } @Test fun `a paused owner passed over when the leaseholder stopped gets the nav lease when it resumes`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - fixture.acquireLease(previous) - navLeaseCoordinator.pauseCandidate(previous, fixture.scope) - fixture.acquireLease(owner) - navLeaseCoordinator.pauseCandidate(owner, fixture.scope) - navLeaseCoordinator.stopCandidate(owner, fixture.scope) + val previousOwner = TestOwner(RESUMED) + val previous = fixture.acquireLease("previous", previousOwner) + previousOwner.lifecycle.currentState = STARTED + val owner = TestOwner(RESUMED) + fixture.acquireLease("owner", owner) + owner.lifecycle.currentState = CREATED - navLeaseCoordinator.resumeCandidate(previous, fixture.scope) + previousOwner.lifecycle.currentState = RESUMED - assertThat(navLeaseCoordinator.hasLease(previous)).isTrue() - assertThat(fixture.published).containsExactly(previous, owner, previous).inOrder() + assertThat(previous.hasLease).isTrue() + assertThat(fixture.published).containsExactly("previous", "owner", "previous").inOrder() } @Test fun `restarting a stopped leaseholder keeps the nav lease`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - fixture.acquireLease(owner) - navLeaseCoordinator.pauseCandidate(owner, fixture.scope) - navLeaseCoordinator.stopCandidate(owner, fixture.scope) + val owner = TestOwner(RESUMED) + val registration = fixture.acquireLease("owner", owner) + owner.lifecycle.currentState = CREATED - navLeaseCoordinator.startCandidate(owner) + owner.lifecycle.currentState = STARTED - assertThat(navLeaseCoordinator.hasLease(owner)).isTrue() - assertThat(fixture.published).containsExactly(owner) + assertThat(registration.hasLease).isTrue() + assertThat(fixture.published).containsExactly("owner") } @Test fun `stopping an owner excludes it from later nav lease fallback`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - fixture.acquireLease(previous) - fixture.acquireLease(owner) - navLeaseCoordinator.pauseCandidate(previous, fixture.scope) - navLeaseCoordinator.stopCandidate(previous, fixture.scope) + val previousOwner = TestOwner(RESUMED) + val previous = fixture.acquireLease("previous", previousOwner) + val registration = fixture.acquireLease("owner", TestOwner(RESUMED)) + previousOwner.lifecycle.currentState = CREATED - navLeaseCoordinator.unregisterCandidate(owner, fixture.scope) + registration.dispose() - assertThat(navLeaseCoordinator.hasLease(previous)).isFalse() - assertThat(fixture.published).containsExactly(previous, owner).inOrder() + assertThat(previous.hasLease).isFalse() + assertThat(fixture.published).containsExactly("previous", "owner").inOrder() } + // endregion + + // region lifecycle events without a scope + @Test fun `resuming without a scope keeps the owner eligible for nav lease fallback`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - fixture.register(owner, Lifecycle.State.CREATED) - navLeaseCoordinator.resumeCandidate(owner, null) - navLeaseCoordinator.onBackStackUpdate(owner, fixture.scope) - fixture.acquireLease(incoming, state = Lifecycle.State.CREATED) + val owner = TestOwner(CREATED) + val registration = fixture.acquireLease("owner", owner) + val incomingOwner = TestOwner(RESUMED) + fixture.acquireLease("incoming", incomingOwner) + fixture.withoutScope { owner.lifecycle.currentState = RESUMED } - navLeaseCoordinator.pauseCandidate(incoming, fixture.scope) + incomingOwner.lifecycle.currentState = STARTED - assertThat(navLeaseCoordinator.hasLease(owner)).isTrue() + assertThat(registration.hasLease).isTrue() } @Test fun `resuming without a scope does not transfer the nav lease`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - fixture.acquireLease(incumbent) - fixture.register(owner, Lifecycle.State.CREATED) - navLeaseCoordinator.stopCandidate(owner, fixture.scope) - navLeaseCoordinator.onBackStackUpdate(owner, fixture.scope) + val incumbent = fixture.acquireLease("incumbent", TestOwner(RESUMED)) + val owner = TestOwner(STARTED) + val registration = fixture.register("owner", owner) + owner.lifecycle.currentState = CREATED + registration.onBackStackUpdate() - navLeaseCoordinator.resumeCandidate(owner, null) + fixture.withoutScope { owner.lifecycle.currentState = RESUMED } - assertThat(navLeaseCoordinator.hasLease(incumbent)).isTrue() - assertThat(fixture.published).containsExactly(incumbent) + assertThat(incumbent.hasLease).isTrue() + assertThat(fixture.published).containsExactly("incumbent") } @Test fun `pausing the leaseholder without a scope releases the nav lease without publishing a fallback`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - fixture.acquireLease(previous) - fixture.acquireLease(owner, state = Lifecycle.State.RESUMED) + val previous = fixture.acquireLease("previous", TestOwner(RESUMED)) + val owner = TestOwner(RESUMED) + val registration = fixture.acquireLease("owner", owner) - navLeaseCoordinator.pauseCandidate(owner, null) + fixture.withoutScope { owner.lifecycle.currentState = STARTED } - assertThat(navLeaseCoordinator.hasLease(owner)).isFalse() - assertThat(navLeaseCoordinator.hasLease(previous)).isFalse() - assertThat(fixture.published).containsExactly(previous, owner).inOrder() + assertThat(registration.hasLease).isFalse() + assertThat(previous.hasLease).isFalse() + assertThat(fixture.published).containsExactly("previous", "owner").inOrder() } + /** + * A real lifecycle pauses before it stops, so the leaseholder here is an owner that acquired the + * lease without ever resuming, and the other owner resumes without a scope, which records the + * resume without restoring its lease. + */ @Test fun `stopping the leaseholder without a scope releases the nav lease without publishing a fallback`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - fixture.acquireLease(previous) - fixture.acquireLease(owner, state = Lifecycle.State.RESUMED) + val previousOwner = TestOwner(RESUMED) + val previous = fixture.acquireLease("previous", previousOwner) + previousOwner.lifecycle.currentState = STARTED + val owner = TestOwner(STARTED) + val registration = fixture.acquireLease("owner", owner) + fixture.withoutScope { previousOwner.lifecycle.currentState = RESUMED } - navLeaseCoordinator.stopCandidate(owner, null) + fixture.withoutScope { owner.lifecycle.currentState = CREATED } - assertThat(navLeaseCoordinator.hasLease(owner)).isFalse() - assertThat(navLeaseCoordinator.hasLease(previous)).isFalse() - assertThat(fixture.published).containsExactly(previous, owner).inOrder() + assertThat(registration.hasLease).isFalse() + assertThat(previous.hasLease).isFalse() + assertThat(fixture.published).containsExactly("previous", "owner").inOrder() } - @Test - fun `an unregistered owner's nav transaction is not retained`() { - val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - val other = Any() - fixture.acquireLease(owner) - - navLeaseCoordinator.retainTransaction(other, transaction) - navLeaseCoordinator.unregisterCandidate(owner, fixture.scope) + // endregion - verifyNoInteractions(transaction) - } + // region unregistering @Test - fun `a nav transaction recorded without the lease is ignored`() { + fun `disposing the leaseholder unpublishes its published nav state and releases the nav lease`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - fixture.acquireLease(owner) - fixture.acquireLease(incoming, state = Lifecycle.State.RESUMED) + val registration = fixture.acquireLease("owner", TestOwner(RESUMED)) - navLeaseCoordinator.retainTransaction(owner, transaction) - navLeaseCoordinator.unregisterCandidate(incoming, fixture.scope) + registration.dispose() - assertThat(navLeaseCoordinator.hasLease(owner)).isTrue() - assertThat(navLeaseCoordinator.isNavTransaction(transaction)).isFalse() - assertThat(fixture.scope.transaction).isNull() - - navLeaseCoordinator.unregisterCandidate(owner, fixture.scope) - - verifyNoInteractions(transaction) + assertThat(fixture.unpublished).containsExactly("owner") + assertThat(registration.hasLease).isFalse() } @Test - fun `an unfinished nav transaction is restored when its owner regains the lease`() { + fun `destroying an owner's lifecycle unregisters its candidate`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - fixture.acquireLease(owner, transaction = transaction) - fixture.acquireLease(incoming, state = Lifecycle.State.RESUMED) + val owner = TestOwner(RESUMED) + val registration = fixture.acquireLease("owner", owner) - assertThat(fixture.scope.transaction).isNull() - navLeaseCoordinator.pauseCandidate(incoming, fixture.scope) + owner.lifecycle.currentState = DESTROYED - assertThat(fixture.scope.transaction).isSameInstanceAs(transaction) - assertThat(navLeaseCoordinator.hasLease(owner)).isTrue() - verify(transaction, never()).finish(any()) + assertThat(registration.hasLease).isFalse() + assertThat(fixture.unpublished).containsExactly("owner") + assertThat(fixture.unregistered).containsExactly("owner" to fixture.scope) } @Test - fun `restoring a nav transaction preserves an unrelated scope transaction`() { + fun `disposing a former leaseholder preserves the current owner's published nav state`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - fixture.acquireLease(owner, transaction = transaction) - fixture.acquireLease(incoming, state = Lifecycle.State.RESUMED) - fixture.scope.transaction = unrelatedTransaction + val previous = fixture.acquireLease("previous", TestOwner(RESUMED)) + val owner = TestOwner(RESUMED) + val registration = fixture.acquireLease("owner", owner) + owner.lifecycle.currentState = STARTED + fixture.unpublished.clear() - navLeaseCoordinator.pauseCandidate(incoming, fixture.scope) + registration.dispose() - assertThat(fixture.scope.transaction).isSameInstanceAs(unrelatedTransaction) + assertThat(previous.hasLease).isTrue() + assertThat(fixture.unpublished).isEmpty() } @Test - fun `a finished nav transaction is discarded when its owner regains the lease`() { + fun `disposing an owner whose nav state is still published without the nav lease unpublishes it`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - fixture.acquireLease(owner, transaction = transaction) - fixture.acquireLease(incoming, state = Lifecycle.State.RESUMED) - whenever(transaction.isFinished).thenReturn(true) + val previous = fixture.acquireLease("previous", TestOwner(RESUMED)) + val owner = TestOwner(RESUMED) + val registration = fixture.acquireLease("owner", owner) + fixture.withoutScope { owner.lifecycle.currentState = STARTED } + fixture.published.clear() + fixture.unpublished.clear() - navLeaseCoordinator.pauseCandidate(incoming, fixture.scope) + registration.dispose() - assertThat(fixture.scope.transaction).isNull() - assertThat(navLeaseCoordinator.hasLease(owner)).isTrue() - assertThat(navLeaseCoordinator.isNavTransaction(transaction)).isFalse() + assertThat(fixture.unpublished).containsExactly("owner") + assertThat(fixture.published).isEmpty() + assertThat(previous.hasLease).isFalse() } @Test - fun `a released unfinished nav transaction is not restored when its owner regains the lease`() { + fun `disposing the leaseholder hands the nav lease to the most recently resumed eligible owner`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - fixture.acquireLease(owner, transaction = transaction) - fixture.acquireLease(incoming, state = Lifecycle.State.RESUMED) + val olderOwner = TestOwner(RESUMED) + fixture.acquireLease("older", olderOwner) + olderOwner.lifecycle.currentState = STARTED + val previousOwner = TestOwner(RESUMED) + val previous = fixture.acquireLease("previous", previousOwner) + previousOwner.lifecycle.currentState = STARTED + val registration = fixture.acquireLease("owner", TestOwner(RESUMED)) - navLeaseCoordinator.releaseTransaction(owner, transaction) - navLeaseCoordinator.pauseCandidate(incoming, fixture.scope) + registration.dispose() - assertThat(navLeaseCoordinator.hasLease(owner)).isTrue() - assertThat(fixture.scope.transaction).isNull() - assertThat(navLeaseCoordinator.isNavTransaction(transaction)).isFalse() + assertThat(previous.hasLease).isTrue() + assertThat(fixture.published.last()).isEqualTo("previous") + assertThat(fixture.unpublished).containsExactly("older", "previous", "owner").inOrder() } @Test - fun `releasing a nav transaction other than the retained one keeps the retained one restorable`() { + fun `disposing the leaseholder without a scope releases the nav lease`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - fixture.acquireLease(owner, transaction = transaction) - fixture.acquireLease(incoming, state = Lifecycle.State.RESUMED) + val previous = fixture.acquireLease("previous", TestOwner(RESUMED)) + val registration = fixture.acquireLease("owner", TestOwner(RESUMED)) + fixture.unpublished.clear() - navLeaseCoordinator.releaseTransaction(owner, unrelatedTransaction) - navLeaseCoordinator.pauseCandidate(incoming, fixture.scope) + fixture.withoutScope { registration.dispose() } - assertThat(navLeaseCoordinator.hasLease(owner)).isTrue() - assertThat(fixture.scope.transaction).isSameInstanceAs(transaction) + assertThat(registration.hasLease).isFalse() + assertThat(previous.hasLease).isFalse() + assertThat(fixture.published).containsExactly("previous", "owner").inOrder() + assertThat(fixture.unpublished).isEmpty() } @Test - fun `a displaced owner's retained nav transaction remains identifiable until unregistering`() { + fun `disposing the leaseholder forgets the owner when unpublishing its nav state throws`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - fixture.acquireLease(owner, transaction = transaction) - fixture.acquireLease(incoming, state = Lifecycle.State.RESUMED) + val failure = IllegalStateException("unpublish failed") + val owner = TestOwner(RESUMED) + val registration = + fixture.acquireLease( + "owner", + owner, + navState = fixture.navState("owner", onUnpublishFrom = { throw failure }), + ) - assertThat(navLeaseCoordinator.isNavTransaction(transaction)).isTrue() - assertThat(navLeaseCoordinator.isNavTransaction(unrelatedTransaction)).isFalse() + val thrown = assertThrows(IllegalStateException::class.java) { registration.dispose() } - navLeaseCoordinator.unregisterCandidate(owner, fixture.scope) + assertThat(thrown).isSameInstanceAs(failure) + assertThat(registration.hasLease).isFalse() + + fixture.published.clear() + owner.lifecycle.currentState = STARTED + owner.lifecycle.currentState = RESUMED + fixture.acquireLease("incoming", TestOwner(RESUMED)) - assertThat(navLeaseCoordinator.isNavTransaction(transaction)).isFalse() + assertThat(fixture.published).containsExactly("incoming") + assertThat(fixture.unpublished).isEmpty() } @Test - fun `acquiring the nav lease binds its retained transaction before publishing nav data`() { + fun `disposing an owner before its first back stack update keeps it from acquiring the nav lease`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - val transactionAtPublication = mutableListOf() - navLeaseCoordinator.registerCandidate( - owner, - Lifecycle.State.RESUMED, - onPublishData = { transactionAtPublication += it.transaction }, - onClearPublishedData = {}, - ) - navLeaseCoordinator.onBackStackUpdate(owner, fixture.scope) - navLeaseCoordinator.retainTransaction(owner, transaction) - fixture.scope.transaction = transaction - fixture.acquireLease(incoming, state = Lifecycle.State.RESUMED) + val registration = fixture.register("owner", TestOwner(RESUMED)) + registration.dispose() - navLeaseCoordinator.pauseCandidate(incoming, fixture.scope) + registration.onBackStackUpdate() - assertThat(navLeaseCoordinator.hasLease(owner)).isTrue() - assertThat(transactionAtPublication).containsExactly(null, transaction).inOrder() + assertThat(registration.hasLease).isFalse() + assertThat(fixture.published).isEmpty() } @Test - fun `unregistering the leaseholder finishes its transaction and clears its published nav data`() { + fun `dispose() - notifies the nav state that it was unregistered after unpublishing it`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - fixture.acquireLease(owner, transaction = transaction) + val calls = mutableListOf() + val registration = + fixture.acquireLease( + "owner", + TestOwner(RESUMED), + navState = + fixture.navState( + "owner", + onUnpublishFrom = { calls += "unpublish" }, + onUnregistered = { calls += "unregistered" }, + ), + ) - navLeaseCoordinator.unregisterCandidate(owner, fixture.scope) + registration.dispose() - verify(transaction).finish(SpanStatus.OK) - assertThat(fixture.scope.transaction).isNull() - assertThat(fixture.cleared).containsExactly(owner) - assertThat(navLeaseCoordinator.hasLease(owner)).isFalse() + assertThat(calls).containsExactly("unpublish", "unregistered").inOrder() } @Test - fun `unregistering a former leaseholder preserves the current owner's published nav data`() { + fun `dispose() - passes the scope to the nav state's onUnregistered`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - fixture.acquireLease(previous) - fixture.acquireLease(owner, state = Lifecycle.State.RESUMED) - navLeaseCoordinator.pauseCandidate(owner, fixture.scope) - fixture.cleared.clear() + val registration = fixture.register("owner", TestOwner(CREATED)) - navLeaseCoordinator.unregisterCandidate(owner, fixture.scope) + registration.dispose() - assertThat(navLeaseCoordinator.hasLease(previous)).isTrue() - assertThat(fixture.cleared).isEmpty() + assertThat(fixture.unregistered).containsExactly("owner" to fixture.scope) } @Test - fun `unregistering the leaseholder hands the nav lease to the most recently resumed eligible owner`() { + fun `dispose() - passes no scope to the nav state's onUnregistered when configureScope skips it`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - val older = Any() - fixture.acquireLease(older) - navLeaseCoordinator.pauseCandidate(older, fixture.scope) - fixture.acquireLease(previous) - navLeaseCoordinator.pauseCandidate(previous, fixture.scope) - fixture.acquireLease(owner) + val registration = fixture.register("owner", TestOwner(CREATED)) - navLeaseCoordinator.unregisterCandidate(owner, fixture.scope) + fixture.withoutScope { registration.dispose() } - assertThat(navLeaseCoordinator.hasLease(previous)).isTrue() - assertThat(fixture.published.last()).isSameInstanceAs(previous) - assertThat(fixture.cleared).containsExactly(older, previous, owner).inOrder() + assertThat(fixture.unregistered).containsExactly("owner" to null) } @Test - fun `unregistering the leaseholder without a scope releases the nav lease`() { + fun `dispose() - notifies the nav state once with the scope when unpublishing it throws`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - fixture.acquireLease(previous) - fixture.acquireLease(owner) - fixture.cleared.clear() + val failure = IllegalStateException("unpublish failed") + val registration = + fixture.acquireLease( + "owner", + TestOwner(RESUMED), + navState = fixture.navState("owner", onUnpublishFrom = { throw failure }), + ) - navLeaseCoordinator.unregisterCandidate(owner, null) + val thrown = assertThrows(IllegalStateException::class.java) { registration.dispose() } - assertThat(navLeaseCoordinator.hasLease(owner)).isFalse() - assertThat(navLeaseCoordinator.hasLease(previous)).isFalse() - assertThat(fixture.published).containsExactly(previous, owner).inOrder() - assertThat(fixture.cleared).isEmpty() + assertThat(thrown).isSameInstanceAs(failure) + assertThat(fixture.unregistered).containsExactly("owner" to fixture.scope) } @Test - fun `unregistering the leaseholder without a scope doesn't finish its nav transaction`() { + fun `dispose() - notifies the nav state only once when called again`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - fixture.acquireLease(owner, transaction = transaction) + val registration = fixture.register("owner", TestOwner(CREATED)) + registration.dispose() - navLeaseCoordinator.unregisterCandidate(owner, null) + registration.dispose() - verify(transaction, never()).finish(any()) + assertThat(fixture.unregistered).hasSize(1) } @Test - fun `unregistering a former leaseholder finishes its retained transaction without clearing an unrelated one`() { + fun `dispose() - notifies the nav state only once when its lifecycle is destroyed afterward`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - whenever(transaction.status).thenReturn(SpanStatus.CANCELLED) - fixture.acquireLease(owner, transaction = transaction) - fixture.acquireLease(incoming, state = Lifecycle.State.RESUMED) - fixture.scope.transaction = unrelatedTransaction - fixture.cleared.clear() + val owner = TestOwner(RESUMED) + val registration = fixture.register("owner", owner) + registration.dispose() - navLeaseCoordinator.unregisterCandidate(owner, fixture.scope) + owner.lifecycle.currentState = DESTROYED - verify(transaction).finish(SpanStatus.CANCELLED) - assertThat(fixture.scope.transaction).isSameInstanceAs(unrelatedTransaction) - assertThat(fixture.cleared).isEmpty() + assertThat(fixture.unregistered).hasSize(1) } @Test - fun `unregistering the leaseholder forgets the owner when finishing its transaction throws`() { + fun `dispose() - stops observing the lifecycle`() { val fixture = Fixture.create() - val navLeaseCoordinator = fixture.navLeaseCoordinator - val failure = IllegalStateException("finish failed") - fixture.acquireLease(owner, transaction = transaction) - doThrow(failure).whenever(transaction).finish(SpanStatus.OK) + val owner = TestOwner(RESUMED) + val registration = fixture.register("owner", owner) - val thrown = - assertThrows(IllegalStateException::class.java) { - navLeaseCoordinator.unregisterCandidate(owner, fixture.scope) - } + registration.dispose() - assertThat(thrown).isSameInstanceAs(failure) - assertThat(navLeaseCoordinator.hasLease(owner)).isFalse() - assertThat(navLeaseCoordinator.isNavTransaction(transaction)).isFalse() + assertThat(owner.lifecycle.observerCount).isEqualTo(0) + } - fixture.published.clear() - navLeaseCoordinator.resumeCandidate(owner, fixture.scope) - fixture.acquireLease(incoming) + // endregion - assertThat(fixture.published).containsExactly(incoming) - assertThat(fixture.cleared).isEmpty() + /** + * A [LifecycleOwner] whose lifecycle starts in [initialState] and can be moved through states by + * setting [LifecycleRegistry.currentState], which dispatches the events in between. + */ + private class TestOwner(initialState: Lifecycle.State) : LifecycleOwner { + override val lifecycle: LifecycleRegistry = + LifecycleRegistry.createUnsafe(this).apply { + // A lifecycle must be created before it can be destroyed. + currentState = CREATED + currentState = initialState + } } - private class Fixture { + private class Fixture private constructor() { val navLeaseCoordinator = NavLeaseCoordinator() val scope = Scope(SentryOptions()) - val published = mutableListOf() - val cleared = mutableListOf() + /** Whether [scopes] runs `configureScope` callbacks; when false, it skips them. */ + private var providesScope = true + + val scopes = + mock().also { scopes -> + doAnswer { + if (providesScope) { + (it.arguments[0] as ScopeCallback).run(scope) + } + null + } + .whenever(scopes) + .configureScope(any()) + } + + /** Names of the nav states published, in publication order. */ + val published = mutableListOf() + + /** Names of the nav states unpublished, in the order they were unpublished. */ + val unpublished = mutableListOf() + + /** Names of the nav states notified of unregistration, with the scope they were given. */ + val unregistered = mutableListOf>() + + /** + * `LifecycleRegistry` holds its owner weakly and throws once it's collected, so the fixture + * keeps registered owners alive for the test. + */ + private val owners = mutableListOf() + + /** + * Nav state named [name] that, by default, records its publication in [published], its + * unpublishing in [unpublished], and its unregistration in [unregistered]. + */ + fun navState( + name: String, + onPublishTo: (IScope) -> Unit = { published += name }, + onUnpublishFrom: (IScope) -> Unit = { unpublished += name }, + onUnregistered: (IScope?) -> Unit = { unregistered += name to it }, + ): LeasedNavState = FakeLeasedNavState(onPublishTo, onUnpublishFrom, onUnregistered) + + /** Registers a candidate for [owner]'s lifecycle with the nav state named [name]. */ fun register( - owner: Any, - state: Lifecycle.State, - onClearPublishedData: (IScope) -> Unit = { cleared += owner }, - ) { - navLeaseCoordinator.registerCandidate( - owner, - state, - onPublishData = { published += owner }, - onClearPublishedData = onClearPublishedData, - ) + name: String, + owner: TestOwner, + navState: LeasedNavState = navState(name), + ): NavLeaseRegistration { + owners += owner + return checkNotNull(navLeaseCoordinator.register(owner.lifecycle, scopes, navState)) } + /** + * Registers a candidate for [owner]'s lifecycle and applies its first back stack update, which + * acquires the nav lease if the owner's lifecycle permits it. + */ fun acquireLease( - owner: Any, - state: Lifecycle.State = Lifecycle.State.RESUMED, - transaction: ITransaction? = null, - onClearPublishedData: (IScope) -> Unit = { cleared += owner }, - ) { - register(owner, state, onClearPublishedData) - navLeaseCoordinator.onBackStackUpdate(owner, scope) - if (transaction != null) { - navLeaseCoordinator.retainTransaction(owner, transaction) - scope.transaction = transaction + name: String, + owner: TestOwner, + navState: LeasedNavState = navState(name), + ): NavLeaseRegistration = register(name, owner, navState).also { it.onBackStackUpdate() } + + /** Runs [block] while `configureScope` skips its callbacks, e.g., as if the SDK is disabled. */ + fun withoutScope(block: () -> Unit) { + providesScope = false + try { + block() + } finally { + providesScope = true } } @@ -870,4 +933,16 @@ class NavLeaseCoordinatorTest { fun create(): Fixture = Fixture() } } + + private class FakeLeasedNavState( + private val onPublishTo: (IScope) -> Unit, + private val onUnpublishFrom: (IScope) -> Unit, + private val onUnregistered: (IScope?) -> Unit, + ) : LeasedNavState { + override fun publishTo(scope: IScope) = onPublishTo(scope) + + override fun unpublishFrom(scope: IScope) = onUnpublishFrom(scope) + + override fun onUnregistered(scope: IScope?) = onUnregistered.invoke(scope) + } } diff --git a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopeTest.kt b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopeTest.kt index be66fb5b165..a079c91a545 100644 --- a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopeTest.kt +++ b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopeTest.kt @@ -1,26 +1,42 @@ package io.sentry.compose.navigation3.lifecycle import androidx.lifecycle.Lifecycle +import androidx.lifecycle.LifecycleOwner +import androidx.lifecycle.LifecycleRegistry import com.google.common.truth.Truth.assertThat +import io.sentry.IScopes import io.sentry.ITransaction import io.sentry.Scope +import io.sentry.ScopeCallback import io.sentry.ScopeObserverAdapter import io.sentry.SentryOptions +import io.sentry.SpanStatus +import io.sentry.TransactionContext +import io.sentry.TransactionOptions +import io.sentry.protocol.App import org.junit.Assert.assertThrows import org.junit.Test +import org.mockito.kotlin.any +import org.mockito.kotlin.doAnswer import org.mockito.kotlin.mock +import org.mockito.kotlin.never +import org.mockito.kotlin.verify import org.mockito.kotlin.whenever +/** + * Tests for the [INavScope] that [NavScopes.configureScope] hands out, which is only reachable + * through it. + */ class NavScopeTest { @Test fun `getScreen() - reads the real screen with the nav lease`() { val fixture = Fixture.create(ownsLease = true) - val navScope = fixture.navScope - navScope.screen = "Staged" - fixture.realScope.screen = "Host" + val navScopes = fixture.navScopes + navScopes.withNavScope { it.screen = "Staged" } + fixture.scope.screen = "Host" - val screen = navScope.screen + val screen = navScopes.withNavScope { it.screen } assertThat(screen).isEqualTo("Host") } @@ -28,11 +44,11 @@ class NavScopeTest { @Test fun `getScreen() - reads the staged screen without the nav lease`() { val fixture = Fixture.create() - val navScope = fixture.navScope - fixture.realScope.screen = "Host" - fixture.stagedNavData.stageScreen("Staged") + val navScopes = fixture.navScopes + fixture.scope.screen = "Host" + navScopes.withNavScope { it.screen = "Staged" } - val screen = navScope.screen + val screen = navScopes.withNavScope { it.screen } assertThat(screen).isEqualTo("Staged") } @@ -40,282 +56,388 @@ class NavScopeTest { @Test fun `setScreen() - publishes the screen with the nav lease`() { val fixture = Fixture.create(ownsLease = true) - val navScope = fixture.navScope + val navScopes = fixture.navScopes - navScope.screen = "Profile" + navScopes.withNavScope { it.screen = "Profile" } - assertThat(fixture.realScope.screen).isEqualTo("Profile") + assertThat(fixture.scope.screen).isEqualTo("Profile") } @Test - fun `setScreen() - stages the screen without the nav lease`() { + fun `setScreen() - stages the screen without the nav lease until the lease is acquired`() { val fixture = Fixture.create() - val navScope = fixture.navScope - fixture.realScope.screen = "Host" + val navScopes = fixture.navScopes + fixture.scope.screen = "Host" - navScope.screen = "Profile" + navScopes.withNavScope { it.screen = "Profile" } + assertThat(fixture.scope.screen).isEqualTo("Host") - assertThat(fixture.realScope.screen).isEqualTo("Host") - assertThat(navScope.screen).isEqualTo("Profile") + fixture.acquireLease() + assertThat(fixture.scope.screen).isEqualTo("Profile") + } + + @Test + fun `getApp() - returns a copy of the scope's app context`() { + val fixture = Fixture.create() + val navScopes = fixture.navScopes + val hostApp = App().apply { appName = "Host" } + fixture.scope.contexts.setApp(hostApp) + + val app = navScopes.withNavScope { it.app } + + assertThat(app?.appName).isEqualTo("Host") + assertThat(app).isNotSameInstanceAs(hostApp) + } + + @Test + fun `getApp() - returns null when the scope has no app context`() { + val fixture = Fixture.create() + val navScopes = fixture.navScopes + + val app = navScopes.withNavScope { it.app } + + assertThat(app).isNull() } @Test fun `setNavigationContext() - publishes the navigation context with the nav lease`() { val fixture = Fixture.create(ownsLease = true) - val navScope = fixture.navScope - val navigationContext = mapOf("route" to "Profile") + val navScopes = fixture.navScopes + + navScopes.withNavScope { it.setNavigationContext(navigationContext) } + + assertThat(fixture.scope.contexts[NAVIGATION_CONTEXT_KEY]).isSameInstanceAs(navigationContext) + } + + @Test + fun `setNavigationContext() - clears the published navigation context given null with the nav lease`() { + val fixture = Fixture.create(ownsLease = true) + val navScopes = fixture.navScopes + navScopes.withNavScope { it.setNavigationContext(navigationContext) } - navScope.setNavigationContext(navigationContext) + navScopes.withNavScope { it.setNavigationContext(null) } - assertThat(fixture.realScope.contexts[NAVIGATION_CONTEXT_KEY]) - .isSameInstanceAs(navigationContext) + assertThat(fixture.scope.contexts[NAVIGATION_CONTEXT_KEY]).isNull() } @Test - fun `setNavigationContext() - stages the navigation context without the nav lease`() { + fun `setNavigationContext() - stages the navigation context without the nav lease until the lease is acquired`() { val fixture = Fixture.create() - val navScope = fixture.navScope + val navScopes = fixture.navScopes val hostContext = mapOf("route" to "Host") - val navigationContext = mapOf("route" to "Profile") - fixture.realScope.setContexts(NAVIGATION_CONTEXT_KEY, hostContext) + fixture.scope.setContexts(NAVIGATION_CONTEXT_KEY, hostContext) - navScope.setNavigationContext(navigationContext) + navScopes.withNavScope { it.setNavigationContext(navigationContext) } + assertThat(fixture.scope.contexts[NAVIGATION_CONTEXT_KEY]).isSameInstanceAs(hostContext) - assertThat(fixture.realScope.contexts[NAVIGATION_CONTEXT_KEY]).isSameInstanceAs(hostContext) - fixture.stagedNavData.publishTo(fixture.realScope) - assertThat(fixture.realScope.contexts[NAVIGATION_CONTEXT_KEY]) - .isSameInstanceAs(navigationContext) + fixture.acquireLease() + assertThat(fixture.scope.contexts[NAVIGATION_CONTEXT_KEY]).isSameInstanceAs(navigationContext) } @Test - fun `rotatePropagationContext() - replaces the real context with the nav lease`() { + fun `rotatePropagationContext() - replaces the real propagation context with the nav lease`() { val fixture = Fixture.create(ownsLease = true) - val realContext = fixture.realScope.propagationContext + val navScopes = fixture.navScopes + val realContext = fixture.scope.propagationContext - fixture.navScope.rotatePropagationContext() + navScopes.withNavScope { it.rotatePropagationContext() } - assertThat(fixture.realScope.propagationContext).isNotSameInstanceAs(realContext) + assertThat(fixture.scope.propagationContext).isNotSameInstanceAs(realContext) } @Test - fun `rotatePropagationContext() - defers rotating the real context without the nav lease`() { + fun `rotatePropagationContext() - defers the rotation without the nav lease until the lease is acquired`() { val fixture = Fixture.create() - val realContext = fixture.realScope.propagationContext + val navScopes = fixture.navScopes + val realContext = fixture.scope.propagationContext - fixture.navScope.rotatePropagationContext() + navScopes.withNavScope { it.rotatePropagationContext() } + assertThat(fixture.scope.propagationContext).isSameInstanceAs(realContext) - assertThat(fixture.realScope.propagationContext).isSameInstanceAs(realContext) - fixture.stagedNavData.publishTo(fixture.realScope) - assertThat(fixture.realScope.propagationContext).isNotSameInstanceAs(realContext) + fixture.acquireLease() + assertThat(fixture.scope.propagationContext).isNotSameInstanceAs(realContext) } @Test - fun `bindTransaction() - binds to a free scope with the nav lease`() { + fun `startNavTransaction() - binds the nav transaction to a free scope with the nav lease`() { val fixture = Fixture.create(ownsLease = true) - val transaction = mock() + val navScopes = fixture.navScopes - fixture.navScope.bindTransaction(transaction) + val navTransaction = navScopes.withNavScope { it.startNavTransaction("/Profile") } - assertThat(fixture.realScope.transaction).isSameInstanceAs(transaction) + assertThat(navTransaction).isNotNull() + assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransactions.single()) } @Test - fun `bindTransaction() - preserves an already bound transaction`() { + fun `startNavTransaction() - starts none and keeps a bound host transaction with the nav lease`() { val fixture = Fixture.create(ownsLease = true) - val bound = mock() - fixture.realScope.transaction = bound + val navScopes = fixture.navScopes + val hostTransaction = mock() + fixture.scope.transaction = hostTransaction - fixture.navScope.bindTransaction(mock()) + val navTransaction = navScopes.withNavScope { it.startNavTransaction("/Profile") } - assertThat(fixture.realScope.transaction).isSameInstanceAs(bound) + assertThat(navTransaction).isNull() + assertThat(fixture.startedTransactions).isEmpty() + assertThat(fixture.scope.transaction).isSameInstanceAs(hostTransaction) } @Test - fun `bindTransaction() - does nothing without the nav lease`() { + fun `startNavTransaction() - replaces a finished bound transaction with the nav lease`() { + val fixture = Fixture.create(ownsLease = true) + val navScopes = fixture.navScopes + fixture.scope.transaction = + mock().also { whenever(it.isFinished).thenReturn(true) } + + val navTransaction = navScopes.withNavScope { it.startNavTransaction("/Profile") } + + assertThat(navTransaction).isNotNull() + assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransactions.single()) + } + + @Test + fun `startNavTransaction() - starts none without the nav lease`() { val fixture = Fixture.create() + val navScopes = fixture.navScopes - fixture.navScope.bindTransaction(mock()) + val navTransaction = navScopes.withNavScope { it.startNavTransaction("/Profile") } - assertThat(fixture.realScope.transaction).isNull() + assertThat(navTransaction).isNull() + assertThat(fixture.startedTransactions).isEmpty() + assertThat(fixture.scope.transaction).isNull() } @Test - fun `unbindTransaction() - unbinds the given transaction without the nav lease`() { + fun `startNavTransaction() - starts none without the nav lease, even once the lease is acquired`() { val fixture = Fixture.create() - val transaction = mock() - fixture.realScope.transaction = transaction + val navScopes = fixture.navScopes + navScopes.withNavScope { it.startNavTransaction("/Profile") } - fixture.navScope.unbindTransaction(transaction) + fixture.acquireLease() - assertThat(fixture.realScope.transaction).isNull() + assertThat(fixture.startedTransactions).isEmpty() + assertThat(fixture.scope.transaction).isNull() } @Test - fun `unbindTransaction() - releases the owner's retained nav transaction with the nav lease`() { - val fixture = Fixture.create(ownsLease = true) - val transaction = mock() - fixture.retainTransaction(transaction) + fun `startNavTransaction() - starts none and keeps another owner's bound nav transaction without the nav lease`() { + val fixture = Fixture.create() + val navScopes = fixture.navScopes + val otherTransaction = fixture.startAnotherOwnersNavTransaction() - fixture.navScope.unbindTransaction(transaction) + val navTransaction = navScopes.withNavScope { it.startNavTransaction("/Profile") } - assertThat(fixture.coordinator.isNavTransaction(transaction)).isFalse() + assertThat(navTransaction).isNull() + assertThat(fixture.startedTransactions).containsExactly(otherTransaction) + assertThat(fixture.scope.transaction).isSameInstanceAs(otherTransaction) } @Test - fun `unbindTransaction() - releases the owner's retained nav transaction without the nav lease`() { + fun `startNavTransaction() - starts none while its unfinished previous nav transaction is bound`() { val fixture = Fixture.create(ownsLease = true) - val transaction = mock() - fixture.retainTransaction(transaction) - fixture.giveLeaseToAnotherOwner() + val navScopes = fixture.navScopes + navScopes.withNavScope { it.startNavTransaction("/Profile") } + val previous = fixture.startedTransactions.single() - fixture.navScope.unbindTransaction(transaction) + val navTransaction = navScopes.withNavScope { it.startNavTransaction("/Settings") } - assertThat(fixture.coordinator.isNavTransaction(transaction)).isFalse() + assertThat(navTransaction).isNull() + verify(previous, never()).finish(any()) + assertThat(fixture.startedTransactions).containsExactly(previous) + assertThat(fixture.scope.transaction).isSameInstanceAs(previous) } @Test - fun `unbindTransaction() - releases the retained nav transaction when a scope observer throws`() { + fun `startNavTransaction() - binds the new nav transaction once the previous one is finished with the nav lease`() { val fixture = Fixture.create(ownsLease = true) - val transaction = mock() - fixture.retainTransaction(transaction) - fixture.realScope.transaction = transaction - // Registered last, so that only clearTransaction() reaches it. - fixture.realScope.options.addScopeObserver( - object : ScopeObserverAdapter() { - override fun setTransaction(transaction: String?) { - throw ThrowingScopeObserverException() - } - } - ) + val navScopes = fixture.navScopes + navScopes.withNavScope { it.startNavTransaction("/Profile") } + navScopes.withNavScope { it.finishNavTransaction() } - assertThrows(ThrowingScopeObserverException::class.java) { - fixture.navScope.unbindTransaction(transaction) - } + val navTransaction = navScopes.withNavScope { it.startNavTransaction("/Settings") } - assertThat(fixture.coordinator.isNavTransaction(transaction)).isFalse() + assertThat(navTransaction).isNotNull() + assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransactions.last()) } @Test - fun `unbindTransaction() - preserves a different bound transaction`() { + fun `startNavTransaction() - starts none while a host transaction is bound after the previous one finishes`() { val fixture = Fixture.create(ownsLease = true) - val bound = mock() - fixture.realScope.transaction = bound + val navScopes = fixture.navScopes + navScopes.withNavScope { it.startNavTransaction("/Profile") } + val previous = fixture.startedTransactions.single() + val hostTransaction = mock() + fixture.scope.transaction = hostTransaction + navScopes.withNavScope { it.finishNavTransaction() } - fixture.navScope.unbindTransaction(mock()) + val navTransaction = navScopes.withNavScope { it.startNavTransaction("/Settings") } - assertThat(fixture.realScope.transaction).isSameInstanceAs(bound) + assertThat(navTransaction).isNull() + verify(previous).finish(SpanStatus.OK) + assertThat(fixture.startedTransactions).containsExactly(previous) + assertThat(fixture.scope.transaction).isSameInstanceAs(hostTransaction) } @Test - fun `clearFinishedTransaction() - unbinds a finished transaction with the nav lease`() { + fun `finishNavTransaction() - finishes and unbinds the nav transaction`() { val fixture = Fixture.create(ownsLease = true) - fixture.realScope.transaction = - mock().also { whenever(it.isFinished).thenReturn(true) } + val navScopes = fixture.navScopes + navScopes.withNavScope { it.startNavTransaction("/Profile") } + val started = fixture.startedTransactions.single() - fixture.navScope.clearFinishedTransaction() + navScopes.withNavScope { it.finishNavTransaction() } - assertThat(fixture.realScope.transaction).isNull() + verify(started).finish(SpanStatus.OK) + assertThat(fixture.scope.transaction).isNull() } @Test - fun `clearFinishedTransaction() - does nothing without the nav lease`() { - val fixture = Fixture.create() - val finished = mock().also { whenever(it.isFinished).thenReturn(true) } - fixture.realScope.transaction = finished + fun `finishNavTransaction() - finishes it without the nav lease and keeps the leaseholder's bound`() { + val fixture = Fixture.create(ownsLease = true) + val navScopes = fixture.navScopes + navScopes.withNavScope { it.startNavTransaction("/Profile") } + val started = fixture.startedTransactions.single() + val otherTransaction = fixture.startAnotherOwnersNavTransaction() - fixture.navScope.clearFinishedTransaction() + navScopes.withNavScope { it.finishNavTransaction() } - assertThat(fixture.realScope.transaction).isSameInstanceAs(finished) + verify(started).finish(SpanStatus.OK) + assertThat(fixture.scope.transaction).isSameInstanceAs(otherTransaction) } @Test - fun `transaction - hides another owner's nav transaction without the nav lease`() { - val fixture = Fixture.create() - val other = Any() - val otherTransaction = mock() - fixture.coordinator.registerCandidate(other, Lifecycle.State.RESUMED, {}, {}) - fixture.coordinator.onBackStackUpdate(other, fixture.realScope) - fixture.coordinator.retainTransaction(other, otherTransaction) - fixture.realScope.transaction = otherTransaction + fun `finishNavTransaction() - forgets the nav transaction if unbinding throws`() { + val fixture = Fixture.create(ownsLease = true) + val navScopes = fixture.navScopes + navScopes.withNavScope { it.startNavTransaction("/Profile") } + val started = fixture.startedTransactions.single() + // Registered after the start, so that only unbinding the transaction reaches it. + fixture.scope.options.addScopeObserver( + object : ScopeObserverAdapter() { + override fun setTransaction(transaction: String?) { + throw ThrowingScopeObserverException() + } + } + ) - val transaction = fixture.navScope.transaction + assertThrows(ThrowingScopeObserverException::class.java) { + navScopes.withNavScope { it.finishNavTransaction() } + } + navScopes.withNavScope { it.finishNavTransaction() } - assertThat(transaction).isNull() + // Finished once: the second call has no nav transaction left to finish. + verify(started).finish(SpanStatus.OK) } @Test - fun `transaction - reports a non-nav transaction without the nav lease`() { - val fixture = Fixture.create() + fun `finishNavTransaction() - keeps a different bound transaction`() { + val fixture = Fixture.create(ownsLease = true) + val navScopes = fixture.navScopes + navScopes.withNavScope { it.startNavTransaction("/Profile") } val hostTransaction = mock() - fixture.realScope.transaction = hostTransaction + fixture.scope.transaction = hostTransaction - val transaction = fixture.navScope.transaction + navScopes.withNavScope { it.finishNavTransaction() } - assertThat(transaction).isSameInstanceAs(hostTransaction) + assertThat(fixture.scope.transaction).isSameInstanceAs(hostTransaction) } @Test - fun `transaction - reports the bound transaction with the nav lease`() { - val fixture = Fixture.create(ownsLease = true) - val boundTransaction = mock() - fixture.realScope.transaction = boundTransaction + fun `finishNavTransaction() - keeps the bound transaction when the nav transaction has not started`() { + val fixture = Fixture.create() + val navScopes = fixture.navScopes + val otherTransaction = fixture.startAnotherOwnersNavTransaction() + navScopes.withNavScope { it.startNavTransaction("/Profile") } + + navScopes.withNavScope { it.finishNavTransaction() } + + assertThat(fixture.scope.transaction).isSameInstanceAs(otherTransaction) + verify(otherTransaction, never()).finish(any()) + } - val transaction = fixture.navScope.transaction + private val navigationContext = mapOf("route" to "Profile") - assertThat(transaction).isSameInstanceAs(boundTransaction) + /** Runs [block] with the [INavScope] handed out by [NavScopes.configureScope]. */ + private fun NavScopes.withNavScope(block: (INavScope) -> T): T { + val results = mutableListOf() + configureScope { results += block(it) } + return results.single() } + private fun INavScope.startNavTransaction(name: String): INavTransaction? = + startNavTransaction(TransactionContext(name, "navigation"), TransactionOptions()) + /** Thrown by a scope observer, to stand in for one the host app registered. */ private class ThrowingScopeObserverException : RuntimeException() - private class Fixture - private constructor( - val realScope: Scope, - val stagedNavData: StagedNavData, - val coordinator: NavLeaseCoordinator, - val navScope: NavScope, - private val owner: Any, - ) { - - /** Retains [transaction] as the owner's nav transaction; the owner must hold the nav lease. */ - fun retainTransaction(transaction: ITransaction) { - coordinator.retainTransaction(owner, transaction) - check(coordinator.isNavTransaction(transaction)) + private class TestLifecycleOwner : LifecycleOwner { + override val lifecycle: LifecycleRegistry = + LifecycleRegistry.createUnsafe(this).apply { currentState = Lifecycle.State.RESUMED } + } + + private class Fixture private constructor() { + val scope = Scope(SentryOptions()) + val coordinator = NavLeaseCoordinator() + val startedTransactions = mutableListOf() + + /** + * `LifecycleRegistry` holds its owner weakly and throws once it's collected, so the fixture + * keeps the owners it creates alive for the test. + */ + private val lifecycleOwners = mutableListOf() + + private val scopes = + mock().also { scopes -> + whenever(scopes.options).thenReturn(SentryOptions()) + doAnswer { + (it.arguments[0] as ScopeCallback).run(scope) + null + } + .whenever(scopes) + .configureScope(any()) + whenever(scopes.startTransaction(any(), any())) + .thenAnswer { + val name = (it.arguments[0] as TransactionContext).name + mock() + .also { transaction -> whenever(transaction.name).thenReturn(name) } + .also(startedTransactions::add) + } + } + + /** Attached to a resumed owner, but without the nav lease until [acquireLease]. */ + val navScopes = resumedNavScopes() + + /** Lets [navScopes] acquire the nav lease by applying its first back stack update. */ + fun acquireLease() { + navScopes.onBackStackUpdate() } - /** Lets another resumed owner take the nav lease from the owner. */ - fun giveLeaseToAnotherOwner() { - val other = Any() - coordinator.registerCandidate(other, Lifecycle.State.RESUMED, {}, {}) - coordinator.onBackStackUpdate(other, realScope) - check(!coordinator.hasLease(owner)) + /** + * Lets another resumed owner acquire the nav lease and start a nav transaction, which is + * returned. + */ + fun startAnotherOwnersNavTransaction(): ITransaction { + val other = resumedNavScopes() + other.onBackStackUpdate() + other.configureScope { + it.startNavTransaction(TransactionContext("/Other", "navigation"), TransactionOptions()) + } + return startedTransactions.single { it.name == "/Other" } } - companion object { + private fun resumedNavScopes(): NavScopes = + NavScopes(scopes, coordinator).also { + it.attach(TestLifecycleOwner().also(lifecycleOwners::add).lifecycle) + } - fun create(ownsLease: Boolean = false): Fixture { - val realScope = Scope(SentryOptions.empty()) - val stagedNavData = StagedNavData() - val coordinator = NavLeaseCoordinator() - val owner = Any() - coordinator.registerCandidate( - owner, - Lifecycle.State.RESUMED, - stagedNavData::publishTo, - stagedNavData::clearPublishedStateFrom, - ) - if (ownsLease) { - coordinator.onBackStackUpdate(owner, realScope) + companion object { + fun create(ownsLease: Boolean = false): Fixture = + Fixture().also { + if (ownsLease) { + it.acquireLease() + } } - - return Fixture( - realScope, - stagedNavData, - coordinator, - NavScope(realScope, stagedNavData, coordinator, owner), - owner, - ) - } } } } diff --git a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopesTest.kt b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopesTest.kt index 4470072b354..3226cbe162d 100644 --- a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopesTest.kt +++ b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopesTest.kt @@ -6,25 +6,58 @@ import androidx.lifecycle.LifecycleRegistry import com.google.common.truth.Truth.assertThat import io.sentry.Breadcrumb import io.sentry.Hint +import io.sentry.ILogger import io.sentry.IScopes import io.sentry.ITransaction +import io.sentry.NoOpTransaction import io.sentry.Scope import io.sentry.ScopeCallback +import io.sentry.ScopeObserverAdapter +import io.sentry.SentryLevel import io.sentry.SentryOptions +import io.sentry.SpanStatus import io.sentry.TransactionContext import io.sentry.TransactionOptions +import io.sentry.protocol.Contexts +import org.junit.Assert.assertThrows import org.junit.Test import org.mockito.kotlin.any import org.mockito.kotlin.doAnswer +import org.mockito.kotlin.eq import org.mockito.kotlin.mock +import org.mockito.kotlin.never +import org.mockito.kotlin.times import org.mockito.kotlin.verify import org.mockito.kotlin.whenever class NavScopesTest { + @Test + fun `attach() - lets the first back stack update acquire the nav lease`() { + val fixture = Fixture.create() + val navScopes = fixture.navScopes() + navScopes.attach(fixture.lifecycle()) + + navScopes.applyFirstBackStackUpdate() + + assertThat(fixture.hasLease(navScopes)).isTrue() + } + + @Test + fun `attach() - does nothing for the lifecycle that's already attached`() { + val fixture = Fixture.create() + val navScopes = fixture.navScopes() + val lifecycle = fixture.lifecycle() + navScopes.attach(lifecycle) + + navScopes.attach(lifecycle) + + assertThat(lifecycle.observerCount).isEqualTo(1) + } + @Test fun `attach() - ignores a second, different lifecycle`() { - val fixture = Fixture() + val fixture = Fixture.create() val navScopes = fixture.navScopes() val first = TestLifecycleOwner() val second = TestLifecycleOwner() @@ -37,45 +70,79 @@ class NavScopesTest { } @Test - fun `lifecycle callbacks are recorded while scope callbacks are skipped`() { - val fixture = Fixture() - val outgoing = fixture.navScopes() - val outgoingOwner = TestLifecycleOwner() - outgoing.attach(outgoingOwner.lifecycle) - fixture.isEnabled = false - outgoingOwner.lifecycle.currentState = Lifecycle.State.RESUMED - fixture.isEnabled = true - outgoing.applyFirstBackStackUpdate() - - val incoming = fixture.navScopes() - val incomingOwner = TestLifecycleOwner(Lifecycle.State.RESUMED) - incoming.attach(incomingOwner.lifecycle) - incoming.applyFirstBackStackUpdate() - assertThat(fixture.hasLease(incoming)).isTrue() + fun `attach() - ignores a destroyed lifecycle`() { + val fixture = Fixture.create() + val navScopes = fixture.navScopes() + val lifecycle = fixture.lifecycle(Lifecycle.State.DESTROYED) - incoming.dispose() + navScopes.attach(lifecycle) + navScopes.applyFirstBackStackUpdate() - assertThat(fixture.hasLease(outgoing)).isTrue() + assertThat(fixture.hasLease(navScopes)).isFalse() } @Test - fun `onBackStackUpdate() - an update skipped by the scope does not count as the first update`() { - val fixture = Fixture() + fun `attach() - accepts a new lifecycle after the attached one is destroyed`() { + val fixture = Fixture.create() val navScopes = fixture.navScopes() - val owner = TestLifecycleOwner() - navScopes.attach(owner.lifecycle) - fixture.isEnabled = false + val first = fixture.lifecycle(Lifecycle.State.RESUMED) + navScopes.attach(first) navScopes.applyFirstBackStackUpdate() - fixture.isEnabled = true + first.currentState = Lifecycle.State.DESTROYED + val second = fixture.lifecycle(Lifecycle.State.RESUMED) + navScopes.attach(second) navScopes.applyFirstBackStackUpdate() + assertThat(second.observerCount).isEqualTo(1) assertThat(fixture.hasLease(navScopes)).isTrue() } + @Test + fun `onBackStackUpdate() - doesn't acquire the nav lease before attach`() { + val fixture = Fixture.create() + val navScopes = fixture.navScopes() + + navScopes.applyFirstBackStackUpdate() + + assertThat(fixture.hasLease(navScopes)).isFalse() + } + + @Test + fun `destroying the attached lifecycle finishes the nav transaction`() { + val fixture = Fixture.create() + val navScopes = fixture.navScopes() + val lifecycle = fixture.lifecycle(Lifecycle.State.RESUMED) + navScopes.attach(lifecycle) + navScopes.applyFirstBackStackUpdate() + navScopes.startNavTransaction("/Home") + val started = fixture.startedTransaction("/Home") + + lifecycle.currentState = Lifecycle.State.DESTROYED + + verify(started).finish(SpanStatus.OK) + assertThat(fixture.scope.transaction).isNull() + } + + @Test + fun `destroying the attached lifecycle without a scope doesn't finish the nav transaction`() { + val fixture = Fixture.create() + val navScopes = fixture.navScopes() + val lifecycle = fixture.lifecycle(Lifecycle.State.RESUMED) + navScopes.attach(lifecycle) + navScopes.applyFirstBackStackUpdate() + navScopes.startNavTransaction("/Home") + val started = fixture.startedTransaction("/Home") + fixture.isEnabled = false + + lifecycle.currentState = Lifecycle.State.DESTROYED + + verify(started, never()).finish(any()) + } + @Test fun `addBreadcrumb() - records breadcrumbs immediately without the nav lease`() { - val fixture = Fixture() + val fixture = Fixture.create() val leaseholder = fixture.navScopes() leaseholder.attach(TestLifecycleOwner().lifecycle) leaseholder.applyFirstBackStackUpdate() @@ -91,75 +158,273 @@ class NavScopesTest { } @Test - fun `startTransaction() - returns a no-op transaction without the nav lease`() { - val fixture = Fixture() - val covered = fixture.navScopes() - covered.attach(TestLifecycleOwner(Lifecycle.State.RESUMED).lifecycle) - covered.applyFirstBackStackUpdate() - val incoming = fixture.navScopes() - incoming.attach(TestLifecycleOwner(Lifecycle.State.RESUMED).lifecycle) - incoming.applyFirstBackStackUpdate() + fun `dispose() - finishes a displaced owner's nav transaction and keeps the leaseholder's bound`() { + val fixture = Fixture.create() + val covered = fixture.coveredNavScopes(startedRoute = "/Home") + fixture.incoming.startNavTransaction("/Incoming") + val coveredTransaction = fixture.startedTransaction("/Home") + val incomingTransaction = fixture.startedTransaction("/Incoming") + + covered.dispose() + + verify(coveredTransaction).finish(SpanStatus.OK) + assertThat(fixture.scope.transaction).isSameInstanceAs(incomingTransaction) + } + + @Test + fun `dispose() - finishes the nav transaction when handing off the nav lease throws`() { + val fixture = Fixture.create() + val covered = fixture.coveredNavScopes() + covered.configureScope { it.screen = "/Result" } + fixture.incoming.startNavTransaction("/Incoming") + val incomingTransaction = fixture.startedTransaction("/Incoming") + val failure = IllegalStateException("observer failure") + fixture.scope.options.addScopeObserver( + object : ScopeObserverAdapter() { + override fun setContexts(contexts: Contexts) { + throw failure + } + } + ) + + val thrown = assertThrows(IllegalStateException::class.java) { fixture.incoming.dispose() } + + assertThat(thrown).isSameInstanceAs(failure) + verify(incomingTransaction).finish(SpanStatus.OK) + } + + @Test + fun `dispose() - doesn't finish the nav transaction without a scope`() { + val fixture = Fixture.create() + val navScopes = fixture.navScopes() + navScopes.attach(fixture.lifecycle(Lifecycle.State.RESUMED)) + navScopes.applyFirstBackStackUpdate() + navScopes.startNavTransaction("/Home") + val started = fixture.startedTransaction("/Home") + fixture.isEnabled = false + + navScopes.dispose() + + verify(started, never()).finish(any()) + } + + @Test + fun `startNavTransaction() - starts and binds a nav transaction with the nav lease`() { + val fixture = Fixture.create() + val navScopes = fixture.navScopes() + navScopes.attach(fixture.lifecycle(Lifecycle.State.RESUMED)) + navScopes.applyFirstBackStackUpdate() + + val transaction = navScopes.startNavTransaction("/Home") + + val started = fixture.startedTransaction("/Home") + assertThat(transaction).isNotNull() + assertThat(fixture.scope.transaction).isSameInstanceAs(started) + } + + @Test + fun `startNavTransaction() - returns null when the delegate declines with the nav lease`() { + val fixture = Fixture.create(declinesTransactions = true) + val navScopes = fixture.navScopes() + navScopes.attach(fixture.lifecycle(Lifecycle.State.RESUMED)) + navScopes.applyFirstBackStackUpdate() + + val transaction = navScopes.startNavTransaction("/Home") + + assertThat(transaction).isNull() + } + + @Test + fun `startNavTransaction() - logs the route when another transaction blocks the start`() { + val fixture = Fixture.create() + val navScopes = fixture.navScopes() + navScopes.attach(fixture.lifecycle(Lifecycle.State.RESUMED)) + navScopes.applyFirstBackStackUpdate() + fixture.scope.transaction = mock() + + navScopes.startNavTransaction("/Home") + + verify(fixture.logger).log(eq(SentryLevel.DEBUG), any(), eq("/Home")) + } + + @Test + fun `startNavTransaction() - starts no nav transaction without the nav lease`() { + val fixture = Fixture.create() + val covered = fixture.coveredNavScopes() - val transaction = - covered.startTransaction(TransactionContext("name", "op"), TransactionOptions()) + val transaction = covered.startNavTransaction("/Result") assertThat(fixture.hasLease(covered)).isFalse() - assertThat(transaction.isNoOp).isTrue() + assertThat(transaction).isNull() + assertThat(fixture.startedTransactions).isEmpty() assertThat(fixture.scope.transaction).isNull() } @Test - fun `startTransaction() - declining without the nav lease defers a propagation context rotation`() { - val fixture = Fixture() - val covered = fixture.navScopes() - covered.attach(TestLifecycleOwner(Lifecycle.State.RESUMED).lifecycle) - covered.applyFirstBackStackUpdate() - val incoming = fixture.navScopes() - incoming.attach(TestLifecycleOwner(Lifecycle.State.RESUMED).lifecycle) - incoming.applyFirstBackStackUpdate() + fun `startNavTransaction() - defers the propagation context rotation without the nav lease`() { + val fixture = Fixture.create() + val covered = fixture.coveredNavScopes() val incomingContext = fixture.scope.propagationContext - covered.startTransaction(TransactionContext("name", "op"), TransactionOptions()) + covered.startNavTransaction("/Result") assertThat(fixture.scope.propagationContext).isSameInstanceAs(incomingContext) - incoming.dispose() + fixture.incoming.dispose() assertThat(fixture.hasLease(covered)).isTrue() assertThat(fixture.scope.propagationContext).isNotSameInstanceAs(incomingContext) } @Test - fun `startTransaction() - returns a no-op transaction after dispose`() { - val fixture = Fixture() + fun `startNavTransaction() - starts no nav transaction after dispose`() { + val fixture = Fixture.create() val navScopes = fixture.navScopes() - navScopes.attach(TestLifecycleOwner().lifecycle) + navScopes.attach(fixture.lifecycle()) navScopes.applyFirstBackStackUpdate() navScopes.dispose() - val transaction = - navScopes.startTransaction(TransactionContext("name", "op"), TransactionOptions()) + navScopes.startNavTransaction("/Home") - assertThat(transaction.isNoOp).isTrue() + assertThat(fixture.startedTransactions).isEmpty() } @Test - fun `rotatePropagationContext() - a rotation requested without the nav lease happens once the lease is acquired`() { - val fixture = Fixture() - val covered = fixture.navScopes() - covered.attach(TestLifecycleOwner(Lifecycle.State.RESUMED).lifecycle) - covered.applyFirstBackStackUpdate() - val incoming = fixture.navScopes() - incoming.attach(TestLifecycleOwner(Lifecycle.State.RESUMED).lifecycle) - incoming.applyFirstBackStackUpdate() - val incomingContext = fixture.scope.propagationContext + fun `finishNavTransaction() - finishes the started nav transaction`() { + val fixture = Fixture.create() + val navScopes = fixture.navScopes() + navScopes.attach(fixture.lifecycle(Lifecycle.State.RESUMED)) + navScopes.applyFirstBackStackUpdate() + navScopes.startNavTransaction("/Home") + val started = fixture.startedTransaction("/Home") - covered.configureScope { it.rotatePropagationContext() } - assertThat(fixture.scope.propagationContext).isSameInstanceAs(incomingContext) + navScopes.finishNavTransaction() - incoming.dispose() + verify(started).finish(SpanStatus.OK) + } + + @Test + fun `finishNavTransaction() - finishes the started nav transaction only once`() { + val fixture = Fixture.create() + val navScopes = fixture.navScopes() + navScopes.attach(fixture.lifecycle(Lifecycle.State.RESUMED)) + navScopes.applyFirstBackStackUpdate() + navScopes.startNavTransaction("/Home") + val started = fixture.startedTransaction("/Home") + navScopes.finishNavTransaction() + + navScopes.finishNavTransaction() + + verify(started, times(1)).finish(any()) + } + + @Test + fun `finishNavTransaction() - does nothing without a nav transaction`() { + val fixture = Fixture.create() + val navScopes = fixture.navScopes() + navScopes.attach(fixture.lifecycle(Lifecycle.State.RESUMED)) + navScopes.applyFirstBackStackUpdate() + val hostTransaction = mock() + fixture.scope.transaction = hostTransaction + + navScopes.finishNavTransaction() + + verify(hostTransaction, never()).finish(any()) + assertThat(fixture.scope.transaction).isSameInstanceAs(hostTransaction) + } + + @Test + fun `a nav transaction starts and is bound after the previous one is finished`() { + val fixture = Fixture.create() + val navScopes = fixture.navScopes() + navScopes.attach(fixture.lifecycle(Lifecycle.State.RESUMED)) + navScopes.applyFirstBackStackUpdate() + navScopes.startNavTransaction("/Home") + navScopes.finishNavTransaction() + + val transaction = navScopes.startNavTransaction("/Profile") + + assertThat(transaction).isNotNull() + assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransaction("/Profile")) + } + + @Test + fun `losing the nav lease unbinds the nav transaction without finishing it`() { + val fixture = Fixture.create() + + fixture.coveredNavScopes(startedRoute = "/Home") + + val started = fixture.startedTransaction("/Home") + assertThat(fixture.scope.transaction).isNull() + verify(started, never()).finish(any()) + } + + @Test + fun `regaining the nav lease rebinds the unfinished nav transaction`() { + val fixture = Fixture.create() + fixture.coveredNavScopes(startedRoute = "/Home") + + fixture.incoming.dispose() + + assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransaction("/Home")) + } + + @Test + fun `regaining the nav lease republishes the nav data before rebinding the nav transaction`() { + val fixture = Fixture.create() + val covered = fixture.coveredNavScopes(startedRoute = "/Home") + covered.configureScope { it.screen = "Home" } + val screensAtBinding = mutableListOf() + fixture.scope.options.addScopeObserver( + object : ScopeObserverAdapter() { + override fun setTransaction(transaction: String?) { + if (transaction == "/Home") screensAtBinding += fixture.scope.screen + } + } + ) + + fixture.incoming.dispose() + + assertThat(screensAtBinding).containsExactly("Home") + } + + @Test + fun `a finished nav transaction is discarded when the nav lease is regained`() { + val fixture = Fixture.create() + val covered = fixture.coveredNavScopes(startedRoute = "/Home") + val started = fixture.startedTransaction("/Home") + whenever(started.isFinished).thenReturn(true) + + fixture.incoming.dispose() assertThat(fixture.hasLease(covered)).isTrue() - assertThat(fixture.scope.propagationContext).isNotSameInstanceAs(incomingContext) + assertThat(fixture.scope.transaction).isNull() + } + + @Test + fun `a nav transaction finished without the nav lease is not bound when the nav lease is regained`() { + val fixture = Fixture.create() + val covered = fixture.coveredNavScopes(startedRoute = "/Home") + // The mock keeps reporting itself unfinished, like a transaction waiting for its child spans. + covered.finishNavTransaction() + + fixture.incoming.dispose() + + assertThat(fixture.hasLease(covered)).isTrue() + assertThat(fixture.scope.transaction).isNull() + } + + /** Starts a nav transaction for [name] through the [INavScope] handed out by [NavScopes]. */ + private fun NavScopes.startNavTransaction(name: String): INavTransaction? { + var transaction: INavTransaction? = null + configureScope { + transaction = it.startNavTransaction(TransactionContext(name, "op"), TransactionOptions()) + } + return transaction + } + + /** Finishes the nav transaction through the [INavScope] handed out by [NavScopes]. */ + private fun NavScopes.finishNavTransaction() { + configureScope { it.finishNavTransaction() } } private fun NavScopes.applyFirstBackStackUpdate() { @@ -169,16 +434,27 @@ class NavScopesTest { private class TestLifecycleOwner(initialState: Lifecycle.State = Lifecycle.State.CREATED) : LifecycleOwner { override val lifecycle: LifecycleRegistry = - LifecycleRegistry.createUnsafe(this).apply { currentState = initialState } + LifecycleRegistry.createUnsafe(this).apply { + // A lifecycle must be created before it can be destroyed. + currentState = Lifecycle.State.CREATED + currentState = initialState + } } - private class Fixture { + private class Fixture private constructor() { var isEnabled = true val scope = Scope(SentryOptions()) val coordinator = NavLeaseCoordinator() + val logger = mock() val scopes = mock().also { scopes -> - whenever(scopes.options).thenReturn(SentryOptions()) + whenever(scopes.options) + .thenReturn( + SentryOptions().apply { + setLogger(this@Fixture.logger) + isDebug = true + } + ) doAnswer { if (isEnabled) { (it.arguments[0] as ScopeCallback).run(scope) @@ -188,11 +464,61 @@ class NavScopesTest { .whenever(scopes) .configureScope(any()) whenever(scopes.startTransaction(any(), any())) - .thenAnswer { mock() } + .thenAnswer { + if (declinesTransactions) { + NoOpTransaction.getInstance() + } else { + val name = (it.arguments[0] as TransactionContext).name + mock() + .also { transaction -> whenever(transaction.name).thenReturn(name) } + .also(startedTransactions::add) + } + } } + /** Whether [scopes] declines to start transactions, returning a no-op transaction instead. */ + var declinesTransactions = false + val startedTransactions = mutableListOf() + + /** The resumed leaseholder that covers the [NavScopes] returned by [coveredNavScopes]. */ + lateinit var incoming: NavScopes + + /** + * `LifecycleRegistry` holds its owner weakly and throws once it's collected, so the fixture + * keeps the owners created by [lifecycle] alive for the test. + */ + private val lifecycleOwners = mutableListOf() + fun navScopes(): NavScopes = NavScopes(scopes, coordinator) + fun lifecycle(state: Lifecycle.State = Lifecycle.State.CREATED): LifecycleRegistry = + TestLifecycleOwner(state).also(lifecycleOwners::add).lifecycle + + fun startedTransaction(name: String): ITransaction = startedTransactions.single { + it.name == name + } + + /** + * Returns a resumed [NavScopes] that applied its first back stack update, started a nav + * transaction for [startedRoute] if given, and then lost the nav lease to the resumed + * [incoming] one. It regains the lease when [incoming] is disposed. + */ + fun coveredNavScopes(startedRoute: String? = null): NavScopes { + val covered = navScopes() + covered.attach(lifecycle(Lifecycle.State.RESUMED)) + covered.onBackStackUpdate() + if (startedRoute != null) { + covered.configureScope { + it.startNavTransaction(TransactionContext(startedRoute, "op"), TransactionOptions()) + } + } + incoming = navScopes() + incoming.attach(lifecycle(Lifecycle.State.RESUMED)) + incoming.onBackStackUpdate() + check(!hasLease(covered)) + return covered + } + private var leaseProbeCount = 0 /** @@ -204,5 +530,13 @@ class NavScopesTest { navScopes.configureScope { it.screen = probe } return scope.screen == probe } + + companion object { + /** + * [declinesTransactions] can also be changed later, e.g., once a transaction was deferred. + */ + fun create(declinesTransactions: Boolean = false): Fixture = + Fixture().also { it.declinesTransactions = declinesTransactions } + } } } diff --git a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavTransactionTest.kt b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavTransactionTest.kt new file mode 100644 index 00000000000..2977db6e5b0 --- /dev/null +++ b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavTransactionTest.kt @@ -0,0 +1,125 @@ +package io.sentry.compose.navigation3.lifecycle + +import com.google.common.truth.Truth.assertThat +import io.sentry.ITransaction +import io.sentry.SpanStatus +import io.sentry.protocol.App +import io.sentry.protocol.Contexts +import org.junit.Test +import org.mockito.kotlin.any +import org.mockito.kotlin.inOrder +import org.mockito.kotlin.mock +import org.mockito.kotlin.never +import org.mockito.kotlin.verify +import org.mockito.kotlin.whenever + +class NavTransactionTest { + + private val app = App().apply { viewNames = listOf("/Checkout") } + private val navigationContext = mapOf("Back Stack" to listOf(mapOf("entry" to "/Checkout"))) + + @Test + fun `started() - is named after the transaction`() { + val fixture = Fixture.create() + + val handle = NavTransaction.started(fixture.transaction) + + assertThat(handle.name).isEqualTo("/Checkout") + } + + @Test + fun `transaction - is the backing transaction for a started handle`() { + val fixture = Fixture.create() + + val handle = NavTransaction.started(fixture.transaction) + + assertThat(handle.transaction).isSameInstanceAs(fixture.transaction) + } + + @Test + fun `a started handle sets data, contexts, and the app context on the transaction`() { + val fixture = Fixture.create() + val handle = NavTransaction.started(fixture.transaction) + + handle.setData("arguments", "id=1") + handle.setContext(NAVIGATION_CONTEXT_KEY, navigationContext) + handle.setApp(app) + + verify(fixture.transaction).setData("arguments", "id=1") + verify(fixture.transaction).setContext(NAVIGATION_CONTEXT_KEY, navigationContext) + assertThat(fixture.transaction.contexts.app).isSameInstanceAs(app) + } + + @Test + fun `finish() - finishes a started handle's transaction with the transaction's status`() { + val fixture = Fixture.create() + whenever(fixture.transaction.status).thenReturn(SpanStatus.CANCELLED) + val handle = NavTransaction.started(fixture.transaction) + + handle.finish(onFinished = {}) + + verify(fixture.transaction).finish(SpanStatus.CANCELLED) + } + + @Test + fun `finish() - finishes a started handle's transaction with OK when the transaction has no status`() { + val fixture = Fixture.create() + val handle = NavTransaction.started(fixture.transaction) + + handle.finish(onFinished = {}) + + verify(fixture.transaction).finish(SpanStatus.OK) + } + + @Test + fun `finish() - passes a started handle's transaction to the callback after finishing it`() { + val fixture = Fixture.create() + val onFinished = mock<(ITransaction) -> Unit>() + val handle = NavTransaction.started(fixture.transaction) + + handle.finish(onFinished) + + inOrder(fixture.transaction, onFinished) { + verify(fixture.transaction).finish(SpanStatus.OK) + verify(onFinished).invoke(fixture.transaction) + } + } + + @Test + fun `finish() - doesn't finish a started handle's transaction that has already finished`() { + val fixture = Fixture.create() + whenever(fixture.transaction.isFinished).thenReturn(true) + val handle = NavTransaction.started(fixture.transaction) + + handle.finish(onFinished = {}) + + verify(fixture.transaction, never()).finish(any()) + } + + @Test + fun `finish() - passes a started handle's already finished transaction to the callback`() { + val fixture = Fixture.create() + val onFinished = mock<(ITransaction) -> Unit>() + whenever(fixture.transaction.isFinished).thenReturn(true) + val handle = NavTransaction.started(fixture.transaction) + + handle.finish(onFinished) + + verify(onFinished).invoke(fixture.transaction) + } + + private class Fixture private constructor(val transaction: ITransaction) { + + companion object { + fun create(): Fixture { + val contexts = Contexts() + val transaction = + mock().also { + whenever(it.name).thenReturn("/Checkout") + whenever(it.contexts).thenReturn(contexts) + } + return Fixture(transaction) + } + } + } +} diff --git a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavTransactionTest.kt b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavTransactionTest.kt new file mode 100644 index 00000000000..8827eba4387 --- /dev/null +++ b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavTransactionTest.kt @@ -0,0 +1,398 @@ +package io.sentry.compose.navigation3.lifecycle + +import com.google.common.truth.Truth.assertThat +import io.sentry.ILogger +import io.sentry.IScopes +import io.sentry.ITransaction +import io.sentry.NoOpTransaction +import io.sentry.Scope +import io.sentry.ScopeObserverAdapter +import io.sentry.SentryLevel +import io.sentry.SentryOptions +import io.sentry.SpanStatus +import io.sentry.TransactionContext +import io.sentry.TransactionOptions +import org.junit.Assert.assertThrows +import org.junit.Test +import org.mockito.kotlin.any +import org.mockito.kotlin.eq +import org.mockito.kotlin.mock +import org.mockito.kotlin.never +import org.mockito.kotlin.verify +import org.mockito.kotlin.whenever + +@Suppress("LargeClass") +class StagedNavTransactionTest { + + private val transactionContext = TransactionContext("/Profile", "navigation") + private val transactionOptions = TransactionOptions() + + private val finishedTransaction = + mock().also { whenever(it.isFinished).thenReturn(true) } + private val hostTransaction = mock() + + @Test + fun `start() - starts a nav transaction and binds it to a free scope`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + + val navTransaction = + stagedNavTransaction.start(fixture.scope, transactionContext, transactionOptions) + + val started = fixture.startedTransaction("/Profile") + verify(fixture.scopes).startTransaction(transactionContext, transactionOptions) + assertThat(navTransaction?.transaction).isSameInstanceAs(started) + assertThat(fixture.scope.transaction).isSameInstanceAs(started) + } + + @Test + fun `start() - starts no nav transaction while another transaction is bound`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + fixture.scope.transaction = hostTransaction + + val navTransaction = + stagedNavTransaction.start(fixture.scope, transactionContext, transactionOptions) + + assertThat(navTransaction).isNull() + assertThat(fixture.startedTransactions).isEmpty() + assertThat(fixture.scope.transaction).isSameInstanceAs(hostTransaction) + } + + @Test + fun `start() - logs the route when another transaction blocks the start`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + fixture.scope.transaction = hostTransaction + + stagedNavTransaction.start(fixture.scope, transactionContext, transactionOptions) + + verify(fixture.logger).log(eq(SentryLevel.DEBUG), any(), eq("/Profile")) + } + + @Test + fun `start() - binds the nav transaction in place of a finished bound transaction`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + fixture.scope.transaction = finishedTransaction + + val navTransaction = + stagedNavTransaction.start(fixture.scope, transactionContext, transactionOptions) + + val started = fixture.startedTransaction("/Profile") + assertThat(navTransaction?.transaction).isSameInstanceAs(started) + assertThat(fixture.scope.transaction).isSameInstanceAs(started) + } + + @Test + fun `start() - returns null and keeps the current nav transaction when the start is declined`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + val current = fixture.start("/Home") + fixture.declinesTransactions = true + + val navTransaction = + stagedNavTransaction.start(fixture.scope, transactionContext, transactionOptions) + + assertThat(navTransaction).isNull() + assertThat(fixture.scope.transaction).isSameInstanceAs(current) + stagedNavTransaction.finish(fixture.scope) + verify(current).finish(SpanStatus.OK) + } + + @Test + fun `start() - replaces the current nav transaction without finishing it`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + val current = fixture.start("/Home") + // Unbound, so that it doesn't block the start. + stagedNavTransaction.unpublishFrom(fixture.scope) + + stagedNavTransaction.start(fixture.scope, transactionContext, transactionOptions) + verify(current, never()).finish(any()) + + stagedNavTransaction.finish(fixture.scope) + verify(fixture.startedTransaction("/Profile")).finish(SpanStatus.OK) + verify(current, never()).finish(any()) + } + + @Test + fun `start() - keeps the started nav transaction when binding it throws`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + fixture.transactionObserverThrows = true + + assertThrows(ThrowingScopeObserverException::class.java) { + stagedNavTransaction.start(fixture.scope, transactionContext, transactionOptions) + } + fixture.transactionObserverThrows = false + val started = fixture.startedTransaction("/Profile") + stagedNavTransaction.finish(fixture.scope) + + verify(started).finish(SpanStatus.OK) + } + + @Test + fun `publishTo() - binds an unpublished unfinished nav transaction again`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + val started = fixture.start() + stagedNavTransaction.unpublishFrom(fixture.scope) + + stagedNavTransaction.publishTo(fixture.scope) + + assertThat(fixture.scope.transaction).isSameInstanceAs(started) + assertThat(fixture.startedTransactions).containsExactly(started) + } + + @Test + fun `publishTo() - keeps the bound nav transaction bound`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + val started = fixture.start() + + stagedNavTransaction.publishTo(fixture.scope) + + assertThat(fixture.scope.transaction).isSameInstanceAs(started) + verify(fixture.logger, never()).log(eq(SentryLevel.DEBUG), any(), any()) + } + + @Test + fun `publishTo() - binds an unpublished nav transaction in place of a finished bound transaction`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + val started = fixture.start() + stagedNavTransaction.unpublishFrom(fixture.scope) + fixture.scope.transaction = finishedTransaction + + stagedNavTransaction.publishTo(fixture.scope) + + assertThat(fixture.scope.transaction).isSameInstanceAs(started) + } + + @Test + fun `publishTo() - doesn't bind an unpublished nav transaction while another transaction is bound`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + val started = fixture.start() + stagedNavTransaction.unpublishFrom(fixture.scope) + fixture.scope.transaction = hostTransaction + + stagedNavTransaction.publishTo(fixture.scope) + + assertThat(fixture.scope.transaction).isSameInstanceAs(hostTransaction) + stagedNavTransaction.finish(fixture.scope) + verify(started).finish(SpanStatus.OK) + } + + @Test + fun `publishTo() - forgets and unbinds a bound nav transaction that has finished`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + val started = fixture.start() + whenever(started.isFinished).thenReturn(true) + + stagedNavTransaction.publishTo(fixture.scope) + + assertThat(fixture.scope.transaction).isNull() + } + + @Test + fun `publishTo() - forgets an unpublished nav transaction that has finished and keeps another transaction`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + val started = fixture.start() + stagedNavTransaction.unpublishFrom(fixture.scope) + whenever(started.isFinished).thenReturn(true) + fixture.scope.transaction = hostTransaction + + stagedNavTransaction.publishTo(fixture.scope) + + assertThat(fixture.scope.transaction).isSameInstanceAs(hostTransaction) + } + + @Test + fun `publishTo() - leaves the scope untouched without a nav transaction`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + fixture.scope.transaction = finishedTransaction + + stagedNavTransaction.publishTo(fixture.scope) + + assertThat(fixture.scope.transaction).isSameInstanceAs(finishedTransaction) + } + + @Test + fun `unpublishFrom() - unbinds the started nav transaction without finishing it`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + val started = fixture.start() + + stagedNavTransaction.unpublishFrom(fixture.scope) + + assertThat(fixture.scope.transaction).isNull() + verify(started, never()).finish(any()) + } + + @Test + fun `unpublishFrom() - keeps another bound transaction`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + fixture.start() + stagedNavTransaction.unpublishFrom(fixture.scope) + fixture.scope.transaction = hostTransaction + + stagedNavTransaction.unpublishFrom(fixture.scope) + + assertThat(fixture.scope.transaction).isSameInstanceAs(hostTransaction) + } + + @Test + fun `finish() - finishes the started nav transaction and unbinds it`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + val started = fixture.start() + + stagedNavTransaction.finish(fixture.scope) + + verify(started).finish(SpanStatus.OK) + assertThat(fixture.scope.transaction).isNull() + } + + @Test + fun `finish() - finishes an unpublished nav transaction and keeps another bound transaction`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + val started = fixture.start() + stagedNavTransaction.unpublishFrom(fixture.scope) + fixture.scope.transaction = hostTransaction + + stagedNavTransaction.finish(fixture.scope) + + verify(started).finish(SpanStatus.OK) + assertThat(fixture.scope.transaction).isSameInstanceAs(hostTransaction) + } + + @Test + fun `finish() - forgets the nav transaction so that publishing never binds it again`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + fixture.start() + stagedNavTransaction.finish(fixture.scope) + + stagedNavTransaction.publishTo(fixture.scope) + + assertThat(fixture.scope.transaction).isNull() + } + + @Test + fun `finish() - forgets the nav transaction when unbinding it throws`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + val started = fixture.start() + fixture.transactionObserverThrows = true + + assertThrows(ThrowingScopeObserverException::class.java) { + stagedNavTransaction.finish(fixture.scope) + } + stagedNavTransaction.finish(fixture.scope) + + // Finished once: the second call has no nav transaction left to finish or unbind. + verify(started).finish(SpanStatus.OK) + } + + @Test + fun `finish() - leaves the scope untouched without a nav transaction`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + fixture.scope.transaction = hostTransaction + + stagedNavTransaction.finish(fixture.scope) + + verify(hostTransaction, never()).finish(any()) + assertThat(fixture.scope.transaction).isSameInstanceAs(hostTransaction) + } + + @Test + fun `forget() - forgets the started nav transaction without finishing or unbinding it`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + val started = fixture.start() + + stagedNavTransaction.forget() + stagedNavTransaction.finish(fixture.scope) + + assertThat(fixture.scope.transaction).isSameInstanceAs(started) + verify(started, never()).finish(any()) + } + + /** Thrown by a scope observer, to stand in for one the host app registered. */ + private class ThrowingScopeObserverException : RuntimeException() + + private class Fixture private constructor() { + val scope = Scope(SentryOptions()) + val logger = mock() + + /** Whether [scopes] declines to start transactions, returning a no-op transaction instead. */ + var declinesTransactions = false + val startedTransactions = mutableListOf() + + /** Whether a scope observer throws when the scope's transaction is bound or unbound. */ + var transactionObserverThrows = false + + val scopes = + mock().also { scopes -> + whenever(scopes.options) + .thenReturn( + SentryOptions().apply { + setLogger(this@Fixture.logger) + isDebug = true + } + ) + whenever(scopes.startTransaction(any(), any())) + .thenAnswer { + if (declinesTransactions) { + NoOpTransaction.getInstance() + } else { + val name = (it.arguments[0] as TransactionContext).name + mock() + .also { transaction -> whenever(transaction.name).thenReturn(name) } + .also(startedTransactions::add) + } + } + } + + val stagedNavTransaction = StagedNavTransaction(scopes) + + init { + scope.options.addScopeObserver( + object : ScopeObserverAdapter() { + override fun setTransaction(transaction: String?) { + if (transactionObserverThrows) { + throw ThrowingScopeObserverException() + } + } + } + ) + } + + fun startedTransaction(name: String): ITransaction = startedTransactions.single { + it.name == name + } + + /** Starts a nav transaction for [name], binding it if the scope is free, and returns it. */ + fun start(name: String = "/Profile"): ITransaction { + stagedNavTransaction.start( + scope, + TransactionContext(name, "navigation"), + TransactionOptions(), + ) + return startedTransaction(name) + } + + companion object { + fun create(declinesTransactions: Boolean = false): Fixture = + Fixture().also { it.declinesTransactions = declinesTransactions } + } + } +} From 3bc3299cd3e1309e7dfb7fc24d6ba9df37e685b9 Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Wed, 4 Nov 2026 13:01:11 +0100 Subject: [PATCH 3/3] fix(android): Nav3 support for ViewPager2 (JAVA-785) (#6400) Make the multi-Activity and multi-Fragment support introduced by the #6301 PR stack play nicely with ViewPager2. BACKGROUND The approach in #6301 was smart about staging nav data (ie, screen name + nav context), but it didn't do the same for nav transactions. A new nav transaction could be started only if the lease was owned at the time of navigation, where "navigation" == initialization or modification of the nav back stack. Because the lease had to be owned, that meant that the Activity or Fragment hosting the SentryNavEffect requesting the lease had to be in a resumed state. That^^ assumption made sense for most use cases, because usually initialization of an Activity or Fragment's nav back stack coincides with the Activity or Fragment being visible and therefore being able to preempt the lease from the previous leaseholder. But the assumption didn't pan out for ViewPager2, because ViewPager2 preloads neighboring pages. That means it initializes the nav back stack for Fragments that aren't in a resumed state and therefore can't preempt the lease, causing them to drop their initial nav transaction and any spans created by the initial destination screen. Sigh. THE SOLUTION The solution this commit implements is to create a mechanism similar to the data staging from #6301, but for transactions. Doing so lets us separate back stack initialization observed during preloading vs attempted lease acquisition when a ViewPager2 page becomes visible. In particular, the new StagedNavTransaction class tracks requests to start nav transactions when a SentryNavEffect is created even if it doesn't currently have the lease. If the lease is later acquired and the transaction slot is available on the shared IScope, the staged transaction is started, which keeps ViewPager2 happy. --- CHANGELOG.md | 1 + .../lifecycle/NavLeaseCoordinator.kt | 13 +- .../navigation3/lifecycle/NavScopes.kt | 44 ++-- .../navigation3/lifecycle/NavTransaction.kt | 102 +++++++- .../lifecycle/StagedNavTransaction.kt | 65 +++-- .../CoordinatedBackStackObserverTest.kt | 224 +++++++++++++++- ...SentryNavEffectLifecycleIntegrationTest.kt | 40 ++- .../navigation3/SentryNavEffectTest.kt | 8 +- .../navigation3/lifecycle/NavScopeTest.kt | 65 ++++- .../lifecycle/NavScopesIntegrationTest.kt | 82 ++++++ .../navigation3/lifecycle/NavScopesTest.kt | 242 +++++++++++++++++- .../lifecycle/NavTransactionTest.kt | 229 ++++++++++++++++- .../lifecycle/StagedNavTransactionTest.kt | 156 +++++++++++ .../navigation/nav3/Nav3MultiActivity.kt | 1 + .../navigation/nav3/Nav3RouteScreens.kt | 14 + 15 files changed, 1217 insertions(+), 69 deletions(-) create mode 100644 sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopesIntegrationTest.kt diff --git a/CHANGELOG.md b/CHANGELOG.md index 85bbfc57c8f..36c72632e0c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ### Fixes +- Fix missing Navigation 3 transactions for preloaded ViewPager2 pages and destinations reached while another Activity or Fragment owns navigation tracking ([#6400](https://github.com/getsentry/sentry-java/pull/6400)) - Keep UI profiling running when `startProfiler` is called before a requested stop takes effect at the end of the current chunk ([#6291](https://github.com/getsentry/sentry-java/pull/6291)) - Before, the start call was ignored and the profiler still stopped at the end of the chunk, even in `TRACE` mode while sampled spans were active. On API 34 and lower, every later profile also stopped after one chunk. diff --git a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinator.kt b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinator.kt index 9f8a0f87fde..15e3706ad4c 100644 --- a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinator.kt +++ b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinator.kt @@ -34,8 +34,6 @@ import io.sentry.IScopes * * † Associating work with the incoming transaction means spans produced by the outgoing owner may * appear as if they were produced by the incoming owner (or vice versa, were our policy reversed). - * A possible solution would be to implement a parent span delivery system similar to - * `LocalSentrySpan`, but that's a task for another day. * * **Lease handoff policy** * @@ -133,6 +131,17 @@ import io.sentry.IScopes * Finish txB if unfinished │ * ``` * + * **Nav transactions follow navigation, not the lease** + * + * A candidate gets a nav transaction for a destination it navigates to, never merely for acquiring + * the lease. A destination reached without the lease gets its transaction when the lease is next + * acquired, if it can start then; otherwise it gets none. On reacquiring the lease without having + * navigated in the meantime, a candidate only gets its unfinished transaction back, if any. + * + * Hence the asymmetry for, e.g., ViewPager2 pages: the first time the user swipes to a preloaded + * page, it gets the transaction for the destination it reached while offscreen, but swiping back to + * a page whose transaction has finished starts none. + * * **Exception policy** * * Methods in this class with an `IScope` parameter can throw arbitrary exceptions†, which propagate diff --git a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopes.kt b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopes.kt index 323656c8df9..37a796a58c6 100644 --- a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopes.kt +++ b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopes.kt @@ -43,12 +43,11 @@ internal const val NAVIGATION_CONTEXT_KEY: String = "Navigation" * and publish their navigation data to the shared `IScope`. (The lease policy is implemented by * [NavLeaseCoordinator].) * - * `NavScopes` instances without a lease write to their own copy of [stagedNavData], and keep their - * unfinished nav transaction in [stagedNavTransaction], which lets them preserve their latest - * updates without overwriting another instance's transaction or published data. When an instance - * acquires the lease, it writes its staged data to the shared `IScope` and binds its unfinished - * transaction again (subject to any publishing conditions, e.g., the transaction stays unbound if - * another transaction is already bound to the shared `IScope`). + * `NavScopes` instances without a lease write to their own copy of [stagedNavTransaction] and + * [stagedNavData], which lets them preserve their latest updates without overwriting another + * instance's transaction or published data. When an instance acquires the lease, it writes its + * staged transaction and data to the shared `IScope` (subject to any publishing conditions, e.g., + * staged transactions are dropped if another transaction is already bound to the shared `IScope`). * * **Usage** * @@ -151,6 +150,8 @@ internal class NavScopes( private inner class NavStatePublisher : LeasedNavState { override fun publishTo(scope: IScope) { + // Publishing the data first ensures a staged transaction is sampled against the rotated + // propagation context when it starts. stagedNavData.publishTo(scope) stagedNavTransaction.publishTo(scope) } @@ -219,13 +220,17 @@ internal class NavScopes( } /** - * Starts and binds a nav transaction with the nav lease (see [StagedNavTransaction]). Callers - * must [finish][finishNavTransaction] the previous nav transaction first. + * Starts and binds a nav transaction with the nav lease, or defers it until the lease is + * acquired (see [StagedNavTransaction]). Callers must [finish][finishNavTransaction] the + * previous nav transaction first. * - * Without the lease, starts none and defers the propagation context rotation a new transaction - * would have implied until the lease is acquired. A transaction started without the lease - * couldn't be bound to the scope, so its destination's spans would attach to the leaseholder's - * bound transaction instead, and the empty transaction would be dropped. + * **Deferred transactions may mis-parent spans generated before lease acquisition** + * + * A deferred transaction starts when the lease is acquired, which may be later than when its + * destination starts doing work (e.g., ViewPager2 can precompose offscreen pages, or an + * Activity that navigates from a result callback before it resumes). Spans created in the + * meantime attach to whichever transaction is bound to the scope at that time, usually the + * leaseholder's. */ override fun startNavTransaction( transactionContext: TransactionContext, @@ -235,7 +240,7 @@ internal class NavScopes( stagedNavTransaction.start(delegate, transactionContext, transactionOptions) } else { stagedNavData.deferPropagationContextRotation() - null + stagedNavTransaction.stage(transactionContext, transactionOptions) } } @@ -303,10 +308,15 @@ internal sealed interface INavScope { /** * Starts a nav transaction and binds it to the scope, unless another unfinished transaction is - * already bound to it or the nav lease isn't held. Returns `null` if no transaction was started. + * already bound to it. Returns `null` if no transaction was started. * * (Note: Callers must first [finish][finishNavTransaction] the nav transaction most recently - * started, if any. Otherwise, it blocks the new one while it's bound.) + * requested, if any. Otherwise, with the nav lease, it blocks the new one while it's bound; + * without the lease, it's replaced without being finished.) + * + * The start may be deferred until the nav lease is acquired, in which case writes to the returned + * transaction are staged in the meantime, and whether another transaction blocks it is decided + * then (see [StagedNavTransaction.publishTo]). */ fun startNavTransaction( transactionContext: TransactionContext, @@ -314,8 +324,10 @@ internal sealed interface INavScope { ): INavTransaction? /** - * Finishes the nav transaction most recently started via [startNavTransaction] and unbinds it + * Finishes the nav transaction most recently requested via [startNavTransaction] and unbinds it * from the shared [IScope]. + * + * Alternatively, discards the most recently requested transaction if it's still deferred. */ fun finishNavTransaction() } diff --git a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavTransaction.kt b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavTransaction.kt index cfbfd05f8fb..c8c1464687c 100644 --- a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavTransaction.kt +++ b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavTransaction.kt @@ -1,11 +1,19 @@ package io.sentry.compose.navigation3.lifecycle +import io.sentry.IScopes import io.sentry.ITransaction import io.sentry.SpanStatus +import io.sentry.TransactionContext +import io.sentry.TransactionOptions import io.sentry.protocol.App /** - * An [INavTransaction] handle backed by a started [ITransaction]. + * An [INavTransaction] handle backed by an [ITransaction] that's either started right away or, when + * the nav lease isn't held, [started later][start]. + * + * A deferred handle records what's set on it and replays it onto the transaction once started. + * [Finishing][finish] a deferred handle before it starts discards it, so its transaction never + * starts. * * **Thread safety** * @@ -13,39 +21,109 @@ import io.sentry.protocol.App */ internal class NavTransaction private constructor( - /** The backing transaction. */ - val transaction: ITransaction + val name: String, + private var state: State, ) : INavTransaction { - val name: String - get() = transaction.name + /** + * The backing transaction, once [started][State.Started]; `null` while [pending][State.Pending] + * or once [discarded][State.Discarded]. + */ + val transaction: ITransaction? + get() = (state as? State.Started)?.transaction override fun setData(key: String, value: Any) { - transaction.setData(key, value) + when (val state = state) { + is State.Pending -> state.data[key] = value + is State.Started -> state.transaction.setData(key, value) + State.Discarded -> Unit + } } override fun setContext(key: String, value: Any) { - transaction.setContext(key, value) + when (val state = state) { + is State.Pending -> state.contexts[key] = value + is State.Started -> state.transaction.setContext(key, value) + State.Discarded -> Unit + } } override fun setApp(app: App) { - transaction.contexts.setApp(app) + when (val state = state) { + is State.Pending -> state.app = app + is State.Started -> state.transaction.contexts.setApp(app) + State.Discarded -> Unit + } + } + + /** + * Starts a deferred handle's transaction via [scopes] and replays what was set on the handle onto + * it. + * + * Returns the started transaction, or `null` if the handle was started already or discarded, or + * [scopes] declined to start one. + */ + fun start(scopes: IScopes): ITransaction? { + val pending = state as? State.Pending ?: return null + val transaction = + scopes.startTransaction(pending.context, pending.options).takeUnless { it.isNoOp } + ?: return null + + state = State.Started(transaction) + pending.data.forEach { (key, value) -> transaction.setData(key, value) } + pending.contexts.forEach { (key, value) -> transaction.setContext(key, value) } + pending.app?.let { transaction.contexts.setApp(it) } + return transaction } /** * Finishes the backing transaction with its own status, or [OK][SpanStatus.OK] if it has none, * and then passes it to [onFinished]. + * + * A deferred handle that hasn't started is discarded instead, so it never starts, and + * [onFinished] isn't called. */ fun finish(onFinished: (ITransaction) -> Unit) { - if (!transaction.isFinished) { - transaction.finish(transaction.status ?: SpanStatus.OK) + when (val state = state) { + is State.Pending -> this.state = State.Discarded + is State.Started -> { + if (!state.transaction.isFinished) { + state.transaction.finish(state.transaction.status ?: SpanStatus.OK) + } + onFinished(state.transaction) + } + State.Discarded -> Unit } - onFinished(transaction) + } + + /** Lifecycle state for a [NavTransaction] handle. */ + private sealed interface State { + + /** + * Not started yet: the transaction will be started from [context] and [options], and what's set + * on the handle until then is recorded for replay. + */ + class Pending(val context: TransactionContext, val options: TransactionOptions) : State { + val data = linkedMapOf() + val contexts = linkedMapOf() + var app: App? = null + } + + /** The backing [transaction] has been started. A terminal [State]. */ + class Started(val transaction: ITransaction) : State + + /** The handle was finished before its backing transaction was started. A terminal [State]. */ + data object Discarded : State } companion object { /** A handle backed by the already-started [transaction]. */ - fun started(transaction: ITransaction): NavTransaction = NavTransaction(transaction) + fun started(transaction: ITransaction): NavTransaction = + NavTransaction(transaction.name, State.Started(transaction)) + + /** A handle whose transaction is started later, via [start]. */ + fun deferred(context: TransactionContext, options: TransactionOptions): NavTransaction = + NavTransaction(context.name, State.Pending(context, options)) } } diff --git a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavTransaction.kt b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavTransaction.kt index 793aad39142..2851a10481c 100644 --- a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavTransaction.kt +++ b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavTransaction.kt @@ -10,9 +10,11 @@ import io.sentry.TransactionOptions /** * Holds a nav transaction staged for publication to a shared [IScope]. * - * A started transaction that's [unpublished][unpublishFrom] is unbound but keeps running, so that - * it can be bound again if it's republished while unfinished. Once [finished][finish], a - * transaction is forgotten and never bound again. + * A transaction requested while publishing isn't permitted is [staged][stage] rather than started, + * and starts when it's next [published][publishTo]. That publication is its only chance to start: + * if it can't start then, it's discarded. A started transaction that's [unpublished][unpublishFrom] + * is unbound but keeps running, so that it can be bound again if it's republished while unfinished. + * Once [finished][finish], a transaction is forgotten and never bound again. * * Starting or publishing a nav transaction never displaces an unfinished transaction bound to the * scope by someone else (e.g., the host app). @@ -23,7 +25,9 @@ import io.sentry.TransactionOptions */ internal class StagedNavTransaction(private val scopes: IScopes) { - /** The nav transaction most recently started, unless it has been finished since then. */ + /** + * The nav transaction most recently started or staged, unless it has been finished since then. + */ private var navTransaction: NavTransaction? = null /** @@ -57,20 +61,46 @@ internal class StagedNavTransaction(private val scopes: IScopes) { } /** - * Binds the nav transaction to [scope]. A transaction that has finished in the meantime is - * forgotten and unbound instead. + * Stages a nav transaction that's started from [context] and [options] when it's + * [published][publishTo], and returns its handle, which records what's set on it in the meantime. + * + * **Warning**: [Finish][finish] the current nav transaction before calling this method, as this + * method replaces the current transaction without finishing it. + */ + fun stage(context: TransactionContext, options: TransactionOptions): NavTransaction = + NavTransaction.deferred(context, options).also { navTransaction = it } + + /** + * Binds the nav transaction to [scope], starting it first if it's staged. A started transaction + * that has finished in the meantime is forgotten and unbound instead. * - * If another unfinished transaction is bound to [scope], the transaction stays unbound, so it can - * be bound the next time it's published. + * If another unfinished transaction is bound to [scope], a started transaction stays unbound, so + * it can be bound the next time it's published. A staged transaction is discarded instead, as it + * is if [scopes] return a no-op transaction for it (see [start]). */ fun publishTo(scope: IScope) { val navTransaction = navTransaction ?: return val started = navTransaction.transaction - if (started.isFinished) { - this.navTransaction = null - unbind(scope, started) - } else if (scope.isFreeFor(navTransaction.name, started)) { - bind(scope, started) + when { + started?.isFinished == true -> { + this.navTransaction = null + unbind(scope, started) + } + started != null -> if (scope.isFreeFor(navTransaction.name, started)) bind(scope, started) + else -> { + val startedNow = + if (scope.isFreeFor(navTransaction.name, transaction = null)) { + navTransaction.start(scopes) + } else { + null + } + if (startedNow != null) { + bind(scope, startedNow) + } else { + this.navTransaction = null + navTransaction.finish(onFinished = {}) + } + } } } @@ -79,10 +109,13 @@ internal class StagedNavTransaction(private val scopes: IScopes) { * [published][publishTo] again. */ fun unpublishFrom(scope: IScope) { - navTransaction?.let { unbind(scope, it.transaction) } + navTransaction?.transaction?.let { unbind(scope, it) } } - /** Finishes the nav transaction and unbinds it from [scope], if it's bound. */ + /** + * Finishes the nav transaction and unbinds it from [scope], if it's bound. A staged transaction + * is discarded instead. + */ fun finish(scope: IScope) { val navTransaction = navTransaction ?: return // Forget the transaction before unbinding it: unbinding notifies the host app's scope @@ -93,7 +126,7 @@ internal class StagedNavTransaction(private val scopes: IScopes) { /** * Forgets the nav transaction without finishing it, e.g., when there's no scope to unbind it - * from. It will be left to time out. + * from. If started, it will be left to time out. */ fun forget() { navTransaction = null diff --git a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/CoordinatedBackStackObserverTest.kt b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/CoordinatedBackStackObserverTest.kt index c742d3dd5b2..cf653c23e31 100644 --- a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/CoordinatedBackStackObserverTest.kt +++ b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/CoordinatedBackStackObserverTest.kt @@ -4,8 +4,11 @@ import androidx.lifecycle.Lifecycle import androidx.lifecycle.LifecycleOwner import androidx.lifecycle.LifecycleRegistry import com.google.common.truth.Truth.assertThat +import io.sentry.Breadcrumb +import io.sentry.Hint import io.sentry.IScope import io.sentry.IScopes +import io.sentry.ITransaction import io.sentry.Scope import io.sentry.ScopeCallback import io.sentry.SentryOptions @@ -53,8 +56,210 @@ class CoordinatedBackStackObserverTest { homeLifecycle.currentState = Lifecycle.State.RESUMED assertThat(fixture.scope.screen).isEqualTo("/Result") - // "/Result" was reached without the nav lease, so it has no nav transaction to bind. + // "/Result" was reached without the nav lease, so its nav transaction starts when the owner + // regains the lease, rather than the one retired by navigating to it being rebound. + assertThat(fixture.scope.transaction).isNotSameInstanceAs(homeTransaction) + assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransaction("/Result")) + } + + @Test + fun `a page swiped to starts its nav transaction when it resumes`() { + val fixture = Fixture() + val pageALifecycle = fixture.resumedLifecycle() + val pageBLifecycle = fixture.resumedLifecycle().apply { currentState = Lifecycle.State.STARTED } + val pageA = fixture.getSut(pageALifecycle) + val pageB = fixture.getSut(pageBLifecycle, arguments = { mapOf("page" to it) }) + pageA.onBackStackChanged(listOf("PageA")) + // Page B is preloaded while page A is resumed, so it doesn't get the nav lease. + pageB.onBackStackChanged(listOf("PageB")) + check(fixture.startedTransactions.map { it.name } == listOf("/PageA")) + + // The swipe settles: ViewPager2 resumes page B and pauses page A. + pageBLifecycle.currentState = Lifecycle.State.RESUMED + pageALifecycle.currentState = Lifecycle.State.STARTED + + val pageBTransaction = fixture.startedTransaction("/PageB") + assertThat(fixture.scope.transaction).isSameInstanceAs(pageBTransaction) + assertThat(pageBTransaction.getData("arguments")).isEqualTo(mapOf("page" to "PageB")) + assertThat(pageBTransaction.contexts.app?.viewNames).containsExactly("/PageB") + assertThat(pageBTransaction.contexts[NAVIGATION_CONTEXT_KEY]) + .isEqualTo( + mapOf( + "Back Stack" to + listOf(mapOf("entry" to "/PageB", "entry_arguments" to mapOf("page" to "PageB"))) + ) + ) + assertThat(fixture.scope.screen).isEqualTo("/PageB") + } + + @Test + fun `a page swiped to starts its nav transaction when the previous page pauses before it resumes`() { + val fixture = Fixture() + val pageALifecycle = fixture.resumedLifecycle() + val pageBLifecycle = fixture.resumedLifecycle().apply { currentState = Lifecycle.State.STARTED } + val pageA = fixture.getSut(pageALifecycle) + val pageB = fixture.getSut(pageBLifecycle) + pageA.onBackStackChanged(listOf("PageA")) + pageB.onBackStackChanged(listOf("PageB")) + + pageALifecycle.currentState = Lifecycle.State.STARTED + pageBLifecycle.currentState = Lifecycle.State.RESUMED + + assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransaction("/PageB")) + assertThat(fixture.scope.screen).isEqualTo("/PageB") + } + + @Test + fun `a page swiped to records no breadcrumb beyond its first back stack update when it resumes`() { + val fixture = Fixture() + val pageALifecycle = fixture.resumedLifecycle() + val pageBLifecycle = fixture.resumedLifecycle().apply { currentState = Lifecycle.State.STARTED } + val pageA = fixture.getSut(pageALifecycle) + val pageB = fixture.getSut(pageBLifecycle) + pageA.onBackStackChanged(listOf("PageA")) + pageB.onBackStackChanged(listOf("PageB")) + + pageBLifecycle.currentState = Lifecycle.State.RESUMED + pageALifecycle.currentState = Lifecycle.State.STARTED + + assertThat(fixture.breadcrumbs.map { it.data["to"] }) + .containsExactly("/PageA", "/PageB") + .inOrder() + } + + @Test + fun `a page preloaded during a host transaction starts its nav transaction when swiped to after it finishes`() { + val fixture = Fixture() + val pageALifecycle = fixture.resumedLifecycle() + val pageBLifecycle = fixture.resumedLifecycle().apply { currentState = Lifecycle.State.STARTED } + val pageA = fixture.getSut(pageALifecycle) + val pageB = fixture.getSut(pageBLifecycle) + // E.g., the Activity's ui.load transaction, still running while the pager's pages load. + val hostTransaction = mock() + fixture.scope.transaction = hostTransaction + pageA.onBackStackChanged(listOf("PageA")) + pageB.onBackStackChanged(listOf("PageB")) + whenever(hostTransaction.isFinished).thenReturn(true) + + pageBLifecycle.currentState = Lifecycle.State.RESUMED + pageALifecycle.currentState = Lifecycle.State.STARTED + + assertThat(fixture.startedTransactions.map { it.name }).containsExactly("/PageB") + assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransaction("/PageB")) + } + + @Test + fun `a page swiped to while a host transaction is bound starts no nav transaction when swiped to again`() { + val fixture = Fixture() + val pageALifecycle = fixture.resumedLifecycle() + val pageBLifecycle = fixture.resumedLifecycle().apply { currentState = Lifecycle.State.STARTED } + val pageA = fixture.getSut(pageALifecycle) + val pageB = fixture.getSut(pageBLifecycle) + val hostTransaction = mock() + fixture.scope.transaction = hostTransaction + pageA.onBackStackChanged(listOf("PageA")) + pageB.onBackStackChanged(listOf("PageB")) + pageBLifecycle.currentState = Lifecycle.State.RESUMED + pageALifecycle.currentState = Lifecycle.State.STARTED + whenever(hostTransaction.isFinished).thenReturn(true) + + // Swipe back to page A, then to page B again. + pageALifecycle.currentState = Lifecycle.State.RESUMED + pageBLifecycle.currentState = Lifecycle.State.STARTED + pageBLifecycle.currentState = Lifecycle.State.RESUMED + pageALifecycle.currentState = Lifecycle.State.STARTED + + // As with page A, which navigated while the host transaction was bound, page B's destination + // gets no nav transaction: it couldn't start one when it first got the nav lease. + assertThat(fixture.startedTransactions).isEmpty() + assertThat(fixture.scope.screen).isEqualTo("/PageB") + } + + @Test + fun `a page that navigates while offscreen starts only its latest destination's nav transaction when swiped to`() { + val fixture = Fixture() + val pageALifecycle = fixture.resumedLifecycle() + val pageBLifecycle = fixture.resumedLifecycle().apply { currentState = Lifecycle.State.STARTED } + val pageA = fixture.getSut(pageALifecycle) + val pageB = fixture.getSut(pageBLifecycle) + pageA.onBackStackChanged(listOf("PageA")) + pageB.onBackStackChanged(listOf("PageB")) + pageB.onBackStackChanged(listOf("PageB", "Detail")) + + pageBLifecycle.currentState = Lifecycle.State.RESUMED + pageALifecycle.currentState = Lifecycle.State.STARTED + + assertThat(fixture.startedTransactions.map { it.name }) + .containsExactly("/PageA", "/Detail") + .inOrder() + assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransaction("/Detail")) + } + + @Test + fun `swiping back to a page restores its unfinished nav transaction instead of starting another`() { + val fixture = Fixture() + val pageALifecycle = fixture.resumedLifecycle() + val pageBLifecycle = fixture.resumedLifecycle().apply { currentState = Lifecycle.State.STARTED } + val pageA = fixture.getSut(pageALifecycle) + val pageB = fixture.getSut(pageBLifecycle) + pageA.onBackStackChanged(listOf("PageA")) + val pageATransaction = fixture.startedTransaction("/PageA") + // A pending child span keeps the nav transaction, which waits for its children, unfinished. + pageATransaction.startChild("http.client") + pageB.onBackStackChanged(listOf("PageB")) + pageBLifecycle.currentState = Lifecycle.State.RESUMED + pageALifecycle.currentState = Lifecycle.State.STARTED + + pageALifecycle.currentState = Lifecycle.State.RESUMED + pageBLifecycle.currentState = Lifecycle.State.STARTED + + assertThat(fixture.scope.transaction).isSameInstanceAs(pageATransaction) + assertThat(fixture.startedTransactions.map { it.name }) + .containsExactly("/PageA", "/PageB") + .inOrder() + } + + @Test + fun `swiping back to a page whose nav transaction finished starts no nav transaction`() { + val fixture = Fixture() + val pageALifecycle = fixture.resumedLifecycle() + val pageBLifecycle = fixture.resumedLifecycle().apply { currentState = Lifecycle.State.STARTED } + val pageA = fixture.getSut(pageALifecycle) + val pageB = fixture.getSut(pageBLifecycle) + pageA.onBackStackChanged(listOf("PageA")) + val pageATransaction = fixture.startedTransaction("/PageA") + pageB.onBackStackChanged(listOf("PageB")) + pageBLifecycle.currentState = Lifecycle.State.RESUMED + pageALifecycle.currentState = Lifecycle.State.STARTED + pageATransaction.finish() + + pageALifecycle.currentState = Lifecycle.State.RESUMED + pageBLifecycle.currentState = Lifecycle.State.STARTED + + assertThat(fixture.startedTransactions.map { it.name }) + .containsExactly("/PageA", "/PageB") + .inOrder() assertThat(fixture.scope.transaction).isNull() + assertThat(fixture.scope.screen).isEqualTo("/PageA") + } + + @Test + fun `with nav transactions disabled, a page swiped to starts no nav transaction`() { + val fixture = Fixture() + val options = SentryNavOptions { enableNavigationTransactions = false } + val pageALifecycle = fixture.resumedLifecycle() + val pageBLifecycle = fixture.resumedLifecycle().apply { currentState = Lifecycle.State.STARTED } + val pageA = fixture.getSut(pageALifecycle, options) + val pageB = fixture.getSut(pageBLifecycle, options) + pageA.onBackStackChanged(listOf("PageA")) + pageB.onBackStackChanged(listOf("PageB")) + + pageBLifecycle.currentState = Lifecycle.State.RESUMED + pageALifecycle.currentState = Lifecycle.State.STARTED + + assertThat(fixture.startedTransactions).isEmpty() + assertThat(fixture.scope.transaction).isNull() + assertThat(fixture.scope.screen).isEqualTo("/PageB") } @Test @@ -122,9 +327,16 @@ class CoordinatedBackStackObserverTest { ) val coordinator = NavLeaseCoordinator() val startedTransactions = mutableListOf() + val breadcrumbs = mutableListOf() private val scopes = mock().also { scopes -> whenever(scopes.options).thenReturn(scope.options) + doAnswer { + breadcrumbs += it.arguments[0] as Breadcrumb + null + } + .whenever(scopes) + .addBreadcrumb(any(), any()) doAnswer { (it.arguments[0] as ScopeCallback).run(scope) null @@ -154,13 +366,17 @@ class CoordinatedBackStackObserverTest { check(other.hasLease) } - fun getSut(lifecycle: Lifecycle = resumedLifecycle()): CoordinatedBackStackObserver = + fun getSut( + lifecycle: Lifecycle = resumedLifecycle(), + options: SentryNavOptions = SentryNavOptions(), + arguments: (String) -> Map? = { null }, + ): CoordinatedBackStackObserver = CoordinatedBackStackObserver( scopes = scopes, - options = SentryNavOptions(), + options = options, entryMapper = ForwardingBackStackEntryMapper { - BackStackEntryMapper { SentryBackStackEntry(it) } + BackStackEntryMapper { SentryBackStackEntry(it, arguments(it)) } }, coordinator = coordinator, ) diff --git a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/SentryNavEffectLifecycleIntegrationTest.kt b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/SentryNavEffectLifecycleIntegrationTest.kt index 7034ba29f68..d5467b10b9a 100644 --- a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/SentryNavEffectLifecycleIntegrationTest.kt +++ b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/SentryNavEffectLifecycleIntegrationTest.kt @@ -407,19 +407,55 @@ class SentryNavEffectLifecycleIntegrationTest { } @Test - fun `switching to a preloaded view pager page publishes a new propagation context instead of a nav transaction`() { + fun `switching to a preloaded view pager page starts its nav transaction with a new propagation context`() { val fixture = Fixture.create() val activity = Robolectric.buildActivity(ViewPagerActivity::class.java).setup() fixture.idle() + val homeTransaction = requireNotNull(fixture.scope.transaction) val propagationContext = fixture.scope.propagationContext activity.get().viewPager.setCurrentItem(1, false) fixture.idle() - assertThat(fixture.scope.transaction).isNull() + assertThat(fixture.scope.transaction?.name).isEqualTo("/Checkout") + assertThat(fixture.scope.transaction).isNotSameInstanceAs(homeTransaction) assertThat(fixture.scope.propagationContext).isNotSameInstanceAs(propagationContext) } + @Test + fun `switching back to a view pager page restores its unfinished nav transaction`() { + val fixture = Fixture.create() + val activity = Robolectric.buildActivity(ViewPagerActivity::class.java).setup() + fixture.idle() + val homeTransaction = requireNotNull(fixture.scope.transaction) + activity.get().viewPager.setCurrentItem(1, false) + fixture.idle() + check(!homeTransaction.isFinished) + + activity.get().viewPager.setCurrentItem(0, false) + fixture.idle() + + assertThat(fixture.scope.transaction).isSameInstanceAs(homeTransaction) + assertThat(fixture.scope.screen).isEqualTo("/Home") + } + + @Test + fun `switching back to a view pager page whose nav transaction finished starts no nav transaction`() { + val fixture = Fixture.create() + val activity = Robolectric.buildActivity(ViewPagerActivity::class.java).setup() + fixture.idle() + val homeTransaction = requireNotNull(fixture.scope.transaction) + activity.get().viewPager.setCurrentItem(1, false) + fixture.idle() + homeTransaction.finish() + + activity.get().viewPager.setCurrentItem(0, false) + fixture.idle() + + assertThat(fixture.scope.transaction).isNull() + assertThat(fixture.scope.screen).isEqualTo("/Home") + } + // Dialogs @Test fun `a dialog fragment shown over a fragment publishes its own nav data while the fragment stays resumed`() { diff --git a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/SentryNavEffectTest.kt b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/SentryNavEffectTest.kt index 32536d28ccf..0ce82b832a6 100644 --- a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/SentryNavEffectTest.kt +++ b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/SentryNavEffectTest.kt @@ -629,7 +629,7 @@ class SentryNavEffectTest { } @Test - fun `navigating without the lease records breadcrumbs, then publishes with a fresh trace on resume`() { + fun `navigating without the lease records breadcrumbs, then starts a nav transaction with a fresh trace on resume`() { val fixture = NavTestFixture() val ownerA = NavTestLifecycle(Lifecycle.State.RESUMED) val ownerB = NavTestLifecycle() @@ -674,10 +674,10 @@ class SentryNavEffectTest { ownerA.resume() } - // A gets no nav transaction for /Result: one started without the lease would have collected no - // spans of its own. It publishes its screen and gets the fresh trace a transaction would have. + // A's nav transaction for /Result starts once it regains the lease: one started without the + // lease would have collected no spans of its own. composeRule.runOnIdle { - assertThat(fixture.scope.transaction).isNull() + assertThat(fixture.scope.transaction?.name).isEqualTo("/Result") assertThat(fixture.scope.screen).isEqualTo("/Result") assertThat(fixture.scope.propagationContext).isNotSameInstanceAs(contextBeforeResume) } diff --git a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopeTest.kt b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopeTest.kt index a079c91a545..57be6d68d5d 100644 --- a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopeTest.kt +++ b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopeTest.kt @@ -197,42 +197,59 @@ class NavScopeTest { } @Test - fun `startNavTransaction() - starts none without the nav lease`() { + fun `startNavTransaction() - defers the start despite a bound host transaction without the nav lease`() { val fixture = Fixture.create() val navScopes = fixture.navScopes + val hostTransaction = mock() + fixture.scope.transaction = hostTransaction val navTransaction = navScopes.withNavScope { it.startNavTransaction("/Profile") } - assertThat(navTransaction).isNull() + assertThat(navTransaction).isNotNull() assertThat(fixture.startedTransactions).isEmpty() - assertThat(fixture.scope.transaction).isNull() + assertThat(fixture.scope.transaction).isSameInstanceAs(hostTransaction) } @Test - fun `startNavTransaction() - starts none without the nav lease, even once the lease is acquired`() { + fun `startNavTransaction() - starts a deferred transaction once the host's is unbound and the lease acquired`() { val fixture = Fixture.create() val navScopes = fixture.navScopes + fixture.scope.transaction = mock() navScopes.withNavScope { it.startNavTransaction("/Profile") } + fixture.scope.clearTransaction() fixture.acquireLease() - assertThat(fixture.startedTransactions).isEmpty() - assertThat(fixture.scope.transaction).isNull() + assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransactions.single()) } @Test - fun `startNavTransaction() - starts none and keeps another owner's bound nav transaction without the nav lease`() { + fun `startNavTransaction() - defers the start despite another owner's bound nav transaction without the nav lease`() { val fixture = Fixture.create() val navScopes = fixture.navScopes val otherTransaction = fixture.startAnotherOwnersNavTransaction() val navTransaction = navScopes.withNavScope { it.startNavTransaction("/Profile") } - assertThat(navTransaction).isNull() + assertThat(navTransaction).isNotNull() assertThat(fixture.startedTransactions).containsExactly(otherTransaction) assertThat(fixture.scope.transaction).isSameInstanceAs(otherTransaction) } + @Test + fun `startNavTransaction() - defers the start and keeps a finished bound transaction without the nav lease`() { + val fixture = Fixture.create() + val navScopes = fixture.navScopes + val finished = mock().also { whenever(it.isFinished).thenReturn(true) } + fixture.scope.transaction = finished + + val navTransaction = navScopes.withNavScope { it.startNavTransaction("/Profile") } + + assertThat(navTransaction).isNotNull() + assertThat(fixture.startedTransactions).isEmpty() + assertThat(fixture.scope.transaction).isSameInstanceAs(finished) + } + @Test fun `startNavTransaction() - starts none while its unfinished previous nav transaction is bound`() { val fixture = Fixture.create(ownsLease = true) @@ -261,6 +278,21 @@ class NavScopeTest { assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransactions.last()) } + @Test + fun `startNavTransaction() - replaces the previous deferred transaction without the nav lease`() { + val fixture = Fixture.create() + val navScopes = fixture.navScopes + val previous = navScopes.withNavScope { it.startNavTransaction("/Profile") } as NavTransaction + + val navTransaction = navScopes.withNavScope { it.startNavTransaction("/Settings") } + fixture.acquireLease() + + assertThat(navTransaction).isNotNull() + assertThat(previous.transaction).isNull() + assertThat(fixture.startedTransactions.map { it.name }).containsExactly("/Settings") + assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransactions.single()) + } + @Test fun `startNavTransaction() - starts none while a host transaction is bound after the previous one finishes`() { val fixture = Fixture.create(ownsLease = true) @@ -279,6 +311,23 @@ class NavScopeTest { assertThat(fixture.scope.transaction).isSameInstanceAs(hostTransaction) } + @Test + fun `startNavTransaction() - replaces the previous deferred transaction while a host transaction is bound`() { + val fixture = Fixture.create() + val navScopes = fixture.navScopes + val previous = navScopes.withNavScope { it.startNavTransaction("/Profile") } as NavTransaction + fixture.scope.transaction = mock() + + val navTransaction = navScopes.withNavScope { it.startNavTransaction("/Settings") } + fixture.scope.clearTransaction() + fixture.acquireLease() + + assertThat(navTransaction).isNotNull() + assertThat(previous.transaction).isNull() + assertThat(fixture.startedTransactions.map { it.name }).containsExactly("/Settings") + assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransactions.single()) + } + @Test fun `finishNavTransaction() - finishes and unbinds the nav transaction`() { val fixture = Fixture.create(ownsLease = true) diff --git a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopesIntegrationTest.kt b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopesIntegrationTest.kt new file mode 100644 index 00000000000..4f01197a0f7 --- /dev/null +++ b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopesIntegrationTest.kt @@ -0,0 +1,82 @@ +package io.sentry.compose.navigation3.lifecycle + +import androidx.lifecycle.Lifecycle +import androidx.lifecycle.LifecycleOwner +import androidx.lifecycle.LifecycleRegistry +import com.google.common.truth.Truth.assertThat +import io.sentry.ISentryClient +import io.sentry.PropagationContext +import io.sentry.Scope +import io.sentry.ScopeType +import io.sentry.Scopes +import io.sentry.SentryOptions +import io.sentry.TransactionContext +import io.sentry.TransactionOptions +import org.junit.Test +import org.mockito.kotlin.mock +import org.mockito.kotlin.whenever + +/** Exercises navigation lease publication together with real SDK transaction sampling. */ +class NavScopesIntegrationTest { + + @Test + fun `a deferred nav transaction is sampled after the propagation context rotates`() { + val fixture = Fixture.create() + val incumbentOwner = TestLifecycleOwner(Lifecycle.State.RESUMED) + val incumbent = fixture.navScopes() + incumbent.attach(incumbentOwner.lifecycle) + incumbent.onBackStackUpdate() + val incomingOwner = TestLifecycleOwner(Lifecycle.State.STARTED) + val incoming = fixture.navScopes() + incoming.attach(incomingOwner.lifecycle) + incoming.onBackStackUpdate() + val contextBeforeResume = fixture.scope.propagationContext + val contextsAtSampling = mutableListOf() + fixture.options.setTracesSampler { + contextsAtSampling += fixture.scope.propagationContext + 1.0 + } + incoming.configureScope { + it.startNavTransaction(TransactionContext("/Checkout", "navigation"), TransactionOptions()) + } + assertThat(contextsAtSampling).isEmpty() + assertThat(fixture.scope.propagationContext).isSameInstanceAs(contextBeforeResume) + + incomingOwner.lifecycle.currentState = Lifecycle.State.RESUMED + + assertThat(contextsAtSampling).hasSize(1) + assertThat(contextsAtSampling.single()).isNotSameInstanceAs(contextBeforeResume) + assertThat(contextsAtSampling.single()).isSameInstanceAs(fixture.scope.propagationContext) + assertThat(fixture.scope.transaction?.name).isEqualTo("/Checkout") + incoming.dispose() + incumbent.dispose() + } + + private class TestLifecycleOwner(initialState: Lifecycle.State) : LifecycleOwner { + override val lifecycle: LifecycleRegistry = + LifecycleRegistry.createUnsafe(this).apply { currentState = initialState } + } + + private class Fixture private constructor() { + val options = + SentryOptions().apply { + dsn = "http://key@localhost/1" + defaultScopeType = ScopeType.CURRENT + } + val scope = Scope(options) + private val scopes = Scopes(scope, Scope(options), Scope(options), "navigation sampling test") + private val coordinator = NavLeaseCoordinator() + + init { + val client = mock() + whenever(client.isEnabled).thenReturn(true) + scopes.bindClient(client) + } + + fun navScopes(): NavScopes = NavScopes(scopes, coordinator) + + companion object { + fun create(): Fixture = Fixture() + } + } +} diff --git a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopesTest.kt b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopesTest.kt index 3226cbe162d..c865e8e91e6 100644 --- a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopesTest.kt +++ b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavScopesTest.kt @@ -208,6 +208,20 @@ class NavScopesTest { verify(started, never()).finish(any()) } + @Test + fun `dispose() - forgets a nav transaction staged while never attached`() { + val fixture = Fixture.create() + val navScopes = fixture.navScopes() + navScopes.startNavTransaction("/Home") + + navScopes.dispose() + navScopes.attach(fixture.lifecycle(Lifecycle.State.RESUMED)) + navScopes.applyFirstBackStackUpdate() + + assertThat(fixture.hasLease(navScopes)).isTrue() + assertThat(fixture.startedTransactions).isEmpty() + } + @Test fun `startNavTransaction() - starts and binds a nav transaction with the nav lease`() { val fixture = Fixture.create() @@ -248,14 +262,14 @@ class NavScopesTest { } @Test - fun `startNavTransaction() - starts no nav transaction without the nav lease`() { + fun `startNavTransaction() - defers the nav transaction start without the nav lease`() { val fixture = Fixture.create() val covered = fixture.coveredNavScopes() val transaction = covered.startNavTransaction("/Result") assertThat(fixture.hasLease(covered)).isFalse() - assertThat(transaction).isNull() + assertThat(transaction).isNotNull() assertThat(fixture.startedTransactions).isEmpty() assertThat(fixture.scope.transaction).isNull() } @@ -413,6 +427,230 @@ class NavScopesTest { assertThat(fixture.scope.transaction).isNull() } + @Test + fun `a deferred transaction starts and is bound when the nav lease is acquired`() { + val fixture = Fixture.create() + val covered = fixture.coveredNavScopes() + covered.startNavTransaction("/Result") + + fixture.incoming.dispose() + + assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransaction("/Result")) + } + + @Test + fun `a deferred transaction receives the data recorded on its handle once it starts`() { + val fixture = Fixture.create() + val covered = fixture.coveredNavScopes() + val transaction = covered.startNavTransaction("/Result") + transaction?.setData("arguments", "id=1") + + fixture.incoming.dispose() + + verify(fixture.startedTransaction("/Result")).setData("arguments", "id=1") + } + + @Test + fun `a preloaded owner's deferred transaction starts when the owner resumes`() { + val fixture = Fixture.create() + val current = fixture.navScopes() + current.attach(fixture.lifecycle(Lifecycle.State.RESUMED)) + current.applyFirstBackStackUpdate() + val preloaded = fixture.navScopes() + val preloadedLifecycle = fixture.lifecycle(Lifecycle.State.STARTED) + preloaded.attach(preloadedLifecycle) + preloaded.applyFirstBackStackUpdate() + preloaded.startNavTransaction("/PageB") + check(!fixture.hasLease(preloaded)) + + preloadedLifecycle.currentState = Lifecycle.State.RESUMED + + assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransaction("/PageB")) + } + + @Test + fun `a started deferred transaction is restored when the nav lease is regained`() { + val fixture = Fixture.create() + val covered = fixture.coveredNavScopes() + covered.startNavTransaction("/Result") + fixture.incoming.dispose() + val started = fixture.startedTransaction("/Result") + val nextIncoming = fixture.navScopes() + nextIncoming.attach(fixture.lifecycle(Lifecycle.State.RESUMED)) + nextIncoming.applyFirstBackStackUpdate() + check(fixture.scope.transaction == null) + + nextIncoming.dispose() + + assertThat(fixture.scope.transaction).isSameInstanceAs(started) + assertThat(fixture.startedTransactions).containsExactly(started) + } + + @Test + fun `regaining the nav lease with a started nav transaction logs no blocked nav transaction start`() { + val fixture = Fixture.create() + val covered = fixture.coveredNavScopes() + covered.startNavTransaction("/Result") + fixture.incoming.dispose() + val nextIncoming = fixture.navScopes() + nextIncoming.attach(fixture.lifecycle(Lifecycle.State.RESUMED)) + nextIncoming.applyFirstBackStackUpdate() + + nextIncoming.dispose() + + verify(fixture.logger, never()).log(eq(SentryLevel.DEBUG), any(), eq("/Result")) + } + + @Test + fun `a started deferred transaction is finished on dispose`() { + val fixture = Fixture.create() + val covered = fixture.coveredNavScopes() + covered.startNavTransaction("/Result") + fixture.incoming.dispose() + val started = fixture.startedTransaction("/Result") + + covered.dispose() + + verify(started).finish(any()) + assertThat(fixture.scope.transaction).isNull() + } + + @Test + fun `a deferred transaction does not start while a host transaction is bound`() { + val fixture = Fixture.create() + val covered = fixture.coveredNavScopes() + covered.startNavTransaction("/Result") + val hostTransaction = mock() + fixture.scope.transaction = hostTransaction + + fixture.incoming.dispose() + + assertThat(fixture.startedTransactions).isEmpty() + assertThat(fixture.scope.transaction).isSameInstanceAs(hostTransaction) + } + + @Test + fun `a deferred transaction blocked by a host transaction logs its route`() { + val fixture = Fixture.create() + val covered = fixture.coveredNavScopes() + covered.startNavTransaction("/Result") + fixture.scope.transaction = mock() + + fixture.incoming.dispose() + + verify(fixture.logger).log(eq(SentryLevel.DEBUG), any(), eq("/Result")) + } + + @Test + fun `a deferred transaction starts in place of a finished bound transaction`() { + val fixture = Fixture.create() + val covered = fixture.coveredNavScopes() + covered.startNavTransaction("/Result") + fixture.scope.transaction = + mock().also { whenever(it.isFinished).thenReturn(true) } + + fixture.incoming.dispose() + + assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransaction("/Result")) + } + + @Test + fun `a deferred transaction requested while a finished transaction is bound starts in its place`() { + val fixture = Fixture.create() + val covered = fixture.coveredNavScopes() + fixture.scope.transaction = + mock().also { whenever(it.isFinished).thenReturn(true) } + covered.startNavTransaction("/Result") + + fixture.incoming.dispose() + + assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransaction("/Result")) + } + + @Test + fun `a finished deferred transaction does not start when the nav lease is acquired`() { + val fixture = Fixture.create() + val covered = fixture.coveredNavScopes() + covered.startNavTransaction("/Result") + covered.finishNavTransaction() + + fixture.incoming.dispose() + + assertThat(fixture.startedTransactions).isEmpty() + assertThat(fixture.scope.transaction).isNull() + } + + @Test + fun `only the most recently deferred transaction starts when the nav lease is acquired`() { + val fixture = Fixture.create() + val covered = fixture.coveredNavScopes() + covered.startNavTransaction("/Result") + covered.finishNavTransaction() + covered.startNavTransaction("/Receipt") + + fixture.incoming.dispose() + + assertThat(fixture.startedTransactions.map { it.name }).containsExactly("/Receipt") + assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransaction("/Receipt")) + } + + @Test + fun `a deferred transaction interrupted by a failed nav lease acquisition starts on the next one`() { + val fixture = Fixture.create() + val covered = fixture.coveredNavScopes() + covered.configureScope { it.screen = "/Result" } + covered.startNavTransaction("/Result") + val failure = IllegalStateException("observer failure") + var observerThrows = true + fixture.scope.options.addScopeObserver( + object : ScopeObserverAdapter() { + override fun setContexts(contexts: Contexts) { + if (observerThrows) throw failure + } + } + ) + val thrown = assertThrows(IllegalStateException::class.java) { fixture.incoming.dispose() } + assertThat(thrown).isSameInstanceAs(failure) + check(fixture.startedTransactions.isEmpty()) + observerThrows = false + val nextIncoming = fixture.navScopes() + nextIncoming.attach(fixture.lifecycle(Lifecycle.State.RESUMED)) + nextIncoming.applyFirstBackStackUpdate() + check(!fixture.hasLease(covered)) + + nextIncoming.dispose() + + assertThat(fixture.hasLease(covered)).isTrue() + assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransaction("/Result")) + } + + @Test + fun `a deferred transaction does not start after dispose`() { + val fixture = Fixture.create() + val covered = fixture.coveredNavScopes() + covered.startNavTransaction("/Result") + + covered.dispose() + fixture.incoming.dispose() + + assertThat(fixture.startedTransactions).isEmpty() + assertThat(fixture.scope.transaction).isNull() + } + + @Test + fun `a declined deferred start binds no transaction but still rotates the propagation context`() { + val fixture = Fixture.create() + val covered = fixture.coveredNavScopes() + val incomingContext = fixture.scope.propagationContext + covered.startNavTransaction("/Result") + fixture.declinesTransactions = true + + fixture.incoming.dispose() + + assertThat(fixture.scope.transaction).isNull() + assertThat(fixture.scope.propagationContext).isNotSameInstanceAs(incomingContext) + } + /** Starts a nav transaction for [name] through the [INavScope] handed out by [NavScopes]. */ private fun NavScopes.startNavTransaction(name: String): INavTransaction? { var transaction: INavTransaction? = null diff --git a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavTransactionTest.kt b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavTransactionTest.kt index 2977db6e5b0..1eca057f395 100644 --- a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavTransactionTest.kt +++ b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavTransactionTest.kt @@ -1,8 +1,12 @@ package io.sentry.compose.navigation3.lifecycle import com.google.common.truth.Truth.assertThat +import io.sentry.IScopes import io.sentry.ITransaction +import io.sentry.NoOpTransaction import io.sentry.SpanStatus +import io.sentry.TransactionContext +import io.sentry.TransactionOptions import io.sentry.protocol.App import io.sentry.protocol.Contexts import org.junit.Test @@ -11,10 +15,14 @@ import org.mockito.kotlin.inOrder import org.mockito.kotlin.mock import org.mockito.kotlin.never import org.mockito.kotlin.verify +import org.mockito.kotlin.verifyNoInteractions import org.mockito.kotlin.whenever class NavTransactionTest { + private val transactionContext = TransactionContext("/Checkout", "navigation") + private val transactionOptions = TransactionOptions() + private val app = App().apply { viewNames = listOf("/Checkout") } private val navigationContext = mapOf("Back Stack" to listOf(mapOf("entry" to "/Checkout"))) @@ -27,6 +35,20 @@ class NavTransactionTest { assertThat(handle.name).isEqualTo("/Checkout") } + @Test + fun `deferred() - is named after the transaction context`() { + val handle = NavTransaction.deferred(transactionContext, transactionOptions) + + assertThat(handle.name).isEqualTo("/Checkout") + } + + @Test + fun `transaction - is null for a deferred handle`() { + val handle = NavTransaction.deferred(transactionContext, transactionOptions) + + assertThat(handle.transaction).isNull() + } + @Test fun `transaction - is the backing transaction for a started handle`() { val fixture = Fixture.create() @@ -36,6 +58,35 @@ class NavTransactionTest { assertThat(handle.transaction).isSameInstanceAs(fixture.transaction) } + @Test + fun `transaction - is the started transaction once a deferred handle has started`() { + val fixture = Fixture.create() + val handle = NavTransaction.deferred(transactionContext, transactionOptions) + + handle.start(fixture.scopes) + + assertThat(handle.transaction).isSameInstanceAs(fixture.transaction) + } + + @Test + fun `transaction - stays null when the scopes decline to start a deferred handle's transaction`() { + val fixture = Fixture.create(declinesTransactions = true) + val handle = NavTransaction.deferred(transactionContext, transactionOptions) + + handle.start(fixture.scopes) + + assertThat(handle.transaction).isNull() + } + + @Test + fun `transaction - is null once a deferred handle is finished before it starts`() { + val handle = NavTransaction.deferred(transactionContext, transactionOptions) + + handle.finish(onFinished = {}) + + assertThat(handle.transaction).isNull() + } + @Test fun `a started handle sets data, contexts, and the app context on the transaction`() { val fixture = Fixture.create() @@ -50,6 +101,116 @@ class NavTransactionTest { assertThat(fixture.transaction.contexts.app).isSameInstanceAs(app) } + @Test + fun `a deferred handle sets data, contexts, and the app context on the transaction once it has started`() { + val fixture = Fixture.create() + val handle = NavTransaction.deferred(transactionContext, transactionOptions) + handle.start(fixture.scopes) + + handle.setData("arguments", "id=1") + handle.setContext(NAVIGATION_CONTEXT_KEY, navigationContext) + handle.setApp(app) + + verify(fixture.transaction).setData("arguments", "id=1") + verify(fixture.transaction).setContext(NAVIGATION_CONTEXT_KEY, navigationContext) + assertThat(fixture.transaction.contexts.app).isSameInstanceAs(app) + } + + @Test + fun `start() - starts and returns the transaction with the deferred context and options`() { + val fixture = Fixture.create() + val handle = NavTransaction.deferred(transactionContext, transactionOptions) + + val started = handle.start(fixture.scopes) + + verify(fixture.scopes).startTransaction(transactionContext, transactionOptions) + assertThat(started).isSameInstanceAs(fixture.transaction) + } + + @Test + fun `start() - replays data, contexts, and the app context set before the start`() { + val fixture = Fixture.create() + val handle = NavTransaction.deferred(transactionContext, transactionOptions) + handle.setData("arguments", "id=1") + handle.setContext(NAVIGATION_CONTEXT_KEY, navigationContext) + handle.setApp(app) + + handle.start(fixture.scopes) + + verify(fixture.transaction).setData("arguments", "id=1") + verify(fixture.transaction).setContext(NAVIGATION_CONTEXT_KEY, navigationContext) + assertThat(fixture.transaction.contexts.app).isSameInstanceAs(app) + } + + @Test + fun `start() - replays only the latest value set for each key`() { + val fixture = Fixture.create() + val handle = NavTransaction.deferred(transactionContext, transactionOptions) + val latestNavigationContext = mapOf("Back Stack" to listOf(mapOf("entry" to "/Receipt"))) + val latestApp = App().apply { viewNames = listOf("/Receipt") } + handle.setData("arguments", "id=1") + handle.setData("arguments", "id=2") + handle.setContext(NAVIGATION_CONTEXT_KEY, navigationContext) + handle.setContext(NAVIGATION_CONTEXT_KEY, latestNavigationContext) + handle.setApp(app) + handle.setApp(latestApp) + + handle.start(fixture.scopes) + + verify(fixture.transaction, never()).setData("arguments", "id=1") + verify(fixture.transaction).setData("arguments", "id=2") + verify(fixture.transaction, never()).setContext(NAVIGATION_CONTEXT_KEY, navigationContext) + verify(fixture.transaction).setContext(NAVIGATION_CONTEXT_KEY, latestNavigationContext) + assertThat(fixture.transaction.contexts.app).isSameInstanceAs(latestApp) + } + + @Test + fun `start() - returns null when the scopes decline to start a transaction`() { + val fixture = Fixture.create(declinesTransactions = true) + val handle = NavTransaction.deferred(transactionContext, transactionOptions) + + val started = handle.start(fixture.scopes) + + assertThat(started).isNull() + } + + @Test + fun `start() - starts the transaction on a retry after the scopes declined to start one`() { + val fixture = Fixture.create(declinesTransactions = true) + val handle = NavTransaction.deferred(transactionContext, transactionOptions) + handle.setData("arguments", "id=1") + handle.start(fixture.scopes) + fixture.declinesTransactions = false + + val started = handle.start(fixture.scopes) + + assertThat(started).isSameInstanceAs(fixture.transaction) + verify(fixture.transaction).setData("arguments", "id=1") + } + + @Test + fun `start() - starts no transaction a second time`() { + val fixture = Fixture.create() + val handle = NavTransaction.deferred(transactionContext, transactionOptions) + handle.start(fixture.scopes) + + val started = handle.start(fixture.scopes) + + assertThat(started).isNull() + verify(fixture.scopes).startTransaction(any(), any()) + } + + @Test + fun `start() - starts no transaction for a started handle`() { + val fixture = Fixture.create() + val handle = NavTransaction.started(fixture.transaction) + + val started = handle.start(fixture.scopes) + + assertThat(started).isNull() + verify(fixture.scopes, never()).startTransaction(any(), any()) + } + @Test fun `finish() - finishes a started handle's transaction with the transaction's status`() { val fixture = Fixture.create() @@ -108,17 +269,79 @@ class NavTransactionTest { verify(onFinished).invoke(fixture.transaction) } - private class Fixture private constructor(val transaction: ITransaction) { + @Test + fun `finish() - finishes a deferred handle's transaction once it has started`() { + val fixture = Fixture.create() + val onFinished = mock<(ITransaction) -> Unit>() + val handle = NavTransaction.deferred(transactionContext, transactionOptions) + handle.start(fixture.scopes) + + handle.finish(onFinished) + + verify(fixture.transaction).finish(SpanStatus.OK) + verify(onFinished).invoke(fixture.transaction) + } + + @Test + fun `finish() - discards a pending handle so that it never starts a transaction`() { + val fixture = Fixture.create() + val handle = NavTransaction.deferred(transactionContext, transactionOptions) + + handle.finish(onFinished = {}) + val started = handle.start(fixture.scopes) + + assertThat(started).isNull() + verify(fixture.scopes, never()).startTransaction(any(), any()) + } + + @Test + fun `finish() - does not call back for a pending handle`() { + val onFinished = mock<(ITransaction) -> Unit>() + val handle = NavTransaction.deferred(transactionContext, transactionOptions) + + handle.finish(onFinished) + + verifyNoInteractions(onFinished) + } + + @Test + fun `a discarded handle ignores later writes and finishes`() { + val fixture = Fixture.create() + val onFinished = mock<(ITransaction) -> Unit>() + val handle = NavTransaction.deferred(transactionContext, transactionOptions) + handle.finish(onFinished = {}) + + handle.setData("arguments", "id=1") + handle.setContext(NAVIGATION_CONTEXT_KEY, navigationContext) + handle.setApp(app) + handle.finish(onFinished) + val started = handle.start(fixture.scopes) + + assertThat(started).isNull() + verifyNoInteractions(fixture.transaction, onFinished) + } + + private class Fixture private constructor(val scopes: IScopes, val transaction: ITransaction) { + + /** Whether [scopes] declines to start transactions, returning a no-op transaction instead. */ + var declinesTransactions = false companion object { - fun create(): Fixture { + fun create(declinesTransactions: Boolean = false): Fixture { val contexts = Contexts() val transaction = mock().also { whenever(it.name).thenReturn("/Checkout") whenever(it.contexts).thenReturn(contexts) } - return Fixture(transaction) + val scopes = mock() + val fixture = Fixture(scopes, transaction) + fixture.declinesTransactions = declinesTransactions + whenever(scopes.startTransaction(any(), any())) + .thenAnswer { + if (fixture.declinesTransactions) NoOpTransaction.getInstance() else transaction + } + return fixture } } } diff --git a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavTransactionTest.kt b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavTransactionTest.kt index 8827eba4387..e732db751e9 100644 --- a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavTransactionTest.kt +++ b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavTransactionTest.kt @@ -132,6 +132,122 @@ class StagedNavTransactionTest { verify(started).finish(SpanStatus.OK) } + @Test + fun `stage() - returns a pending nav transaction without starting a transaction`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + + val navTransaction = stagedNavTransaction.stage(transactionContext, transactionOptions) + + assertThat(navTransaction.transaction).isNull() + assertThat(navTransaction.name).isEqualTo("/Profile") + assertThat(fixture.startedTransactions).isEmpty() + assertThat(fixture.scope.transaction).isNull() + } + + @Test + fun `stage() - replaces the current nav transaction without finishing it`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + val current = fixture.start("/Home") + + stagedNavTransaction.stage(transactionContext, transactionOptions) + stagedNavTransaction.finish(fixture.scope) + + verify(current, never()).finish(any()) + } + + @Test + fun `publishTo() - starts a staged nav transaction and binds it to a free scope`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + val navTransaction = stagedNavTransaction.stage(transactionContext, transactionOptions) + + stagedNavTransaction.publishTo(fixture.scope) + + val started = fixture.startedTransaction("/Profile") + assertThat(navTransaction.transaction).isSameInstanceAs(started) + assertThat(fixture.scope.transaction).isSameInstanceAs(started) + } + + @Test + fun `publishTo() - starts a staged nav transaction in place of a finished bound transaction`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + stagedNavTransaction.stage(transactionContext, transactionOptions) + fixture.scope.transaction = finishedTransaction + + stagedNavTransaction.publishTo(fixture.scope) + + assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransaction("/Profile")) + } + + @Test + fun `publishTo() - doesn't start a staged nav transaction while another transaction is bound`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + stagedNavTransaction.stage(transactionContext, transactionOptions) + fixture.scope.transaction = hostTransaction + + stagedNavTransaction.publishTo(fixture.scope) + + assertThat(fixture.startedTransactions).isEmpty() + assertThat(fixture.scope.transaction).isSameInstanceAs(hostTransaction) + } + + @Test + fun `publishTo() - logs the route when another transaction blocks the nav transaction`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + stagedNavTransaction.stage(transactionContext, transactionOptions) + fixture.scope.transaction = hostTransaction + + stagedNavTransaction.publishTo(fixture.scope) + + verify(fixture.logger).log(eq(SentryLevel.DEBUG), any(), eq("/Profile")) + } + + @Test + fun `publishTo() - discards a staged nav transaction blocked by another transaction`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + val navTransaction = stagedNavTransaction.stage(transactionContext, transactionOptions) + fixture.scope.transaction = hostTransaction + stagedNavTransaction.publishTo(fixture.scope) + fixture.scope.clearTransaction() + + stagedNavTransaction.publishTo(fixture.scope) + + assertThat(navTransaction.start(fixture.scopes)).isNull() + assertThat(fixture.startedTransactions).isEmpty() + assertThat(fixture.scope.transaction).isNull() + } + + @Test + fun `publishTo() - binds nothing when a staged nav transaction's start is declined`() { + val fixture = Fixture.create(declinesTransactions = true) + val stagedNavTransaction = fixture.stagedNavTransaction + stagedNavTransaction.stage(transactionContext, transactionOptions) + + stagedNavTransaction.publishTo(fixture.scope) + + assertThat(fixture.scope.transaction).isNull() + } + + @Test + fun `publishTo() - discards a staged nav transaction whose start was declined`() { + val fixture = Fixture.create(declinesTransactions = true) + val stagedNavTransaction = fixture.stagedNavTransaction + stagedNavTransaction.stage(transactionContext, transactionOptions) + stagedNavTransaction.publishTo(fixture.scope) + fixture.declinesTransactions = false + + stagedNavTransaction.publishTo(fixture.scope) + + assertThat(fixture.startedTransactions).isEmpty() + assertThat(fixture.scope.transaction).isNull() + } + @Test fun `publishTo() - binds an unpublished unfinished nav transaction again`() { val fixture = Fixture.create() @@ -247,6 +363,19 @@ class StagedNavTransactionTest { assertThat(fixture.scope.transaction).isSameInstanceAs(hostTransaction) } + @Test + fun `unpublishFrom() - keeps the bound transaction while the nav transaction is staged`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + val navTransaction = stagedNavTransaction.stage(transactionContext, transactionOptions) + fixture.scope.transaction = hostTransaction + + stagedNavTransaction.unpublishFrom(fixture.scope) + + assertThat(fixture.scope.transaction).isSameInstanceAs(hostTransaction) + assertThat(navTransaction.transaction).isNull() + } + @Test fun `finish() - finishes the started nav transaction and unbinds it`() { val fixture = Fixture.create() @@ -285,6 +414,20 @@ class StagedNavTransactionTest { assertThat(fixture.scope.transaction).isNull() } + @Test + fun `finish() - discards a staged nav transaction so that publishing never starts it`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + val navTransaction = stagedNavTransaction.stage(transactionContext, transactionOptions) + stagedNavTransaction.finish(fixture.scope) + + stagedNavTransaction.publishTo(fixture.scope) + + assertThat(navTransaction.start(fixture.scopes)).isNull() + assertThat(fixture.startedTransactions).isEmpty() + assertThat(fixture.scope.transaction).isNull() + } + @Test fun `finish() - forgets the nav transaction when unbinding it throws`() { val fixture = Fixture.create() @@ -326,6 +469,19 @@ class StagedNavTransactionTest { verify(started, never()).finish(any()) } + @Test + fun `forget() - forgets a staged nav transaction so that publishing never starts it`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + stagedNavTransaction.stage(transactionContext, transactionOptions) + stagedNavTransaction.forget() + + stagedNavTransaction.publishTo(fixture.scope) + + assertThat(fixture.startedTransactions).isEmpty() + assertThat(fixture.scope.transaction).isNull() + } + /** Thrown by a scope observer, to stand in for one the host app registered. */ private class ThrowingScopeObserverException : RuntimeException() diff --git a/sentry-samples/sentry-samples-android/src/main/java/io/sentry/samples/android/navigation/nav3/Nav3MultiActivity.kt b/sentry-samples/sentry-samples-android/src/main/java/io/sentry/samples/android/navigation/nav3/Nav3MultiActivity.kt index 5e6ca0b9d07..9f226a86e96 100644 --- a/sentry-samples/sentry-samples-android/src/main/java/io/sentry/samples/android/navigation/nav3/Nav3MultiActivity.kt +++ b/sentry-samples/sentry-samples-android/src/main/java/io/sentry/samples/android/navigation/nav3/Nav3MultiActivity.kt @@ -301,6 +301,7 @@ internal fun MultiRootNavDisplay( maxCapturedBackStackEntries = configuration.maxCapturedBackStackEntries } } + // Keep this before the NavDisplay below: TracedNav3Route's resume tagging relies on it. SentryNavEffect( backStack = backStack, backStackEntryMapper = { route -> diff --git a/sentry-samples/sentry-samples-android/src/main/java/io/sentry/samples/android/navigation/nav3/Nav3RouteScreens.kt b/sentry-samples/sentry-samples-android/src/main/java/io/sentry/samples/android/navigation/nav3/Nav3RouteScreens.kt index 64e3ee41f39..a67ed6d9d4e 100644 --- a/sentry-samples/sentry-samples-android/src/main/java/io/sentry/samples/android/navigation/nav3/Nav3RouteScreens.kt +++ b/sentry-samples/sentry-samples-android/src/main/java/io/sentry/samples/android/navigation/nav3/Nav3RouteScreens.kt @@ -46,6 +46,7 @@ import androidx.compose.ui.text.font.FontWeight import androidx.compose.ui.text.style.TextAlign import androidx.compose.ui.text.style.TextOverflow import androidx.compose.ui.unit.dp +import androidx.lifecycle.compose.LifecycleResumeEffect import io.sentry.ITransaction import io.sentry.Sentry import io.sentry.compose.SentryModifier.sentryTag @@ -80,6 +81,19 @@ internal fun TracedNav3Route( val hostName = LocalNav3SampleHost.current ?: activityName // Tag after composition is applied so abandoned compositions don't tag anything. SideEffect { tagCurrentNav3Scenario(route, scenario, hostName, activityName) } + // Tag again on resume: a root can get its nav transaction without composing (e.g., a ViewPager2 + // page composes while it's dragged into view, but only gets the nav lease and a transaction once + // it resumes). That transaction would otherwise go untagged and be missing from the history. + // + // This relies on the transaction starting before this effect runs. It does, as long as the root's + // SentryNavEffect starts observing the host's lifecycle before its NavDisplay does, i.e., it's + // composed first (see MultiRootNavDisplay) and isn't recreated afterwards (e.g., by new + // SentryNavOptions). The NavDisplay's scene and entry lifecycle owners are children of the host's + // lifecycle, so they only resume once its earlier observers have handled the resume. + LifecycleResumeEffect(route, scenario, hostName, activityName) { + tagCurrentNav3Scenario(route, scenario, hostName, activityName) + onPauseOrDispose {} + } SentryTraced( tag = "Nav3 /${route.routeName}", enableUserInteractionTracing = false,