-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Add guidelines for image comparison tests #5714
Conversation
Preview available at https://egui-pr-preview.github.io/pr/5714-image-comparision-guide |
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, but I'm a bit worried that the "Why does CI / another computer produce a different image?" section might overwhelm the average user who just wants to test whether their button still looks the same.
If they run into issues with snapshots being different in CI, the reasonable thing for them to do would be increasing the threshold until it works. I think we should mention that somewhere, before explaining all the rendering things.
I disagree, it's important to understand why an image comparison tests may fail or even be flaky if you ever run into that situation. I do recognize that the detail I went into here stretches the intended audience by quite a bit which may only be concerned with "simple" ui testing. But as I see it it offers a way for more systematic reasoning than just mindlessly adjusting thresholds until it works (which, while being the inevitable reality 😞, is obviously a very precarious practice). Not feeling super strongly about this though, I can always just leave that in in the Rerun contribution docs only 🤷 (Emil suggested to put this here and I thought that made sense) |
maybe I can adjust the text a bit to make this more of "stop, think, look at the changes" before going into the weeds. |
I changed the framing and added more words talking about immediate actions instead. What do you think @lucasmerlin , does this hit the right tone or would you still rather leave that out entirely? |
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.
Awesome, I think it's perfect now!
Guidelines & why images may differ
Based on (but slightly altered):