Skip to content
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.

Sign up for GitHub

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

Run accessibility checks after each screenshot #575

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
+345 −130
Open

Run accessibility checks after each screenshot #575

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
26e1625
Run accessibility checks after each screenshot
yschimke Nov 23, 2024
e45dded
Cleanup
yschimke Nov 23, 2024
e5d8ab7
Fix test
yschimke Nov 23, 2024
b970058
Merge branch 'refs/heads/main' into each_screenshot
yschimke Nov 23, 2024
3578557
Fix test
yschimke Nov 23, 2024
64c1ac9
Merge branch 'refs/heads/main' into each_screenshot
yschimke Nov 24, 2024
6bba1d9
fix merge
yschimke Nov 24, 2024
ab53441
simplify API
yschimke Nov 24, 2024
fbd34bf
simplify API
yschimke Nov 24, 2024
7913478
cleanup
yschimke Nov 24, 2024
254600b
Update roborazzi-accessibility-check/src/main/java/com/github/takahir…
yschimke Nov 24, 2024
86b4ca1
Rename CaptureRoot to RuleCaptureRoot to clarify the naming
takahirom Nov 25, 2024
a48585b
Add RoboComponent parameter to onCanvas
takahirom Nov 25, 2024
3a2c583
Merge branch 'main' into each_screenshot
yschimke Dec 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 25 additions & 5 deletions ...ty-check/src/main/java/com/github/takahirom/roborazzi/RoborazziATFAccessibilityChecker.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import androidx.compose.ui.test.SemanticsNodeInteraction
import androidx.test.espresso.UiController
import androidx.test.espresso.ViewAction
import androidx.test.espresso.ViewInteraction
import com.github.takahirom.roborazzi.RoborazziRule.AccessibilityCheckStrategy
import com.github.takahirom.roborazzi.RoborazziRule.AccessibilityCheckStrategy.CheckPoint
import com.github.takahirom.roborazzi.RoborazziRule.CaptureRoot
import com.google.android.apps.common.testing.accessibility.framework.AccessibilityCheckPreset
import com.google.android.apps.common.testing.accessibility.framework.AccessibilityCheckResult.AccessibilityCheckResultType
Expand Down Expand Up @@ -247,14 +249,14 @@ data class RoborazziATFAccessibilityChecker(
companion object
}

@ExperimentalRoborazziApi
data class AccessibilityCheckAfterTestStrategy(
val accessibilityOptionsFactory: () -> RoborazziATFAccessibilityCheckOptions = { provideATFAccessibilityOptionsOrCreateDefault() },
) : RoborazziRule.AccessibilityCheckStrategy {
abstract class BaseAccessibilityCheckStrategy: AccessibilityCheckStrategy {
yschimke marked this conversation as resolved.
Show resolved Hide resolved
abstract val accessibilityOptionsFactory: () -> RoborazziATFAccessibilityCheckOptions

private val accessibilityOptions by lazy { accessibilityOptionsFactory() }

override fun runAccessibilityChecks(
captureRoot: CaptureRoot, roborazziOptions: RoborazziOptions
) {
val accessibilityOptions = accessibilityOptionsFactory()
accessibilityOptions
.checker
.runAccessibilityChecks(
Expand All @@ -273,3 +275,21 @@ data class AccessibilityCheckAfterTestStrategy(
)
}
}

@ExperimentalRoborazziApi
data class AccessibilityCheckEachScreenshotStrategy(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I have a question about the current implementation.
I haven't read the code thoroughly. The AccessibilityCheckEachScreenshotStrategy is only for RoborazziRule.Options.captureRule and not for captureRoboImage(), right?
I believe that if we provide the RoborazziContext, we can check every captureRoboImage. Do you have any thoughts on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to take your advice, I'm not as familiar with all the different APIs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really the intention is to run when roborazzi produces distinct screenshots.

From

  • once per test
  • per screenshot
  • none (roborazzi or a11y checks not enabled)

What gets us closer to that?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can add ruleAccessibilityCheckStrategy and use it in captureRoboImage() like provideRoborazziContext().accessibilityCheckStrategy.afterScreenshot() to check the accessibility of each screenshot now that you added onEach{}.

77fa9dc#diff-84240ff001c21726c87098748f05f02f55cb70b8425874729662bc07ad8786ecR66

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I move that type from rule to core module?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like a big change I won't get to until next weekend.

Options

  • I'll take a look and make changes then
  • feel free to make the changes yourself before then
  • I can remove that screenshot mode, and just land the other changes for now

No rush, just letting you know where I am

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started to make the change and noticed some are already there. I'm unclear if you made all the changes already?

But for RoboComponent, what to do about Screen? Since it has multiple roots?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure, but can we conduct checks for each root? I don't have a good idea for that, but it might work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or just the top window?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. We could output logs like:
"Roborazzi: It seems we have multiple windows, so we'll check accessibility for the top window only."

override val accessibilityOptionsFactory: () -> RoborazziATFAccessibilityCheckOptions = { provideATFAccessibilityOptionsOrCreateDefault() },
) : BaseAccessibilityCheckStrategy() {
override fun shouldRunAt(checkPoint: CheckPoint): Boolean {
yschimke marked this conversation as resolved.
Show resolved Hide resolved
return checkPoint == CheckPoint.AfterScreenshot || checkPoint == CheckPoint.AfterTest
}
}

@ExperimentalRoborazziApi
data class AccessibilityCheckAfterTestStrategy(
override val accessibilityOptionsFactory: () -> RoborazziATFAccessibilityCheckOptions = { provideATFAccessibilityOptionsOrCreateDefault() },
) : BaseAccessibilityCheckStrategy() {
override fun shouldRunAt(checkPoint: CheckPoint): Boolean {
return checkPoint == CheckPoint.AfterTest
}
}
50 changes: 39 additions & 11 deletions roborazzi-junit-rule/src/main/java/com/github/takahirom/roborazzi/RoborazziRule.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package com.github.takahirom.roborazzi
import androidx.compose.ui.test.SemanticsNodeInteraction
import androidx.compose.ui.test.junit4.ComposeTestRule
import androidx.test.espresso.ViewInteraction
import com.github.takahirom.roborazzi.RoborazziRule.AccessibilityCheckStrategy.CheckPoint
import org.junit.rules.TestWatcher
import org.junit.runner.Description
import org.junit.runners.model.Statement
Expand Down Expand Up @@ -67,6 +68,13 @@ class RoborazziRule private constructor(
captureRoot: CaptureRoot,
roborazziOptions: RoborazziOptions,
)

enum class CheckPoint {
AfterTest, AfterScreenshot
}

fun shouldRunAt(checkPoint: CheckPoint): Boolean

// Use `roborazzi-accessibility-check`'s [com.github.takahirom.roborazzi.AccessibilityCheckAfterTestStrategy]
data object None : AccessibilityCheckStrategy {
override fun runAccessibilityChecks(
Expand All @@ -75,6 +83,8 @@ class RoborazziRule private constructor(
) {
// Do nothing
}

override fun shouldRunAt(checkPoint: CheckPoint): Boolean = false
}
}

Expand Down Expand Up @@ -182,16 +192,7 @@ class RoborazziRule private constructor(
) {
val evaluate: () -> Unit = {
try {
val accessibilityChecks = options.accessibilityCheckStrategy
// TODO enable a11y before showing content

base.evaluate()

accessibilityChecks.runAccessibilityChecks(
captureRoot = captureRoot,
roborazziOptions = options.roborazziOptions
)

} catch (e: Exception) {
throw e
}
Expand Down Expand Up @@ -219,16 +220,34 @@ class RoborazziRule private constructor(
}

is CaptureType.AllImage, is CaptureType.Gif -> {
val accessibilityChecks = options.accessibilityCheckStrategy

val result = when (captureRoot) {
is CaptureRoot.Compose -> captureRoot.semanticsNodeInteraction.captureComposeNode(
composeRule = captureRoot.composeRule,
roborazziOptions = roborazziOptions,
block = evaluate
block = evaluate,
onEach = {
if (accessibilityChecks.shouldRunAt(CheckPoint.AfterScreenshot)) {
accessibilityChecks.runAccessibilityChecks(
captureRoot = captureRoot,
roborazziOptions = options.roborazziOptions
)
}
},
)

is CaptureRoot.View -> captureRoot.viewInteraction.captureAndroidView(
roborazziOptions = roborazziOptions,
block = evaluate
block = evaluate,
onEach = {
if (accessibilityChecks.shouldRunAt(CheckPoint.AfterScreenshot)) {
accessibilityChecks.runAccessibilityChecks(
captureRoot = captureRoot,
roborazziOptions = options.roborazziOptions
)
}
},
)

CaptureRoot.None -> {
Expand Down Expand Up @@ -260,7 +279,16 @@ class RoborazziRule private constructor(

is CaptureType.LastImage -> {
val result = runCatching {
val accessibilityChecks = options.accessibilityCheckStrategy

evaluate()

if (accessibilityChecks.shouldRunAt(CheckPoint.AfterTest)) {
accessibilityChecks.runAccessibilityChecks(
captureRoot = captureRoot,
roborazziOptions = options.roborazziOptions
)
}
}
if (!captureType.onlyFail || result.isFailure) {
val outputFile =
Expand Down
33 changes: 24 additions & 9 deletions roborazzi/src/main/java/com/github/takahirom/roborazzi/Roborazzi.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ fun ViewInteraction.captureRoboGif(
) {
// currently, gif compare is not supported
if (!roborazziOptions.taskType.isRecording()) return
captureAndroidView(roborazziOptions, block).apply {
captureAndroidView(roborazziOptions = roborazziOptions, onEach = {}, block = block).apply {
saveGif(file)
clear()
result.getOrThrow()
Expand All @@ -244,7 +244,7 @@ fun ViewInteraction.captureRoboLastImage(
block: () -> Unit
) {
if (!roborazziOptions.taskType.isEnabled()) return
captureAndroidView(roborazziOptions, block).apply {
captureAndroidView(roborazziOptions = roborazziOptions, onEach = {}, block = block).apply {
saveLastImage(file)
clear()
result.getOrThrow()
Expand All @@ -257,7 +257,7 @@ fun ViewInteraction.captureRoboAllImage(
block: () -> Unit
) {
if (!roborazziOptions.taskType.isEnabled()) return
captureAndroidView(roborazziOptions, block).apply {
captureAndroidView(roborazziOptions = roborazziOptions, onEach = {}, block = block).apply {
saveAllImage(fileNameCreator)
clear()
result.getOrThrow()
Expand Down Expand Up @@ -321,7 +321,12 @@ fun SemanticsNodeInteraction.captureRoboGif(
) {
// currently, gif compare is not supported
if (!roborazziOptions.taskType.isRecording()) return
captureComposeNode(composeRule, roborazziOptions, block).apply {
captureComposeNode(
composeRule = composeRule,
roborazziOptions = roborazziOptions,
onEach = {},
block = block
).apply {
saveGif(file)
clear()
result.getOrThrow()
Expand All @@ -340,6 +345,7 @@ class CaptureInternalResult(
@InternalRoborazziApi
fun ViewInteraction.captureAndroidView(
roborazziOptions: RoborazziOptions,
onEach: () -> Unit = {},
block: () -> Unit
): CaptureInternalResult {
var removeListener = {}
Expand All @@ -351,7 +357,9 @@ fun ViewInteraction.captureAndroidView(
handler.postAtFrontOfQueue {
[email protected](
ImageCaptureViewAction(roborazziOptions) { canvas ->
canvases.addIfChanged(canvas, roborazziOptions)
if (canvases.addIfChanged(canvas, roborazziOptions)) {
onEach()
}
}
)
}
Expand Down Expand Up @@ -404,7 +412,9 @@ fun ViewInteraction.captureAndroidView(
// If there is already a screen, we should take the screenshot first not to miss the frame.
perform(
ImageCaptureViewAction(roborazziOptions) { canvas ->
canvases.addIfChanged(canvas, roborazziOptions)
if (canvases.addIfChanged(canvas, roborazziOptions)) {
onEach()
}
}
)
perform(viewTreeListenerAction)
Expand Down Expand Up @@ -445,18 +455,20 @@ fun ViewInteraction.captureAndroidView(
private fun MutableList<AwtRoboCanvas>.addIfChanged(
next: AwtRoboCanvas,
roborazziOptions: RoborazziOptions
) {
): Boolean {
val prev = this.lastOrNull() ?: run {
this.add(next)
return
return true
}
val differ: ImageComparator.ComparisonResult =
prev.differ(next, 1.0, roborazziOptions.compareOptions.imageComparator)
if (!roborazziOptions.compareOptions.resultValidator(differ)) {
this.add(next)
return true
} else {
// If the image is not changed, we should release the image.
next.release()
return false
}
}

Expand All @@ -481,6 +493,7 @@ private fun saveLastImage(
fun SemanticsNodeInteraction.captureComposeNode(
composeRule: ComposeTestRule,
roborazziOptions: RoborazziOptions = provideRoborazziContext().options,
onEach: () -> Unit = {},
block: () -> Unit
): CaptureInternalResult {
val canvases = mutableListOf<AwtRoboCanvas>()
Expand All @@ -493,7 +506,9 @@ fun SemanticsNodeInteraction.captureComposeNode(
),
roborazziOptions = roborazziOptions
) {
canvases.addIfChanged(it, roborazziOptions)
if (canvases.addIfChanged(it, roborazziOptions)) {
onEach()
}
}
}
val handler = Handler(Looper.getMainLooper())
Expand Down
Loading
Loading