-
Notifications
You must be signed in to change notification settings - Fork 109
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
ci: Prevent auto-update of visual regression screenshots #902
Conversation
My two cents: in case we rely on automation - which should be the goal as much as possible! - we need to make sure it works reliably. If that's not the case, I agree doing the work manually saves time in the end. What I also see it that our E2E setup works nicely on Mac, but it seems like there are some mitigations on Windows machines. Maybe @Skaiir can share his opinion here. |
IMO the task is manual by nature, when it fails you'll have to download the assets from the CI and check it manually if the failure makes sense and then wait until it gets updated on master/develop and check if the update didn't mess up anything on the CI If there's issue on Windows let's fix that, and not just ignore the tests |
So the issue with windows is this: It's creating a bunch of new snapshots, might be worth figuring out. Anyways, I don't necessarily agree with this PR. Firstly I'm certainly not ignoring the tests. If it's red I check why, if it's fine I push it through, simple workflow. There are some issues I can see with flaky Playwright runs, we can also look into fixing those. But what was the cause of the issue you're pointing to as an example exactly, I saw it was resolved by adding extra CSS, so there was something wrong on master right? The way I understand it it's not automation error, but just something wrong getting to master, which can always happen. |
@Skaiir did you follow the instructions I linked? If you follow them and use Docker this won't happen |
19a2c93
to
803a19b
Compare
3c93eea
to
7b76110
Compare
@Skaiir This PR should be ready for review again, sorry for taking so long I added a workflow that updates the screenshots automatically when you add the label |
The last commits is just to show how the actions works, I'll remove them when approved |
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.
This is great. My team at Amazon had so many issues with visual regressions and never had anything like this.
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.
Elegant solution that makes us all happy, nice one :)
3ef7af7
to
78b1841
Compare
78b1841
to
834fa91
Compare
834fa91
to
664b0bf
Compare
I don't believe the auto-update of screenshots is working out, it just makes us ignore the visual regression CI run on PRs and if we there's problems when we merge master to develop or vice-versa it updates the baseline screenshots wrongly like in here (and this is not the first time I noticed an update that shouldn't happen)
We have the instructions on how to update the screenshots here, it takes less than 5 min to update so I believe it's not a blocker