diff --git a/CHANGELOG.md b/CHANGELOG.md index d6df029830..f1064f34d2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ ### Behavioral Changes - Nav3: Returning to an Activity or Fragment with its own `SentryNavEffect` within the app (e.g., closing the Activity that covered it, swiping back to its ViewPager2 page, or dismissing a dialog over it) now starts a new navigation transaction for the current destination if the previous one finished, so spans produced upon return can now be consistently captured ([#6482](https://github.com/getsentry/sentry-java/pull/6482)) +- Nav3: `ui.load` transactions are bound to `IScope` in preference to Nav3 transactions when transitioning between Activities, matching Nav2 behavior (API 29+) ([#6484](https://github.com/getsentry/sentry-java/pull/6484)) ### Fixes diff --git a/sentry-android-navigation3/build.gradle.kts b/sentry-android-navigation3/build.gradle.kts index 7ad51b4390..1837bafa67 100644 --- a/sentry-android-navigation3/build.gradle.kts +++ b/sentry-android-navigation3/build.gradle.kts @@ -66,6 +66,7 @@ dependencies { compileOnly(libs.androidx.compose.runtime) compileOnly(libs.androidx.compose.ui) + testImplementation(projects.sentryAndroidCore) testImplementation(libs.androidx.compose.ui.test.junit4) testImplementation(libs.androidx.fragment.ktx) testImplementation(libs.androidx.test.core) diff --git a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavActivityWatcher.kt b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavActivityWatcher.kt index 0810c41f27..3c949384f5 100644 --- a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavActivityWatcher.kt +++ b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/NavActivityWatcher.kt @@ -3,14 +3,39 @@ package io.sentry.compose.navigation3.lifecycle import android.app.Activity import android.app.Application import android.app.Application.ActivityLifecycleCallbacks +import android.os.Build import android.os.Bundle /** * Watches every Activity of the host application and reports the Activity lifecycle events that - * [NavLeaseCoordinator] acts on. + * [NavLeaseCoordinator] acts on, so that the coordinator needn't know how they differ across API + * levels. * - * - [onActivityResumed] is called when any Activity resumes, including Activities without a - * `SentryNavEffect`. + * Each event is reported relative to `ActivityLifecycleIntegration`, which binds an Activity's + * `ui.load` transaction to the scope when the Activity is created and removes it when the Activity + * is destroyed: + * + * - [onActivityCreatedOrResumed] is called when an Activity is created and again when it resumes, + * with [ActivityEvent.CREATED] or [ActivityEvent.RESUMED] as its event. On API 29+, creation is + * reported before `ActivityLifecycleIntegration` tries to bind the Activity's `ui.load` + * transaction; below API 29, after. So below API 29, a withdrawal on creation comes too late for + * that `ui.load` transaction, which stays unbound (see "Non-candidate roots and lease withdrawal" + * in the package README). + * + * - [onActivityDestroyed] is called when an Activity is destroyed, after + * `ActivityLifecycleIntegration` has removed the Activity's `ui.load` transaction from the scope. + * + * **Android ordering assumptions** + * + * The watcher relies on the following callback orders, which Android doesn't document but AOSP has + * historically upheld: + * + * - `Application` calls `ActivityLifecycleCallbacks` in registration order. Below API 29, + * `ActivityLifecycleIntegration`'s callbacks run before this class's, provided the SDK is + * initialized before this class starts [watching][watch]. + * + * - On API 29+, every `onActivityPreCreated` callback runs before any `onActivityCreated` callback, + * and every `onActivityDestroyed` callback before any `onActivityPostDestroyed` callback. * * **Lifetime** * @@ -18,7 +43,19 @@ import android.os.Bundle * `defaultNavLeaseCoordinator`, which owns the watcher used in practice, they're meant to live as * long as the process. */ -internal class NavActivityWatcher(private val onActivityResumed: (activity: Activity) -> Unit) { +internal class NavActivityWatcher( + private val onActivityCreatedOrResumed: (activity: Activity, event: ActivityEvent) -> Unit, + private val onActivityDestroyed: (activity: Activity) -> Unit, +) { + + /** + * The Activity lifecycle event passed to [onActivityCreatedOrResumed], described by [description] + * in past tense (e.g., "created") for log messages. + */ + enum class ActivityEvent(val description: String) { + CREATED("created"), + RESUMED("resumed"), + } private var isWatching = false @@ -33,11 +70,23 @@ internal class NavActivityWatcher(private val onActivityResumed: (activity: Acti private inner class Callbacks : ActivityLifecycleCallbacks { - override fun onActivityResumed(activity: Activity) { - this@NavActivityWatcher.onActivityResumed(activity) + // Only called on API 29+, before any callback's onActivityCreated. + override fun onActivityPreCreated(activity: Activity, savedInstanceState: Bundle?) { + onActivityCreatedOrResumed(activity, ActivityEvent.CREATED) + } + + // Covers API levels below 29, as they don't call onActivityPreCreated. Runs after + // ActivityLifecycleIntegration's onActivityCreated, which finds the slot taken and never binds + // the Activity's ui.load transaction. This is a known limitation (see the package README). + override fun onActivityCreated(activity: Activity, savedInstanceState: Bundle?) { + if (Build.VERSION.SDK_INT < Build.VERSION_CODES.Q) { + onActivityCreatedOrResumed(activity, ActivityEvent.CREATED) + } } - override fun onActivityCreated(activity: Activity, savedInstanceState: Bundle?) = Unit + override fun onActivityResumed(activity: Activity) { + onActivityCreatedOrResumed(activity, ActivityEvent.RESUMED) + } override fun onActivityStarted(activity: Activity) = Unit @@ -47,6 +96,16 @@ internal class NavActivityWatcher(private val onActivityResumed: (activity: Acti override fun onActivitySaveInstanceState(activity: Activity, outState: Bundle) = Unit - override fun onActivityDestroyed(activity: Activity) = Unit + // Covers API levels below 29, which don't call onActivityPostDestroyed. + override fun onActivityDestroyed(activity: Activity) { + if (Build.VERSION.SDK_INT < Build.VERSION_CODES.Q) { + this@NavActivityWatcher.onActivityDestroyed(activity) + } + } + + // Only called on API 29+, after every callback's onActivityDestroyed. + override fun onActivityPostDestroyed(activity: Activity) { + this@NavActivityWatcher.onActivityDestroyed(activity) + } } } 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 e2791e3141..803a4bad66 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 @@ -57,9 +57,14 @@ internal class NavLeaseCoordinator { /** * Reports the host application's Activities to this coordinator so that it can detect whether a - * leaseholder is covered by a non-candidate root. + * leaseholder is covered by a non-candidate root, and retry a nav transaction blocked by a closed + * Activity's `ui.load` transaction. */ - private val activityWatcher = NavActivityWatcher(onActivityResumed = ::onAnyActivityResumed) + private val activityWatcher = + NavActivityWatcher( + onActivityCreatedOrResumed = ::onActivityCreatedOrResumed, + onActivityDestroyed = ::onAnyActivityDestroyed, + ) /** * Registers a [nav root][rootInfo] as a lease candidate until disposal or lifecycle destruction. @@ -307,14 +312,17 @@ internal class NavLeaseCoordinator { it.canReceiveLease && it.isActivityNavRoot(activity) } - /** Withdraws the lease when the resumed [activity] appears to cover a non-resumed nav root. */ - private fun onAnyActivityResumed(activity: Activity) { - // Assume the resumed `activity` is covering the current leaseholder if i) the leaseholder - // itself isn't resumed, ii) `activity` isn't the leaseholder's host Activity, and iii) - // `activity` isn't itself a nav root (see [isActivityNavRoot]). (i) checks the leaseholder's - // own state rather than its host Activity's, as they differ for Fragment nav roots: a Fragment - // leaseholder that isn't resumed isn't what the user is interacting with, even if its host - // Activity is resumed. + /** Withdraws the lease when [activity] appears to cover a non-resumed nav root. */ + private fun onActivityCreatedOrResumed( + activity: Activity, + event: NavActivityWatcher.ActivityEvent, + ) { + // Assume the created or resumed `activity` is covering the current leaseholder if i) the + // leaseholder itself isn't resumed, ii) `activity` isn't the leaseholder's host Activity, and + // iii) `activity` isn't itself a nav root (see [isActivityNavRoot]). (i) checks the + // leaseholder's own state rather than its host Activity's, as they differ for Fragment nav + // roots: a Fragment leaseholder that isn't resumed isn't what the user is interacting with, + // even if its host Activity is resumed. val holder = leaseHolder ?: return val hostActivity = holder.hostActivity ?: return if ( @@ -329,11 +337,28 @@ internal class NavLeaseCoordinator { withdrawLease( scope, withdrawalReason = - "${activity.javaClass.name} was resumed while the nav root wasn't resumed", + "${activity.javaClass.name} was ${event.description} while the nav root wasn't resumed", ) } } + /** + * After [activity] is destroyed, asks the leaseholder to retry publication if an Activity's + * `ui.load` blocked it. The publisher decides whether the blocker has left and retry is allowed. + */ + private fun onAnyActivityDestroyed(activity: Activity) { + val holder = leaseHolder ?: return + if (holder.hostActivity === activity) { + return + } + + holder.withScopeOrNull { scope -> + if (scope != null) { + holder.publisher.retryTransactionPublication(scope) + } + } + } + /** * A [nav root][rootInfo] eligible to hold the lease managed by [NavLeaseCoordinator]. * @@ -502,6 +527,12 @@ internal interface INavStatePublisher { /** Publishes the candidate's staged nav state to [scope]. */ fun publishTo(scope: IScope) + /** + * Retries a nav transaction blocked by an Activity's `ui.load`, subject to + * [StagedNavTransaction.retryPublishIfBlocked]. Leaves other published nav state alone. + */ + fun retryTransactionPublication(scope: IScope) + /** Removes published nav state from [scope], retaining it for a later lease acquisition. */ fun unpublishFrom(scope: IScope) 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 39568fa2a2..8bce50a60d 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 @@ -154,6 +154,10 @@ internal class NavScopes( stagedNavTransaction.publishTo(scope) } + override fun retryTransactionPublication(scope: IScope) { + stagedNavTransaction.retryPublishIfBlocked(scope) + } + override fun unpublishFrom(scope: IScope) { stagedNavTransaction.unpublishFrom(scope) stagedNavData.clearPublishedStateFrom(scope) @@ -228,7 +232,9 @@ internal class NavScopes( override fun startNavTransaction(request: NavTransactionRequest) { stagedNavTransaction.stage(request) if (hasLease) { - stagedNavTransaction.publishTo(delegate) + // Don't retry b/c ui.load retry is only for returns to a nav root, not for navigation + // within it. + stagedNavTransaction.publishTo(delegate, canRetry = false) } else { stagedNavData.deferPropagationContextRotation() } diff --git a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/README.md b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/README.md index 2b496552c9..e34b746f5e 100644 --- a/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/README.md +++ b/sentry-android-navigation3/src/main/kotlin/io/sentry/compose/navigation3/lifecycle/README.md @@ -153,8 +153,8 @@ leaving the transaction unbound but running until the leaseholder resumes. Non-candidate roots never register, so the coordinator infers their presence from either of two signals: -- another Activity resumes while the leaseholder isn't resumed, unless the resumed Activity is - itself a nav root, which takes the lease when it resumes; or +- another Activity is created or resumed while the leaseholder isn't resumed, unless the resumed + Activity is itself a nav root, which takes the lease when it resumes; or - the leaseholder pauses while its host Activity stays resumed (e.g., a ViewPager2 swipe). (Note that an Activity pauses before its Fragments do when the Activity is backgrounded or covered.) @@ -163,10 +163,19 @@ Non-candidate roots never register, so the coordinator infers their presence fro (e.g., dialogs) go undetected. These two signals are deliberately the only ones: other surfaces that cover a leaseholder without pausing it (e.g., bottom sheets or Compose dialogs) aren't detected, and shouldn't be by adding signals here. -**Limitation (before the covering Activity resumes)**: The lease is withdrawn only once the covering Activity resumes. Until then (i.e., -during its `onCreate` and `onStart`), errors and spans the covering Activity produces are still attributed to the leaseholder. For the same -reason, the covering Activity's `ui.load` transaction isn't bound to the scope: `ActivityLifecycleIntegration` tries to bind it when the -Activity is created, while the leaseholder's nav transaction still holds the scope's transaction slot. +**Limitation (below API 29)**: An Activity created over the leaseholder doesn't get its `ui.load` transaction bound to the scope. There's no +`onActivityPreCreated` below API 29, so the lease is withdrawn only after `ActivityLifecycleIntegration` has tried to bind the Activity's +`ui.load` transaction and found the leaseholder's nav transaction in the scope's transaction slot. The integration doesn't try again, so its +`ui.load` transaction stays unbound, as it did before withdrawal existed. Withdrawal still keeps the Activity's errors and spans from being +attributed to the leaseholder: spans the Activity produces attach to no transaction until one is bound (e.g., the nav transaction for the +first destination of the Activity's own `SentryNavEffect`, if it has one). Closing this gap would take a change to +`ActivityLifecycleIntegration` that affects every app, so it's deliberately left open. + +As a result, an Activity with its own `SentryNavEffect` gets a different transaction bound for its spans depending on the API level. On +API 29+, its `ui.load` transaction is bound, and its first destination's nav transaction isn't started because the slot is taken. Below +API 29, when the leaseholder's nav transaction held the slot as the Activity was created, its first destination's nav transaction is bound +instead. Either way, `ActivityLifecycleIntegration` still sends the `ui.load` transaction, including its TTID and TTFD spans. Only the +parent of auto-instrumented spans (e.g., HTTP or database spans) differs. ### Returning to a nav root @@ -185,6 +194,24 @@ Either way, the transaction is bound only if the scope's transaction slot is fre happen when the candidate publishes (`INavStatePublisher.publishTo`) its nav state, so the coordinator needn't tell a return from a first acquisition: on its first back stack update, a candidate has no destination to start a transaction for yet. +When the user returns by closing an Activity, that Activity's `ui.load` transaction may still be running and hold the slot. It normally +leaves the slot when the closed Activity is destroyed, which happens after the nav root has resumed. If the Activity is destroyed within +`UI_LOAD_RETRY_WINDOW_NANOS` of the blocked attempt, binding the nav root's transaction is retried once (see `onAnyActivityDestroyed`). +Otherwise, the nav root goes without a bound nav transaction until its next navigation or return. + +Only the current leaseholder is asked to retry when an Activity is destroyed. Its publisher checks whether the blocked `ui.load` has left +the slot and whether the retry window is still open. A leaseholder that loses or withdraws its lease forgets the blocked publication. + +The retry has three gaps: + +- A `ui.action` transaction started by the gesture that caused the return (e.g., a ViewPager2 swipe or an in-app back button) can block the + nav transaction. Only Activity `ui.load` transactions trigger this retry. +- Spans produced between the nav root's resume and the closed Activity's destruction may attach to the closed Activity's `ui.load`. +- An Activity left without being destroyed (e.g., when another Activity is reordered to the front) triggers no retry. + +Unbinding a finishing Activity's `ui.load` when it pauses would close the last two gaps, but would change `ActivityLifecycleIntegration` +for every app. Giving navigation priority over `ui.action` is an SDK-wide scope-slot policy decision shared with other integrations. + A nav root that's destroyed and recreated after losing the lease (e.g., on a configuration change) isn't returned to: it registers anew and gets a transaction on its first back stack update. See [#6454](https://github.com/getsentry/sentry-java/issues/6454). @@ -194,7 +221,8 @@ existing Activity comes back to the foreground. ### Android ordering assumptions -`NavLeaseCoordinator` relies on the following sequences, which Android doesn't document but AOSP and androidx have historically upheld: +Besides the callback ordering `NavActivityWatcher` relies on, `NavLeaseCoordinator` relies on the following sequences, which Android doesn't +document but AOSP and androidx have historically upheld: - An Activity's `onActivityResumed` callbacks run before its `Lifecycle` dispatches `ON_RESUME` (see `isActivityNavRoot`). @@ -203,5 +231,8 @@ existing Activity comes back to the foreground. lifecycles do. androidx's `ReportFragment` and `FragmentActivity` ensure this, not the platform. +- A visible Activity that finishes is destroyed once the Activity that replaces it reports idle, + or when the system's idle timeout fires (see `UI_LOAD_RETRY_WINDOW_NANOS`). + Going forward, if an order changes, nav data may be attributed to the wrong nav root, or a nav transaction may go unbound, but nothing would throw. 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 59916420c7..f5460d1a3e 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 @@ -3,8 +3,28 @@ package io.sentry.compose.navigation3.lifecycle import io.sentry.IScope import io.sentry.IScopes import io.sentry.ITransaction +import io.sentry.SentryDate import io.sentry.SentryLevel import io.sentry.SpanStatus +import java.lang.ref.WeakReference +import java.util.concurrent.TimeUnit + +/** + * The origin of the `ui.load` transactions `ActivityLifecycleIntegration` starts for Activities. + * + * Mirrors `ActivityLifecycleIntegration.TRACE_ORIGIN`, which isn't public. + */ +internal const val ACTIVITY_UI_LOAD_ORIGIN: String = "auto.ui.activity" + +/** + * Maximum time to retry a nav transaction if an Activity’s `ui.load` transaction blocks its + * publication. + * + * Android usually destroys a closed Activity once the returned-to Activity goes idle, removing its + * `ui.load` transaction from the shared [IScope]. A busy main thread can delay destruction past + * this window; in that case, the return gets no nav transaction until the next navigation. + */ +internal val UI_LOAD_RETRY_WINDOW_NANOS: Long = TimeUnit.SECONDS.toNanos(10) /** * Holds a nav transaction staged for publication to a shared [IScope]. @@ -26,12 +46,14 @@ import io.sentry.SpanStatus * * Navigation doesn't displace an unfinished transaction already bound to the shared `IScope`. This * includes `ui.load`, `ui.action`, and host-app transactions. Changing their relative priority - * requires an SDK-wide policy. + * requires an SDK-wide policy; this class only retries briefly after a closed Activity's `ui.load` + * releases the slot (see [retryPublishIfBlocked]). * * **Thread safety** * * This class is ***not*** thread-safe. Apply the thread safety policy from [NavScopes]. */ +@Suppress("TooManyFunctions") internal class StagedNavTransaction(private val scopes: IScopes) { /** @@ -47,6 +69,15 @@ internal class StagedNavTransaction(private val scopes: IScopes) { /** The transaction most recently started for [request], unless it has been forgotten since. */ private var transaction: ITransaction? = null + /** + * The most recent [publication][publishTo], if an Activity `ui.load` transaction kept it from + * binding a nav transaction. Cleared when the publication is retried, and when the nav + * transaction is next published, unpublished, finished, or forgotten. + * + * See [retryPublishIfBlocked]. + */ + private var blockedPublication: BlockedPublication? = null + /** * Stages [request], so that the next [publishTo] starts a transaction for it. * @@ -73,25 +104,57 @@ internal class StagedNavTransaction(private val scopes: IScopes) { * Publishes the staged transaction by binding it to [scope], or starts a transaction from the * staged request if needed. An occupied scope slot leaves the transaction unbound and the request * available for later publication. + * + * If [canRetry] is true and an Activity's `ui.load` transaction occupies the scope slot, the + * publication can be retried once the `ui.load` transaction leaves the scope (see + * [retryPublishIfBlocked]). */ - fun publishTo(scope: IScope) { + fun publishTo(scope: IScope, canRetry: Boolean = true) { + blockedPublication = null val request = request ?: return val started = transaction if (started != null && !started.isFinished) { if (scope.isFreeFor(request.name, started)) { bind(scope, started) + } else if (canRetry) { + blockedPublication = scope.blockedPublicationFor(started) } return } transaction = null started?.let { unbind(scope, it) } - startAndBind(scope, request) + // A start declined by the scopes (i.e., a no-op transaction) leaves the slot free, so it isn't + // retried. + if (startAndBind(scope, request) == null && canRetry) { + blockedPublication = scope.blockedPublicationFor(transaction = null) + } + } + + /** + * Retries publication to [scope] after the blocking Activity `ui.load` leaves its slot. Retries + * only within [UI_LOAD_RETRY_WINDOW_NANOS], and never arms another retry. Other blockers do not + * qualify. + */ + fun retryPublishIfBlocked(scope: IScope) { + val blocked = blockedPublication ?: return + if (scopes.options.dateProvider.now().diff(blocked.blockedAt) > UI_LOAD_RETRY_WINDOW_NANOS) { + blockedPublication = null + return + } + val uiLoad = blocked.uiLoad.get() + if (uiLoad != null && !uiLoad.isFinished && scope.transaction === uiLoad) { + // The ui.load transaction still holds the slot; wait for it to leave. + return + } + + publishTo(scope, canRetry = false) } /** Unbinds the transaction from [scope] without finishing it, allowing later publication. */ fun unpublishFrom(scope: IScope) { + blockedPublication = null transaction?.let { unbind(scope, it) } } @@ -101,6 +164,7 @@ internal class StagedNavTransaction(private val scopes: IScopes) { */ fun finish(scope: IScope) { request = null + blockedPublication = null val transaction = transaction ?: return // Forget the transaction before unbinding it: unbinding notifies the host app's scope // observers, which can throw, and a transaction left behind would be bound again later. @@ -115,6 +179,7 @@ internal class StagedNavTransaction(private val scopes: IScopes) { fun forget() { request = null transaction = null + blockedPublication = null } /** Whether the slot is empty, holds a finished transaction, or already holds [transaction]. */ @@ -173,4 +238,27 @@ internal class StagedNavTransaction(private val scopes: IScopes) { ) return false } + + /** + * Returns a blocker only when this scope holds an Activity `ui.load` instead of [transaction]. + * Pass `null` to check whether it blocks a new transaction. + */ + private fun IScope.blockedPublicationFor(transaction: ITransaction?): BlockedPublication? { + val current = this.transaction ?: return null + if (canHold(current, transaction) || current.spanContext.origin != ACTIVITY_UI_LOAD_ORIGIN) { + return null + } + return BlockedPublication(WeakReference(current), scopes.options.dateProvider.now()) + } + + /** + * A nav publication blocked by [uiLoad] at [blockedAt]. + * + * The `ui.load` transaction is held weakly, as it's only compared with the scope's transaction. + * (A collected transaction can't be bound to the scope anymore.) + */ + private class BlockedPublication( + val uiLoad: WeakReference, + val blockedAt: SentryDate, + ) } 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 6164f479ba..2b8547b408 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 @@ -522,6 +522,8 @@ class CoordinatedBackStackObserverTest { private object NoOpNavStatePublisher : INavStatePublisher { override fun publishTo(scope: IScope) = Unit + override fun retryTransactionPublication(scope: IScope) = Unit + override fun unpublishFrom(scope: IScope) = Unit override fun onUnregistered(scope: IScope?) = Unit diff --git a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/SentryNavEffectLifecycleIntegrationTest.kt b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/SentryNavEffectLifecycleIntegrationTest.kt index 2a9138b93e..c46fe6a99a 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 @@ -27,11 +27,19 @@ import androidx.viewpager2.adapter.FragmentStateAdapter import androidx.viewpager2.widget.ViewPager2 import com.google.common.truth.Truth.assertThat import io.sentry.IScope +import io.sentry.IScopes import io.sentry.ISentryClient +import io.sentry.ITransaction 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 io.sentry.android.core.ActivityFramesTracker +import io.sentry.android.core.ActivityLifecycleIntegration +import io.sentry.android.core.BuildInfoProvider +import io.sentry.android.core.SentryAndroidOptions +import io.sentry.android.core.performance.AppStartMetrics import io.sentry.compose.navigation3.lifecycle.NAVIGATION_CONTEXT_KEY import io.sentry.compose.navigation3.lifecycle.NavLeaseCoordinator import org.junit.After @@ -41,6 +49,7 @@ import org.mockito.kotlin.mock import org.mockito.kotlin.whenever import org.robolectric.Robolectric import org.robolectric.Shadows.shadowOf +import org.robolectric.android.controller.ActivityController import org.robolectric.annotation.Config /** @@ -62,6 +71,7 @@ class SentryNavEffectLifecycleIntegrationTest { @After fun tearDown() { + Fixture.current?.close() Fixture.current = null } @@ -89,6 +99,63 @@ class SentryNavEffectLifecycleIntegrationTest { .inOrder() } + /** + * On API 29+, core's `onActivityPreCreated` callback can run while the previous Activity's Nav3 + * transaction is still bound to the scope. Our Nav3 implementation releases it in its own + * `onActivityPreCreated` callback. Android runs all of those callbacks before + * `onActivityCreated`, when core binds the incoming Activity's `ui.load` transaction, so it takes + * the freed slot. + */ + @Test + fun `an incoming activity binds its own ui load transaction instead of its first nav transaction`() { + val fixture = Fixture.create() + fixture.installActivityLifecycleIntegration(CheckoutActivity::class.java) + val home = Robolectric.buildActivity(HomeActivity::class.java).setup() + fixture.idle() + val homeTransaction = requireNotNull(fixture.scope.transaction) + + home.pause() + Robolectric.buildActivity(CheckoutActivity::class.java).setup() + fixture.idle() + + val uiLoadTransaction = fixture.uiLoadTransactions.single() + assertThat(uiLoadTransaction.operation).isEqualTo("ui.load") + assertThat(fixture.scope.transaction).isSameInstanceAs(uiLoadTransaction) + assertThat(homeTransaction.isFinished).isFalse() + assertThat(fixture.scope.screen).isEqualTo("/Checkout") + assertThat(fixture.scope.navigationEntries()).containsExactly("/Checkout") + } + + /** + * Below API 29, there's no `onActivityPreCreated`, so the outgoing Activity's lease is withdrawn + * from `onActivityCreated` if `ActivityLifecycleIntegration` finds that the scope's transaction + * slot is taken. `ActivityLifecycleIntegration` doesn't try again, so its `ui.load` transaction + * remains unbound. The incoming Activity's first nav transaction takes the freed slot instead (a + * known limitation – see `NavLeaseCoordinator`). + */ + @Test + @Config(sdk = [28]) + fun `below API 29, first nav transaction binds when incoming ui load cannot`() { + val fixture = Fixture.create() + fixture.installActivityLifecycleIntegration(CheckoutActivity::class.java) + val home = Robolectric.buildActivity(HomeActivity::class.java).setup() + fixture.idle() + val homeTransaction = requireNotNull(fixture.scope.transaction) + + home.pause() + Robolectric.buildActivity(CheckoutActivity::class.java).setup() + fixture.idle() + + val uiLoadTransaction = fixture.uiLoadTransactions.single() + assertThat(uiLoadTransaction.isFinished).isFalse() + val checkoutTransaction = requireNotNull(fixture.scope.transaction) + assertThat(checkoutTransaction.name).isEqualTo("/Checkout") + assertThat(checkoutTransaction.operation).isEqualTo("navigation") + assertThat(homeTransaction.isFinished).isFalse() + assertThat(fixture.scope.screen).isEqualTo("/Checkout") + assertThat(fixture.scope.navigationEntries()).containsExactly("/Checkout") + } + @Test fun `an incoming activity's nav data and transaction survive the outgoing activity stopping`() { val fixture = Fixture.create() @@ -215,6 +282,85 @@ class SentryNavEffectLifecycleIntegrationTest { .inOrder() } + /** + * Known limitation: until the closing Activity is destroyed, its unfinished `ui.load` transaction + * keeps the scope's transaction slot, so the Activity returned to has no nav transaction bound + * yet, and spans produced on return attach to the closing Activity's `ui.load` instead. + */ + @Test + fun `going back keeps the covering activity's unfinished ui load bound until it's destroyed`() { + val fixture = Fixture.create() + fixture.returnToHomeWhileCheckoutUiLoadIsUnfinished() + val uiLoadTransaction = fixture.uiLoadTransactions.single() + + assertThat(fixture.scope.transaction).isSameInstanceAs(uiLoadTransaction) + assertThat(fixture.scope.screen).isEqualTo("/Home") + } + + @Test + fun `going back during the covering activity's ui load starts a nav transaction once it's destroyed`() { + val fixture = Fixture.create() + val (_, checkout) = fixture.returnToHomeWhileCheckoutUiLoadIsUnfinished() + + checkout.stop() + checkout.destroy() + fixture.idle() + + val restarted = requireNotNull(fixture.scope.transaction) + assertThat(restarted.name).isEqualTo("/Home") + assertThat(restarted.operation).isEqualTo("navigation") + assertThat(fixture.uiLoadTransactions.single().isFinished).isTrue() + assertThat(fixture.scope.screen).isEqualTo("/Home") + } + + /** + * Below API 29, the covering Activity's `ui.load` transaction only binds if the scope's + * transaction slot is free when the Activity is created (see `NavLeaseCoordinator`), e.g., + * because Home's nav transaction has already finished. + */ + @Test + @Config(sdk = [28]) + fun `below API 29, going back during the covering activity's ui load starts a nav transaction once it's destroyed`() { + val fixture = Fixture.create() + fixture.installActivityLifecycleIntegration(CheckoutActivity::class.java) + val home = Robolectric.buildActivity(HomeActivity::class.java).setup() + fixture.idle() + requireNotNull(fixture.scope.transaction).finish() + home.pause() + val checkout = Robolectric.buildActivity(CheckoutActivity::class.java).setup() + fixture.idle() + home.stop() + checkout.pause() + home.restart() + home.resume() + fixture.idle() + assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.uiLoadTransactions.single()) + + checkout.stop() + checkout.destroy() + fixture.idle() + + val restarted = requireNotNull(fixture.scope.transaction) + assertThat(restarted.name).isEqualTo("/Home") + assertThat(restarted.operation).isEqualTo("navigation") + assertThat(fixture.scope.screen).isEqualTo("/Home") + } + + @Test + fun `going back during the covering activity's ui load rebinds the unfinished nav transaction once it's destroyed`() { + val fixture = Fixture.create() + val (_, checkout) = + fixture.returnToHomeWhileCheckoutUiLoadIsUnfinished(finishHomeTransaction = false) + val homeTransaction = fixture.homeTransaction + + checkout.stop() + checkout.destroy() + fixture.idle() + + assertThat(fixture.scope.transaction).isSameInstanceAs(homeTransaction) + assertThat(homeTransaction.isFinished).isFalse() + } + // Activity recreation @Test fun `a recreated activity publishes its own nav data and transaction over the destroyed instance's`() { @@ -867,6 +1013,59 @@ class SentryNavEffectLifecycleIntegrationTest { assertThat(homeTransaction.isFinished).isFalse() } + /** + * `ActivityLifecycleIntegration` binds an Activity's `ui.load` transaction when the Activity is + * created, and only if the scope's transaction slot is free. + */ + @Test + fun `an activity without an effect binds its own ui load transaction over a covered activity's nav transaction`() { + val fixture = Fixture.create() + fixture.installActivityLifecycleIntegration(PlainActivity::class.java) + val home = Robolectric.buildActivity(HomeActivity::class.java).setup() + fixture.idle() + val homeTransaction = requireNotNull(fixture.scope.transaction) + + home.pause() + Robolectric.buildActivity(PlainActivity::class.java).setup() + fixture.idle() + + val uiLoadTransaction = fixture.uiLoadTransactions.single() + assertThat(uiLoadTransaction.operation).isEqualTo("ui.load") + assertThat(fixture.scope.screen).isEqualTo(PlainActivity::class.java.canonicalName) + assertThat(fixture.scope.navigationEntries()).isNull() + assertThat(fixture.scope.transaction).isSameInstanceAs(uiLoadTransaction) + assertThat(homeTransaction.isFinished).isFalse() + } + + /** + * Below API 29, there's no `onActivityPreCreated`, so the lease is withdrawn from + * `onActivityCreated`, after `ActivityLifecycleIntegration` has found the scope's transaction + * slot taken. `ActivityLifecycleIntegration` doesn't try again, so its `ui.load` transaction + * stays unbound (a known limitation – see `NavLeaseCoordinator`). The covered Activity's nav data + * and transaction are still removed, so they aren't attributed to the Activity. The screen + * `ActivityLifecycleIntegration` set for the Activity is left in place, as it isn't the covered + * Activity's. + */ + @Test + @Config(sdk = [28]) + fun `below API 29, activity without an effect leaves no transaction bound`() { + val fixture = Fixture.create() + fixture.installActivityLifecycleIntegration(PlainActivity::class.java) + val home = Robolectric.buildActivity(HomeActivity::class.java).setup() + fixture.idle() + val homeTransaction = requireNotNull(fixture.scope.transaction) + + home.pause() + Robolectric.buildActivity(PlainActivity::class.java).setup() + fixture.idle() + + assertThat(fixture.uiLoadTransactions.single().isFinished).isFalse() + assertThat(fixture.scope.screen).isEqualTo(PlainActivity::class.java.canonicalName) + assertThat(fixture.scope.navigationEntries()).isNull() + assertThat(fixture.scope.transaction).isNull() + assertThat(homeTransaction.isFinished).isFalse() + } + @Test fun `going back to an activity covered by one without an effect restores its nav data and unfinished transaction`() { val fixture = Fixture.create() @@ -1242,7 +1441,7 @@ class SentryNavEffectLifecycleIntegrationTest { private class Fixture private constructor(tracesSampleRate: Double) { val options = - SentryOptions().apply { + SentryAndroidOptions().apply { dsn = "http://key@localhost/1" defaultScopeType = ScopeType.CURRENT setTracesSampleRate(tracesSampleRate) @@ -1283,6 +1482,74 @@ class SentryNavEffectLifecycleIntegrationTest { events += "$name:$event" } + /** `ui.load` transactions started by [installActivityLifecycleIntegration], in start order. */ + val uiLoadTransactions = mutableListOf() + + /** Home's nav transaction, as set up by [returnToHomeWhileCheckoutUiLoadIsUnfinished]. */ + lateinit var homeTransaction: ITransaction + + /** + * Opens Checkout over Home, binding Checkout's `ui.load` transaction, then returns to Home + * while that `ui.load` transaction is unfinished: Checkout is paused and Home resumed, but + * Checkout isn't stopped or destroyed yet. Finishes Home's nav transaction while Checkout + * covers it unless [finishHomeTransaction] is `false`. + * + * API 29+ only: below API 29, Checkout's `ui.load` transaction can't bind while Home's nav + * transaction holds the scope's transaction slot. + */ + fun returnToHomeWhileCheckoutUiLoadIsUnfinished( + finishHomeTransaction: Boolean = true + ): Pair, ActivityController> { + installActivityLifecycleIntegration(CheckoutActivity::class.java) + val home = Robolectric.buildActivity(HomeActivity::class.java).setup() + idle() + homeTransaction = requireNotNull(scope.transaction) + home.pause() + val checkout = Robolectric.buildActivity(CheckoutActivity::class.java).setup() + idle() + home.stop() + idle() + if (finishHomeTransaction) { + homeTransaction.finish() + } + + checkout.pause() + home.restart() + home.resume() + idle() + return home to checkout + } + + private var activityLifecycleIntegration: ActivityLifecycleIntegration? = null + + /** + * Installs a real [ActivityLifecycleIntegration] that traces only instances of + * [tracedActivityClass], so that each such Activity gets a `ui.load` transaction that competes + * with nav transactions for the scope's transaction slot as it does in an app. + * + * Call it before building any Activity: the SDK registers the integration at initialization, so + * its callbacks run before those the coordinator registers when the first effect attaches. + * + * Limiting it to [tracedActivityClass] keeps the other Activities' nav transactions bindable, + * as the tests expect. + */ + fun installActivityLifecycleIntegration(tracedActivityClass: Class) { + val application: Application = ApplicationProvider.getApplicationContext() + val integration = + ActivityLifecycleIntegration( + TracedActivityApplication(application, tracedActivityClass), + BuildInfoProvider(options.logger), + mock(), + ) + integration.register(UiLoadRecordingScopes(scopes, uiLoadTransactions), options) + activityLifecycleIntegration = integration + } + + fun close() { + activityLifecycleIntegration?.close() + AppStartMetrics.getInstance().clear() + } + /** * The host-app setup under test: one `SentryNavEffect` in an Activity's or Fragment's content. */ @@ -1323,6 +1590,123 @@ class SentryNavEffectLifecycleIntegrationTest { } } + /** + * Stands in for [application] when [ActivityLifecycleIntegration] registers its callbacks, and + * relays the application's Activity lifecycle callbacks to it only for instances of + * [tracedActivityClass]. + */ + private class TracedActivityApplication( + private val application: Application, + private val tracedActivityClass: Class, + ) : Application() { + + private val relays = mutableMapOf() + + override fun registerActivityLifecycleCallbacks(callback: ActivityLifecycleCallbacks) { + val relay = TracedActivityCallbacks(callback, tracedActivityClass) + relays[callback] = relay + application.registerActivityLifecycleCallbacks(relay) + } + + override fun unregisterActivityLifecycleCallbacks(callback: ActivityLifecycleCallbacks) { + relays.remove(callback)?.let { application.unregisterActivityLifecycleCallbacks(it) } + } + } + + /** Relays each callback to [delegate] only for instances of [tracedActivityClass]. */ + @Suppress("TooManyFunctions") + private class TracedActivityCallbacks( + private val delegate: Application.ActivityLifecycleCallbacks, + private val tracedActivityClass: Class, + ) : Application.ActivityLifecycleCallbacks { + + private inline fun relay(activity: Activity, block: () -> Unit) { + if (tracedActivityClass.isInstance(activity)) { + block() + } + } + + override fun onActivityPreCreated(activity: Activity, savedInstanceState: Bundle?) = + relay(activity) { delegate.onActivityPreCreated(activity, savedInstanceState) } + + override fun onActivityCreated(activity: Activity, savedInstanceState: Bundle?) = + relay(activity) { delegate.onActivityCreated(activity, savedInstanceState) } + + override fun onActivityPostCreated(activity: Activity, savedInstanceState: Bundle?) = + relay(activity) { delegate.onActivityPostCreated(activity, savedInstanceState) } + + override fun onActivityPreStarted(activity: Activity) = + relay(activity) { delegate.onActivityPreStarted(activity) } + + override fun onActivityStarted(activity: Activity) = + relay(activity) { delegate.onActivityStarted(activity) } + + override fun onActivityPostStarted(activity: Activity) = + relay(activity) { delegate.onActivityPostStarted(activity) } + + override fun onActivityPreResumed(activity: Activity) = + relay(activity) { delegate.onActivityPreResumed(activity) } + + override fun onActivityResumed(activity: Activity) = + relay(activity) { delegate.onActivityResumed(activity) } + + override fun onActivityPostResumed(activity: Activity) = + relay(activity) { delegate.onActivityPostResumed(activity) } + + override fun onActivityPrePaused(activity: Activity) = + relay(activity) { delegate.onActivityPrePaused(activity) } + + override fun onActivityPaused(activity: Activity) = + relay(activity) { delegate.onActivityPaused(activity) } + + override fun onActivityPostPaused(activity: Activity) = + relay(activity) { delegate.onActivityPostPaused(activity) } + + override fun onActivityPreStopped(activity: Activity) = + relay(activity) { delegate.onActivityPreStopped(activity) } + + override fun onActivityStopped(activity: Activity) = + relay(activity) { delegate.onActivityStopped(activity) } + + override fun onActivityPostStopped(activity: Activity) = + relay(activity) { delegate.onActivityPostStopped(activity) } + + override fun onActivityPreSaveInstanceState(activity: Activity, outState: Bundle) = + relay(activity) { delegate.onActivityPreSaveInstanceState(activity, outState) } + + override fun onActivitySaveInstanceState(activity: Activity, outState: Bundle) = + relay(activity) { delegate.onActivitySaveInstanceState(activity, outState) } + + override fun onActivityPostSaveInstanceState(activity: Activity, outState: Bundle) = + relay(activity) { delegate.onActivityPostSaveInstanceState(activity, outState) } + + override fun onActivityPreDestroyed(activity: Activity) = + relay(activity) { delegate.onActivityPreDestroyed(activity) } + + override fun onActivityDestroyed(activity: Activity) = + relay(activity) { delegate.onActivityDestroyed(activity) } + + override fun onActivityPostDestroyed(activity: Activity) = + relay(activity) { delegate.onActivityPostDestroyed(activity) } + } + + /** Records each `ui.load` transaction started through [delegate] in [uiLoadTransactions]. */ + private class UiLoadRecordingScopes( + private val delegate: IScopes, + private val uiLoadTransactions: MutableList, + ) : IScopes by delegate { + + override fun startTransaction( + transactionContext: TransactionContext, + transactionOptions: TransactionOptions, + ): ITransaction = + delegate.startTransaction(transactionContext, transactionOptions).also { + if (transactionContext.operation == "ui.load") { + uiLoadTransactions += it + } + } + } + private companion object { fun navEffectView(context: Context, route: String): View { diff --git a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinatorTest.kt b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinatorTest.kt index ae13a38d27..ec24919b23 100644 --- a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinatorTest.kt +++ b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/NavLeaseCoordinatorTest.kt @@ -743,6 +743,62 @@ class NavLeaseCoordinatorTest { // region lease withdrawal when another Activity covers the leaseholder + @Test + fun `another activity being pre-created while the leaseholder is paused withdraws the nav lease`() { + val fixture = Fixture.create() + val owner = TestOwner(RESUMED) + val registration = fixture.acquireLease("owner", owner, host = fixture.hostActivity) + owner.lifecycle.currentState = STARTED + + fixture.activityCallbacks.onActivityPreCreated(fixture.coveringActivity, null) + + assertThat(registration.hasLease).isFalse() + assertThat(fixture.unpublished).containsExactly("owner") + } + + @Test + fun `another activity being created while the leaseholder is paused withdraws the lease and unpublishes nav state`() { + val fixture = Fixture.create() + val owner = TestOwner(RESUMED) + val registration = fixture.acquireLease("owner", owner, host = fixture.hostActivity) + owner.lifecycle.currentState = STARTED + + fixture.activityCallbacks.onActivityCreated(fixture.coveringActivity, null) + + assertThat(registration.hasLease).isFalse() + assertThat(fixture.unpublished).containsExactly("owner") + } + + @Test + fun `another activity being created while the leaseholder is resumed keeps the nav lease`() { + val fixture = Fixture.create() + val registration = + fixture.acquireLease("owner", TestOwner(RESUMED), host = fixture.hostActivity) + + fixture.activityCallbacks.onActivityCreated(fixture.coveringActivity, null) + + assertThat(registration.hasLease).isTrue() + assertThat(fixture.unpublished).isEmpty() + } + + @Test + fun `another activity being created while the leaseholder is paused logs the withdrawal`() { + val fixture = Fixture.create() + val owner = TestOwner(RESUMED) + fixture.acquireLease("owner", owner, host = fixture.hostActivity) + owner.lifecycle.currentState = STARTED + + fixture.activityCallbacks.onActivityCreated(fixture.coveringActivity, null) + + verify(fixture.logger) + .log( + SentryLevel.DEBUG, + "Withdrew the Nav3 lease from a nav root hosted by %s: %s.", + fixture.hostActivity.javaClass.name, + "${fixture.coveringActivity.javaClass.name} was created while the nav root wasn't resumed", + ) + } + @Test fun `another activity resuming while the leaseholder is paused withdraws the nav lease`() { val fixture = Fixture.create() @@ -1024,6 +1080,66 @@ class NavLeaseCoordinatorTest { // endregion + // region retrying nav transaction publication when an Activity is destroyed + + @Test + fun `another activity being post-destroyed retries the leaseholder's transaction publication`() { + val fixture = Fixture.create() + fixture.acquireLease("owner", TestOwner(RESUMED), host = fixture.hostActivity) + + fixture.activityCallbacks.onActivityPostDestroyed(fixture.coveringActivity) + + assertThat(fixture.retried).containsExactly("owner") + } + + /** Unit tests run against a stubbed `Build.VERSION.SDK_INT` of 0, i.e., below API 29. */ + @Test + fun `below API 29, another activity being destroyed retries the leaseholder's transaction publication`() { + val fixture = Fixture.create() + fixture.acquireLease("owner", TestOwner(RESUMED), host = fixture.hostActivity) + + fixture.activityCallbacks.onActivityDestroyed(fixture.coveringActivity) + + assertThat(fixture.retried).containsExactly("owner") + } + + @Test + fun `the leaseholder's host activity being destroyed doesn't retry its transaction publication`() { + val fixture = Fixture.create() + fixture.acquireLease("owner", TestOwner(RESUMED), host = fixture.hostActivity) + + fixture.activityCallbacks.onActivityPostDestroyed(fixture.hostActivity) + + assertThat(fixture.retried).isEmpty() + } + + @Test + fun `an activity being destroyed without a leaseholder retries no transaction publication`() { + val fixture = Fixture.create() + val owner = TestOwner(RESUMED) + fixture.acquireLease("owner", owner, host = fixture.hostActivity) + owner.lifecycle.currentState = STARTED + fixture.activityCallbacks.onActivityCreated(fixture.coveringActivity, null) + + fixture.activityCallbacks.onActivityPostDestroyed(fixture.coveringActivity) + + assertThat(fixture.retried).isEmpty() + } + + @Test + fun `an activity being destroyed without a scope retries no transaction publication`() { + val fixture = Fixture.create() + fixture.acquireLease("owner", TestOwner(RESUMED), host = fixture.hostActivity) + + fixture.withoutScope { + fixture.activityCallbacks.onActivityPostDestroyed(fixture.coveringActivity) + } + + assertThat(fixture.retried).isEmpty() + } + + // endregion + // region lease withdrawal when the leaseholder pauses within a resumed Activity @Test @@ -1433,6 +1549,9 @@ class NavLeaseCoordinatorTest { /** Names of the nav states unpublished, in the order they were unpublished. */ val unpublished = mutableListOf() + /** Names of the nav states asked to retry publishing their nav transactions, in order. */ + val retried = mutableListOf() + /** Names of the nav states notified of unregistration, with the scope they were given. */ val unregistered = mutableListOf>() @@ -1444,14 +1563,22 @@ class NavLeaseCoordinatorTest { /** * Publisher named [name] that, by default, records its publication in [published], its - * unpublishing in [unpublished], and its unregistration in [unregistered]. + * transaction publication retries in [retried], its unpublishing in [unpublished], and its + * unregistration in [unregistered]. */ fun publisher( name: String, onPublishTo: (IScope) -> Unit = { published += name }, + onRetryTransactionPublication: (IScope) -> Unit = { retried += name }, onUnpublishFrom: (IScope) -> Unit = { unpublished += name }, onUnregistered: (IScope?) -> Unit = { unregistered += name to it }, - ): INavStatePublisher = FakeNavStatePublisher(onPublishTo, onUnpublishFrom, onUnregistered) + ): INavStatePublisher = + FakeNavStatePublisher( + onPublishTo, + onRetryTransactionPublication, + onUnpublishFrom, + onUnregistered, + ) /** The application of [hostActivity] and [coveringActivity]. */ val application = mock() @@ -1532,11 +1659,14 @@ class NavLeaseCoordinatorTest { private class FakeNavStatePublisher( private val onPublishTo: (IScope) -> Unit, + private val onRetryTransactionPublication: (IScope) -> Unit, private val onUnpublishFrom: (IScope) -> Unit, private val onUnregistered: (IScope?) -> Unit, ) : INavStatePublisher { override fun publishTo(scope: IScope) = onPublishTo(scope) + override fun retryTransactionPublication(scope: IScope) = onRetryTransactionPublication(scope) + override fun unpublishFrom(scope: IScope) = onUnpublishFrom(scope) override fun onUnregistered(scope: IScope?) = onUnregistered.invoke(scope) diff --git a/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavTransactionTest.kt b/sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/lifecycle/StagedNavTransactionTest.kt index 0488c7d1c1..5072e35ccf 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 @@ -7,12 +7,15 @@ import io.sentry.ITransaction import io.sentry.NoOpTransaction import io.sentry.Scope import io.sentry.ScopeObserverAdapter +import io.sentry.SentryDateProvider import io.sentry.SentryLevel +import io.sentry.SentryNanotimeDate import io.sentry.SentryOptions import io.sentry.SpanContext import io.sentry.SpanStatus import io.sentry.TransactionContext import io.sentry.TransactionOptions +import java.util.Date import org.junit.Assert.assertThrows import org.junit.Test import org.mockito.kotlin.any @@ -31,6 +34,7 @@ class StagedNavTransactionTest { private val finishedTransaction = mock().also { whenever(it.isFinished).thenReturn(true) } private val hostTransaction = transactionWithOrigin("manual") + private val uiLoadTransaction = transactionWithOrigin(ACTIVITY_UI_LOAD_ORIGIN) @Test fun `publishTo() - keeps the started nav transaction when binding it throws`() { @@ -433,6 +437,237 @@ class StagedNavTransactionTest { assertThat(fixture.scope.transaction).isSameInstanceAs(hostTransaction) } + @Test + fun `retryPublishIfBlocked() - starts a nav transaction blocked by a ui load once it leaves the slot`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + stagedNavTransaction.stage(request) + fixture.scope.transaction = uiLoadTransaction + stagedNavTransaction.publishTo(fixture.scope) + fixture.scope.clearTransaction() + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransaction("/Profile")) + } + + @Test + fun `retryPublishIfBlocked() - binds an unfinished nav transaction blocked by a ui load once it leaves the slot`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + val started = fixture.start() + stagedNavTransaction.unpublishFrom(fixture.scope) + fixture.scope.transaction = uiLoadTransaction + stagedNavTransaction.publishTo(fixture.scope) + fixture.scope.clearTransaction() + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.scope.transaction).isSameInstanceAs(started) + assertThat(fixture.startedTransactions).containsExactly(started) + } + + @Test + fun `retryPublishIfBlocked() - starts a nav transaction blocked by a ui load once it finishes`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + val uiLoad = transactionWithOrigin(ACTIVITY_UI_LOAD_ORIGIN) + stagedNavTransaction.stage(request) + fixture.scope.transaction = uiLoad + stagedNavTransaction.publishTo(fixture.scope) + whenever(uiLoad.isFinished).thenReturn(true) + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransaction("/Profile")) + } + + @Test + fun `retryPublishIfBlocked() - waits while the blocking ui load holds the slot`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + stagedNavTransaction.stage(request) + fixture.scope.transaction = uiLoadTransaction + stagedNavTransaction.publishTo(fixture.scope) + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.startedTransactions).isEmpty() + assertThat(fixture.scope.transaction).isSameInstanceAs(uiLoadTransaction) + + fixture.scope.clearTransaction() + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransaction("/Profile")) + } + + @Test + fun `retryPublishIfBlocked() - retries within the retry window`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + stagedNavTransaction.stage(request) + fixture.scope.transaction = uiLoadTransaction + stagedNavTransaction.publishTo(fixture.scope) + fixture.scope.clearTransaction() + fixture.nowNanos = UI_LOAD_RETRY_WINDOW_NANOS + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.scope.transaction).isSameInstanceAs(fixture.startedTransaction("/Profile")) + } + + @Test + fun `retryPublishIfBlocked() - retries nothing after the retry window`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + stagedNavTransaction.stage(request) + fixture.scope.transaction = uiLoadTransaction + stagedNavTransaction.publishTo(fixture.scope) + fixture.scope.clearTransaction() + fixture.nowNanos = UI_LOAD_RETRY_WINDOW_NANOS + 1 + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.startedTransactions).isEmpty() + assertThat(fixture.scope.transaction).isNull() + } + + @Test + fun `retryPublishIfBlocked() - retries nothing once the retry window passes while the ui load holds the slot`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + stagedNavTransaction.stage(request) + fixture.scope.transaction = uiLoadTransaction + stagedNavTransaction.publishTo(fixture.scope) + fixture.nowNanos = UI_LOAD_RETRY_WINDOW_NANOS + 1 + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + fixture.scope.clearTransaction() + fixture.nowNanos = 0 + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.startedTransactions).isEmpty() + } + + @Test + fun `retryPublishIfBlocked() - retries only once`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + stagedNavTransaction.stage(request) + fixture.scope.transaction = uiLoadTransaction + stagedNavTransaction.publishTo(fixture.scope) + fixture.scope.transaction = transactionWithOrigin(ACTIVITY_UI_LOAD_ORIGIN) + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + fixture.scope.clearTransaction() + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.startedTransactions).isEmpty() + assertThat(fixture.scope.transaction).isNull() + } + + @Test + fun `retryPublishIfBlocked() - retries nothing after a transaction other than a ui load blocked it`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + stagedNavTransaction.stage(request) + fixture.scope.transaction = hostTransaction + stagedNavTransaction.publishTo(fixture.scope) + fixture.scope.clearTransaction() + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.startedTransactions).isEmpty() + assertThat(fixture.scope.transaction).isNull() + } + + @Test + fun `retryPublishIfBlocked() - starts no new nav transaction when the publication wasn't blocked`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + val started = fixture.start() + stagedNavTransaction.publishTo(fixture.scope) + whenever(started.isFinished).thenReturn(true) + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.startedTransactions).containsExactly(started) + } + + @Test + fun `retryPublishIfBlocked() - retries nothing after a start the scopes declined`() { + val fixture = Fixture.create(declinesTransactions = true) + val stagedNavTransaction = fixture.stagedNavTransaction + stagedNavTransaction.stage(request) + stagedNavTransaction.publishTo(fixture.scope) + fixture.declinesTransactions = false + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.startedTransactions).isEmpty() + } + + @Test + fun `retryPublishIfBlocked() - retries nothing once the nav transaction is unpublished`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + stagedNavTransaction.stage(request) + fixture.scope.transaction = uiLoadTransaction + stagedNavTransaction.publishTo(fixture.scope) + stagedNavTransaction.unpublishFrom(fixture.scope) + fixture.scope.clearTransaction() + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.startedTransactions).isEmpty() + assertThat(fixture.scope.transaction).isNull() + } + + @Test + fun `retryPublishIfBlocked() - retries nothing once the nav transaction is finished`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + stagedNavTransaction.stage(request) + fixture.scope.transaction = uiLoadTransaction + stagedNavTransaction.publishTo(fixture.scope) + stagedNavTransaction.finish(fixture.scope) + fixture.scope.clearTransaction() + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.startedTransactions).isEmpty() + } + + @Test + fun `retryPublishIfBlocked() - starts nothing after a ui load blocked a publication that can't be retried`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + stagedNavTransaction.stage(request) + fixture.scope.transaction = uiLoadTransaction + stagedNavTransaction.publishTo(fixture.scope, canRetry = false) + fixture.scope.clearTransaction() + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.startedTransactions).isEmpty() + assertThat(fixture.scope.transaction).isNull() + } + + @Test + fun `retryPublishIfBlocked() - binds nothing after a ui load blocked a rebind that can't be retried`() { + val fixture = Fixture.create() + val stagedNavTransaction = fixture.stagedNavTransaction + fixture.start() + stagedNavTransaction.unpublishFrom(fixture.scope) + fixture.scope.transaction = uiLoadTransaction + stagedNavTransaction.publishTo(fixture.scope, canRetry = false) + fixture.scope.clearTransaction() + + stagedNavTransaction.retryPublishIfBlocked(fixture.scope) + + assertThat(fixture.scope.transaction).isNull() + } + @Test fun `unpublishFrom() - unbinds the started nav transaction without finishing it`() { val fixture = Fixture.create() @@ -613,6 +848,9 @@ class StagedNavTransactionTest { val scope = Scope(SentryOptions()) val logger = mock() + /** The time [scopes]' date provider reports, in nanoseconds. */ + var nowNanos = 0L + /** Whether [scopes] declines to start transactions, returning a no-op transaction instead. */ var declinesTransactions = false val startedTransactions = mutableListOf() @@ -627,6 +865,7 @@ class StagedNavTransactionTest { SentryOptions().apply { setLogger(this@Fixture.logger) isDebug = true + dateProvider = SentryDateProvider { SentryNanotimeDate(Date(0), nowNanos) } } ) whenever(scopes.startTransaction(any(), any()))