-
-
Notifications
You must be signed in to change notification settings - Fork 468
feat(replay): Add ReplaySnapshotObserver 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 all commits
d49b64a
4b5fb3a
9d6f12a
240dd96
1f6b03c
d2b2259
2b576be
f2c0c49
f39d8f5
1720230
4a829fe
23af62e
a7a01d9
8a44963
6481cdf
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,71 @@ | ||
| 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.SentryReplayOptions | ||
| import io.sentry.TypeCheckHint | ||
| 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") | ||
| 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 | ||
| it.sessionReplay.frameObserver = | ||
| SentryReplayOptions.ReplayFrameObserver { hint, frameTimestamp, screenName -> | ||
| val bitmap = | ||
| hint.getAs(TypeCheckHint.REPLAY_FRAME_BITMAP, Bitmap::class.java) | ||
| ?: return@ReplayFrameObserver | ||
| 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() | ||
| 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 |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ import android.view.MotionEvent | |
| import io.sentry.Breadcrumb | ||
| import io.sentry.DataCategory.All | ||
| import io.sentry.DataCategory.Replay | ||
| import io.sentry.Hint | ||
| import io.sentry.IConnectionStatusProvider.ConnectionStatus | ||
| import io.sentry.IConnectionStatusProvider.ConnectionStatus.DISCONNECTED | ||
| import io.sentry.IConnectionStatusProvider.IConnectionStatusObserver | ||
|
|
@@ -17,8 +18,10 @@ 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.TypeCheckHint | ||
| import io.sentry.android.replay.ReplayState.CLOSED | ||
| import io.sentry.android.replay.ReplayState.PAUSED | ||
| import io.sentry.android.replay.ReplayState.RESUMED | ||
|
|
@@ -308,6 +311,20 @@ public class ReplayIntegration( | |
| var screen: String? = null | ||
| scopes?.configureScope { screen = it.screen?.substringAfterLast('.') } | ||
| captureStrategy?.onScreenshotRecorded(bitmap) { frameTimeStamp -> | ||
| val observer = options.sessionReplay.frameObserver | ||
| if (observer != null) { | ||
| val copy = bitmap.copy(bitmap.config!!, false) | ||
|
cursor[bot] marked this conversation as resolved.
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. we copy the bitmap now
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. We should have our
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 think the only types of errors that can happen when copying the bitmap are memory errors and we shouldn't try to catch those.
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. javadoc says:
that said, I think in our case it will never be null since we always use
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) { | ||
|
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. no point in calling the API if we have a null bitmap |
||
| try { | ||
| val hint = Hint() | ||
| hint.set(TypeCheckHint.REPLAY_FRAME_BITMAP, copy) | ||
| observer.onMaskedFrameCaptured(hint, frameTimeStamp, screen) | ||
| } catch (e: Throwable) { | ||
| options.logger.log(ERROR, "Error in ReplayFrameObserver", e) | ||
| copy.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. I would not have thought of calling copy.recycle() if we get an exception here. AI is smart. |
||
| } | ||
| } | ||
| } | ||
| 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 👍 |
||
| checkCanRecord() | ||
|
|
||
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.
I copied this from
ReplayTestbut why are we even running these on emulators in gh actions?Uh oh!
There was an error while loading. Please reload this page.
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.
i think the purpose of this tests running in gh actions is split in two parts:
sentry-android-corewithout integrations to catch issues like CIRCULAR REFERENCE: com.android.tools.r8.utils.b: Missing class io.sentry.compose.viewhierarchy.ComposeViewHierarchyExporter #2738 earlyThat 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)