-
-
Notifications
You must be signed in to change notification settings - Fork 469
feat(replay): Add ReplayFrameObserver for snapshot testing #5386
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 11 commits
d49b64a
4b5fb3a
9d6f12a
240dd96
1f6b03c
d2b2259
2b576be
f2c0c49
f39d8f5
1720230
4a829fe
23af62e
a7a01d9
8a44963
6481cdf
7d51e78
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,4 +32,5 @@ artifacts: | |
| when: always | ||
| match: | ||
| - junit.xml | ||
| - "*.png" | ||
| directory: ./artifacts/ | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| package io.sentry.uitest.android | ||
|
|
||
| import android.graphics.Bitmap | ||
| import android.os.Environment | ||
| import androidx.lifecycle.Lifecycle | ||
| import androidx.test.core.app.launchActivity | ||
| import io.sentry.Sentry | ||
| import io.sentry.android.replay.ReplayIntegration | ||
| import io.sentry.android.replay.ReplaySnapshotObserver | ||
| import java.io.File | ||
| import java.util.concurrent.CopyOnWriteArraySet | ||
| import java.util.concurrent.CountDownLatch | ||
| import java.util.concurrent.TimeUnit | ||
| import kotlin.test.Test | ||
| import kotlin.test.assertTrue | ||
| import org.hamcrest.CoreMatchers.`is` | ||
| import org.junit.Assume.assumeThat | ||
| import org.junit.Before | ||
|
|
||
| class ReplaySnapshotTest : BaseUiTest() { | ||
|
|
||
| @Before | ||
| fun setup() { | ||
| // GH Actions emulators don't support capturing screenshots for replay | ||
| @Suppress("KotlinConstantConditions") | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I copied this from
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think the purpose of this tests running in gh actions is split in two parts:
That said, we probably don't even need to run the app, just compiling it would be sufficient to catch these two potential issues. Actually running the tests was a nice addition to also verify the SDK behaviour at runtime, considering the two things above (e.g. if R8 strips out some code over-aggressively it'd crash at runtime) |
||
| assumeThat(BuildConfig.ENVIRONMENT != "github", `is`(true)) | ||
| } | ||
|
|
||
| @Test | ||
| fun captureComposeReplayFrameSnapshots() { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just FYI, this is a pretty contrived test but it just makes sure we can capture a replay using the new snapshot observer api |
||
| val snapshotsDir = | ||
| File( | ||
| Environment.getExternalStoragePublicDirectory(Environment.DIRECTORY_DOWNLOADS), | ||
| "sauce_labs_custom_screenshots", | ||
| ) | ||
| .apply { | ||
| deleteRecursively() | ||
| mkdirs() | ||
| } | ||
| val frameReceived = CountDownLatch(1) | ||
| val capturedScreens = CopyOnWriteArraySet<String>() | ||
|
|
||
| val activityScenario = launchActivity<ComposeActivity>() | ||
| activityScenario.moveToState(Lifecycle.State.RESUMED) | ||
|
|
||
| initSentry { it.sessionReplay.sessionSampleRate = 1.0 } | ||
|
|
||
| val integration = Sentry.getCurrentScopes().options.replayController as? ReplayIntegration | ||
| integration?.snapshotObserver = ReplaySnapshotObserver { bitmap, frameTimestamp, screenName -> | ||
| val name = screenName ?: "unknown" | ||
| if (capturedScreens.add(name)) { | ||
| val file = File(snapshotsDir, "${name}_$frameTimestamp.png") | ||
| file.outputStream().use { out -> bitmap.compress(Bitmap.CompressFormat.PNG, 100, out) } | ||
| } | ||
| bitmap.recycle() | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. with the bitmap copy, we now have to make sure that we call |
||
| frameReceived.countDown() | ||
| } | ||
|
|
||
| assertTrue(frameReceived.await(10, TimeUnit.SECONDS), "Expected at least one replay frame") | ||
| assertTrue(capturedScreens.isNotEmpty(), "Expected at least one screen captured") | ||
|
|
||
| val files = snapshotsDir.listFiles()?.filter { it.extension == "png" } ?: emptyList() | ||
| assertTrue(files.isNotEmpty(), "Expected snapshot PNG files on disk") | ||
| assertTrue(files.all { it.length() > 0 }, "Snapshot files should not be empty") | ||
|
|
||
| activityScenario.moveToState(Lifecycle.State.DESTROYED) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -61,6 +61,8 @@ android { | |
|
|
||
| buildFeatures { buildConfig = true } | ||
|
|
||
| configurations.all { resolutionStrategy.force(libs.jetbrains.annotations.get()) } | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I need to dig deeper in to why we need to add this to all the modules that add |
||
|
|
||
| androidComponents.beforeVariants { | ||
| it.enable = !Config.Android.shouldSkipDebugVariant(it.buildType) | ||
| } | ||
|
|
@@ -71,6 +73,7 @@ kotlin { explicitApi() } | |
| dependencies { | ||
| api(projects.sentry) | ||
|
|
||
| compileOnly(libs.jetbrains.annotations) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure if I missed something, but this one turns out to be unused?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is used for the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but that one is in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh i see, this is leftover from a previous refactoring! thanks! |
||
| compileOnly(libs.androidx.compose.ui.replay) | ||
| implementation(kotlin(Config.kotlinStdLib, Config.kotlinStdLibVersionAndroid)) | ||
| // tests | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,7 @@ import io.sentry.ReplayBreadcrumbConverter | |
| import io.sentry.ReplayController | ||
| import io.sentry.SentryIntegrationPackageStorage | ||
| import io.sentry.SentryLevel.DEBUG | ||
| import io.sentry.SentryLevel.ERROR | ||
| import io.sentry.SentryLevel.INFO | ||
| import io.sentry.SentryOptions | ||
| import io.sentry.android.replay.ReplayState.CLOSED | ||
|
|
@@ -122,6 +123,8 @@ public class ReplayIntegration( | |
| private val lifecycleLock = AutoClosableReentrantLock() | ||
| private val lifecycle = ReplayLifecycle() | ||
|
|
||
| @Volatile public var snapshotObserver: ReplaySnapshotObserver? = null | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this awkward API is the consequence of moving the API to the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❗- Ah, I thought we were going the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, then I misunderstood, this is the same API as in your branch which is what I thought we agreed to: a97de37#diff-cb8ccc45cef66aca485d205c9e99c17c114b6319ab9b7d52173b52ee72b047a8R126 We just need to make it
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| override fun register(scopes: IScopes, options: SentryOptions) { | ||
| this.options = options | ||
|
|
||
|
|
@@ -308,6 +311,18 @@ public class ReplayIntegration( | |
| var screen: String? = null | ||
| scopes?.configureScope { screen = it.screen?.substringAfterLast('.') } | ||
| captureStrategy?.onScreenshotRecorded(bitmap) { frameTimeStamp -> | ||
| val observer = snapshotObserver | ||
| if (observer != null) { | ||
| val copy = bitmap.copy(bitmap.config!!, false) | ||
|
cursor[bot] marked this conversation as resolved.
runningcode marked this conversation as resolved.
runningcode marked this conversation as resolved.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Newbie question, but are we able to observe the performance hit of copying on our end? Esp if we want to expand our use-cases beyond testing or debugging at some point, it'd be good to know whether we need to optimize.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Memory-wise, each bitmap is roughly 900kb and we produce one frame per second.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 900kb sounds like a lot, last time check it was way less 😅 I guess it depends on complexity of app's UI as well as the device used. We'll probably have to look into this as part of SDK Perf overhead initiative, and we even have an issue for that: #4154. I'd imagine we could use webp without losing anything, but that's for later |
||
| if (copy != null) { | ||
|
runningcode marked this conversation as resolved.
|
||
| try { | ||
| observer.onSnapshotCaptured(copy, frameTimeStamp, screen) | ||
| } catch (e: Throwable) { | ||
| options.logger.log(ERROR, "Error in ReplaySnapshotObserver", e) | ||
| copy.recycle() | ||
|
runningcode marked this conversation as resolved.
|
||
| } | ||
| } | ||
| } | ||
| addFrame(bitmap, frameTimeStamp, screen) | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❗- It's not in the diff, so apologies for missing it the first time round, but I see there's an
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yep, that one is used by Flutter exclusively, but would be good to align here and also call the observer 👍
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok added below. Note, we read the file and decode it as a bitmap. |
||
| checkCanRecord() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,8 @@ | ||
| package io.sentry.android.replay | ||
|
|
||
| import android.graphics.Bitmap | ||
| import io.sentry.SentryReplayOptions | ||
| import org.jetbrains.annotations.ApiStatus | ||
|
|
||
| // since we don't have getters for maskAllText and maskAllimages, they won't be accessible as | ||
| // properties in Kotlin, therefore we create these extensions where a getter is dummy, but a setter | ||
|
|
@@ -29,3 +31,17 @@ public var SentryReplayOptions.maskAllImages: Boolean | |
| @Deprecated("Getter is unsupported.", level = DeprecationLevel.ERROR) | ||
| get() = error("Getter not supported") | ||
| set(value) = setMaskAllImages(value) | ||
|
|
||
| /** | ||
| * Observer that is notified when a replay snapshot is captured. The snapshot bitmap has masking | ||
| * already applied. | ||
| * | ||
| * **Bitmap lifecycle:** The bitmap is a copy owned by the caller. You may store it or use it on | ||
| * another thread. Call [Bitmap.recycle] when you no longer need it to free native memory promptly. | ||
| * | ||
| * The callback runs on a background thread (the replay executor). | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Worth emphasizing that folks should keep processing quick or hand off to another thread. (Otherwise they'll slow down the SDK's replay pipeline.) |
||
| */ | ||
| @ApiStatus.Experimental | ||
|
runningcode marked this conversation as resolved.
Outdated
runningcode marked this conversation as resolved.
Outdated
|
||
| public fun interface ReplaySnapshotObserver { | ||
| public fun onSnapshotCaptured(bitmap: Bitmap, frameTimestamp: Long, screenName: String?) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth noting that this is for use in snapshot tests and for debugging purposes and isn't currently optimized for production use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree, and could we also add a short code snippet showcasing the usage? (in Kotlin would be enough)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean a snippet here in the CHANGELOG ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes! you can find some example down there in the already released versions