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,