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
Open

Run accessibility checks after each screenshot #575

Show file tree
Hide file tree
Changes from all 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
54 changes: 40 additions & 14 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,7 +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.CaptureRoot
import com.github.takahirom.roborazzi.RoborazziRule.AccessibilityCheckStrategy
import com.github.takahirom.roborazzi.RoborazziRule.RuleCaptureRoot
import com.google.android.apps.common.testing.accessibility.framework.AccessibilityCheckPreset
import com.google.android.apps.common.testing.accessibility.framework.AccessibilityCheckResult.AccessibilityCheckResultType
import com.google.android.apps.common.testing.accessibility.framework.AccessibilityHierarchyCheck
Expand Down Expand Up @@ -78,7 +79,6 @@ data class RoborazziATFAccessibilityChecker(
data class Compose(val semanticsNodeInteraction: SemanticsNodeInteraction) : CheckNode
}


@ExperimentalRoborazziApi
enum class CheckLevel(private vararg val failedTypes: AccessibilityCheckResultType) {
Error(AccessibilityCheckResultType.ERROR),
Expand Down Expand Up @@ -226,6 +226,7 @@ data class RoborazziATFAccessibilityChecker(
AccessibilityCheckResultType.WARNING -> roborazziErrorLog(
"Warning: $check"
)

AccessibilityCheckResultType.SUPPRESSED -> roborazziReportLog(
"Suppressed: $check"
)
Expand All @@ -248,28 +249,53 @@ data class RoborazziATFAccessibilityChecker(
}

@ExperimentalRoborazziApi
data class AccessibilityCheckAfterTestStrategy(
val accessibilityOptionsFactory: () -> RoborazziATFAccessibilityCheckOptions = { provideATFAccessibilityOptionsOrCreateDefault() },
) : RoborazziRule.AccessibilityCheckStrategy {
override fun runAccessibilityChecks(
captureRoot: CaptureRoot, roborazziOptions: RoborazziOptions
abstract class BaseAccessibilityCheckStrategy : AccessibilityCheckStrategy {
abstract val accessibilityOptionsFactory: () -> RoborazziATFAccessibilityCheckOptions

private val accessibilityOptions by lazy { accessibilityOptionsFactory() }

@InternalRoborazziApi
fun runAccessibilityChecks(
yschimke marked this conversation as resolved.
Show resolved Hide resolved
ruleCaptureRoot: RuleCaptureRoot, roborazziOptions: RoborazziOptions
) {
val accessibilityOptions = accessibilityOptionsFactory()
accessibilityOptions
.checker
.runAccessibilityChecks(
checkNode = when (captureRoot) {
is CaptureRoot.Compose -> RoborazziATFAccessibilityChecker.CheckNode.Compose(
semanticsNodeInteraction = captureRoot.semanticsNodeInteraction
checkNode = when (ruleCaptureRoot) {
is RuleCaptureRoot.Compose -> RoborazziATFAccessibilityChecker.CheckNode.Compose(
semanticsNodeInteraction = ruleCaptureRoot.semanticsNodeInteraction
)

CaptureRoot.None -> return
is CaptureRoot.View -> RoborazziATFAccessibilityChecker.CheckNode.View(
viewInteraction = captureRoot.viewInteraction
RuleCaptureRoot.None -> return
is RuleCaptureRoot.View -> RoborazziATFAccessibilityChecker.CheckNode.View(
viewInteraction = ruleCaptureRoot.viewInteraction
)
},
roborazziOptions = roborazziOptions,
failureLevel = accessibilityOptions.failureLevel,
)
}
}

@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 afterScreenshot(ruleCaptureRoot: RuleCaptureRoot, roborazziOptions: RoborazziOptions) {
runAccessibilityChecks(ruleCaptureRoot, roborazziOptions)
}

override fun afterTest(ruleCaptureRoot: RuleCaptureRoot, roborazziOptions: RoborazziOptions) {
runAccessibilityChecks(ruleCaptureRoot, roborazziOptions)
}
}

@ExperimentalRoborazziApi
data class AccessibilityCheckAfterTestStrategy(
override val accessibilityOptionsFactory: () -> RoborazziATFAccessibilityCheckOptions = { provideATFAccessibilityOptionsOrCreateDefault() },
) : BaseAccessibilityCheckStrategy() {

override fun afterTest(ruleCaptureRoot: RuleCaptureRoot, roborazziOptions: RoborazziOptions) {
runAccessibilityChecks(ruleCaptureRoot, roborazziOptions)
}
}
85 changes: 42 additions & 43 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 @@ -25,7 +25,7 @@ private val defaultFileProvider: FileProvider =
* 2. Capture screenshots for each test when specifying RoborazziRule.options.captureType.
*/
class RoborazziRule private constructor(
private val captureRoot: CaptureRoot,
private val ruleCaptureRoot: RuleCaptureRoot,
private val options: Options = Options()
) : TestWatcher() {
init {
Expand Down Expand Up @@ -82,20 +82,11 @@ class RoborazziRule private constructor(

@ExperimentalRoborazziApi
interface AccessibilityCheckStrategy {
@InternalRoborazziApi
fun runAccessibilityChecks(
captureRoot: CaptureRoot,
roborazziOptions: RoborazziOptions,
)
fun afterScreenshot(ruleCaptureRoot: RuleCaptureRoot, roborazziOptions: RoborazziOptions) {}
fun afterTest(ruleCaptureRoot: RuleCaptureRoot, roborazziOptions: RoborazziOptions) {}

// Use `roborazzi-accessibility-check`'s [com.github.takahirom.roborazzi.AccessibilityCheckAfterTestStrategy]
data object None : AccessibilityCheckStrategy {
override fun runAccessibilityChecks(
captureRoot: CaptureRoot,
roborazziOptions: RoborazziOptions
) {
// Do nothing
}
}
data object None : AccessibilityCheckStrategy
}

sealed interface CaptureType {
Expand Down Expand Up @@ -136,21 +127,21 @@ class RoborazziRule private constructor(
}

@InternalRoborazziApi
sealed interface CaptureRoot {
object None : CaptureRoot
sealed interface RuleCaptureRoot {
object None : RuleCaptureRoot
class Compose(
val composeRule: ComposeTestRule,
val semanticsNodeInteraction: SemanticsNodeInteraction
) : CaptureRoot
) : RuleCaptureRoot

class View(val viewInteraction: ViewInteraction) : CaptureRoot
class View(val viewInteraction: ViewInteraction) : RuleCaptureRoot
}

constructor(
captureRoot: ViewInteraction,
options: Options = Options()
) : this(
captureRoot = CaptureRoot.View(captureRoot),
ruleCaptureRoot = RuleCaptureRoot.View(captureRoot),
options = options
)

Expand All @@ -159,14 +150,14 @@ class RoborazziRule private constructor(
captureRoot: SemanticsNodeInteraction,
options: Options = Options()
) : this(
captureRoot = CaptureRoot.Compose(composeRule, captureRoot),
ruleCaptureRoot = RuleCaptureRoot.Compose(composeRule, captureRoot),
options = options
)

constructor(
options: Options = Options()
) : this(
captureRoot = CaptureRoot.None,
ruleCaptureRoot = RuleCaptureRoot.None,
options = options
)

Expand All @@ -183,7 +174,7 @@ class RoborazziRule private constructor(
provideRoborazziContext().setRuleOverrideFileProvider(options.outputFileProvider)
provideRoborazziContext().setRuleOverrideDescription(description)
provideRoborazziContext().setRuleOverrideAccessibilityOptions(options.roborazziAccessibilityOptions)
runTest(base, description, captureRoot)
runTest(base, description, ruleCaptureRoot)
} finally {
provideRoborazziContext().clearRuleOverrideOutputDirectory()
provideRoborazziContext().clearRuleOverrideRoborazziOptions()
Expand All @@ -198,20 +189,11 @@ class RoborazziRule private constructor(
private fun runTest(
base: Statement,
description: Description,
captureRoot: CaptureRoot
ruleCaptureRoot: RuleCaptureRoot
) {
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 @@ -239,19 +221,31 @@ class RoborazziRule private constructor(
}

is CaptureType.AllImage, is CaptureType.Gif -> {
val result = when (captureRoot) {
is CaptureRoot.Compose -> captureRoot.semanticsNodeInteraction.captureComposeNode(
composeRule = captureRoot.composeRule,
val result = when (ruleCaptureRoot) {
is RuleCaptureRoot.Compose -> ruleCaptureRoot.semanticsNodeInteraction.captureComposeNode(
composeRule = ruleCaptureRoot.composeRule,
roborazziOptions = roborazziOptions,
block = evaluate
block = evaluate,
onEach = {
options.accessibilityCheckStrategy.afterScreenshot(
ruleCaptureRoot = ruleCaptureRoot,
roborazziOptions = options.roborazziOptions
)
},
)

is CaptureRoot.View -> captureRoot.viewInteraction.captureAndroidView(
is RuleCaptureRoot.View -> ruleCaptureRoot.viewInteraction.captureAndroidView(
roborazziOptions = roborazziOptions,
block = evaluate
block = evaluate,
onEach = {
options.accessibilityCheckStrategy.afterScreenshot(
ruleCaptureRoot = ruleCaptureRoot,
roborazziOptions = options.roborazziOptions
)
},
)

CaptureRoot.None -> {
RuleCaptureRoot.None -> {
error("captureRoot is required for AllImage and Gif")
}
}
Expand Down Expand Up @@ -281,22 +275,27 @@ class RoborazziRule private constructor(
is CaptureType.LastImage -> {
val result = runCatching {
evaluate()

options.accessibilityCheckStrategy.afterTest(
ruleCaptureRoot = ruleCaptureRoot,
roborazziOptions = options.roborazziOptions
)
}
if (!captureType.onlyFail || result.isFailure) {
val outputFile =
fileWithRecordFilePathStrategy(DefaultFileNameGenerator.generateFilePath())
when (captureRoot) {
is CaptureRoot.Compose -> captureRoot.semanticsNodeInteraction.captureRoboImage(
when (ruleCaptureRoot) {
is RuleCaptureRoot.Compose -> ruleCaptureRoot.semanticsNodeInteraction.captureRoboImage(
file = outputFile,
roborazziOptions = roborazziOptions
)

is CaptureRoot.View -> captureRoot.viewInteraction.captureRoboImage(
is RuleCaptureRoot.View -> ruleCaptureRoot.viewInteraction.captureRoboImage(
file = outputFile,
roborazziOptions = roborazziOptions
)

CaptureRoot.None -> {
RuleCaptureRoot.None -> {
error("captureRoot is required for LastImage")
}
}
Expand Down
Loading
Loading