From 867786beb0e54a81907bd8aa6157ed48908cdd34 Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Tue, 27 Oct 2026 19:14:18 +0100 Subject: [PATCH] fix(android): Defer Nav3 transactions requested without the nav lease A SentryNavEffect that navigates without the nav lease got no nav transaction for that destination. That covers a preloaded ViewPager2 page that's swiped to, and an owner that navigates while covered by another Activity or Fragment (e.g., a delayed push or a result callback) and is then returned to. Such a request is now staged and starts when the owner next acquires the lease, with the arguments, view names and nav context recorded in the meantime. The acquisition is its only chance to start: if another transaction is bound or the scopes decline it then, it's discarded rather than kept for a later acquisition. Swiping back to a page whose transaction has finished still starts none. The sample tags transactions again on resume, as a page can now get its transaction without composing. Refs JAVA-785 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 1 + .../lifecycle/NavLeaseCoordinator.kt | 11 + .../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(+), 67 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 85bbfc57c8..36c72632e0 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 9f8a0f87fd..55a7dfe9c8 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 @@ -133,6 +133,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 323656c8df..37a796a58c 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 cfbfd05f8f..c8c1464687 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 793aad3914..2851a10481 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 c742d3dd5b..cf653c23e3 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 7034ba29f6..d5467b10b9 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 32536d28cc..0ce82b832a 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 a079c91a54..57be6d68d5 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 0000000000..4f01197a0f --- /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 3226cbe162..c865e8e91e 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 2977db6e5b..1eca057f39 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 8827eba438..e732db751e 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 5e6ca0b9d0..9f226a86e9 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 64e3ee41f3..a67ed6d9d4 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,