-
Notifications
You must be signed in to change notification settings - Fork 289
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
feat: add new assertVisual
command
#2078
base: main
Are you sure you want to change the base?
feat: add new assertVisual
command
#2078
Conversation
You've mentioned missing the "changes accepting flow". What have you got in mind for the workflow for a failure where the new "look" is as-intended? |
@Fishbowler - the only thing i could've think of as of right now is to keep |
We would use this asap in our project and for us modifying the "baseline" image as part of the PR would be our expected workflow, and would actually be a great way for a design/product stakeholder to review any of the baseline screenshot diffs as part of a PR. In other words, no additional build here at all is a perfectly good "accept changes" workflow for our team. |
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'm not staff, but this LGTM. Awesome, in fact!
I've left a couple of comments, but neither is a hill I'll die on.
Would need an accompanying PR in the docs repo, which we'd probably want to label with "experimental" or something at this point.
override val label: String? = null, | ||
) : Command { | ||
override fun description(): String { | ||
return label ?: "Assert visual difference with baseline $baseline (threshold: $thresholdPercentage%)" |
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.
It's a nitpick, I know, but thinking of terminal in a vscode window on a MacBook... Could this be shorter?
Perhaps not 🤔
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.
ideas:
visual difference
->visual diff
maybe?- display just filename of $baseline, not the whole path
import com.fasterxml.jackson.annotation.JsonCreator | ||
import java.lang.UnsupportedOperationException | ||
|
||
private const val DEFAULT_DIFF_THRESHOLD = 95 |
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.
Should this be exposed as an environment variable (assuming you don't want to go as far as to expose it to workspace config at this point)?
assertVisual
command draftassertVisual
command
@bartekpacia - would gladly accept review/guidance on this one. Also #2086 would be invaluable for this. |
Hey @TheKohan – this is some exciting work. I no longer work at @mobile-dev-inc, and am a bit sad I didn't have time to finish this:( cc @amanjeetsingh150 – he's the right person to review this PR:) |
@amanjeetsingh150 - can you review this PR ? |
} | ||
} | ||
|
||
|
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.
Can you add an integration test for this command for the different cases you expect?
val photoNow: BufferedImage = ImageIO.read(expected) | ||
val oldPhoto: BufferedImage = ImageIO.read(actual) | ||
val failedRegressionDir = File(".maestro/failed_visual_regression").apply { mkdirs() } | ||
val regressionFailedFile = File(failedRegressionDir, baseline) |
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.
Will this generated file highlight the diff between old and new?
} | ||
|
||
val photoNow: BufferedImage = ImageIO.read(expected) | ||
val oldPhoto: BufferedImage = ImageIO.read(actual) |
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 feel we should ignore the system status bar as scope of this PR since it would start to fail with difference in time or having notifications 🤔 ? WDYT?
Proposed changes
This PR is built on top of #1898 by @bartekpacia and implements a basic
assertVisual
flow which is much awaited in the community (#1222).Given a simple React Native app
app.tsx
and a flow
test-flow.yml
maestro
will compare screenshots, producing result image with differences masking.Result
for now is being saved in.maestro/failed_visual_regression/
directory.If there's no
baseline
,current
will be saved as one.What is missing:
test-id
)Testing
in progress
- test plan has not yet been laid out due to potentially big changes in the proposed flow.Issues fixed