Skip to content

Added Docs for the new visual tests #7640

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

Merged
merged 6 commits into from
Mar 29, 2025
Merged

Conversation

Vaivaswat2244
Copy link

Resolves #7617

Changes:

Added docs and explained the new algorithm.
Added visual examples for better understanding.

Screenshots of the change:

PR Checklist

Copy link
Contributor

@davepagurek davepagurek left a comment

Choose a reason for hiding this comment

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

This is great @Vaivaswat2244, thanks!

In addition to this explanation of how the new diff check works, would you also be interested in adding a section that explains some best practices? Off the top of my head, this probably means:

  • keeping the images as close as possible to 50x50, since we resize for efficiency before doing a diff
  • trying to keep the level of detail in the test to something that can be seen easily at that small size
  • calling screenshot() multiple times in a test if you need to test a bunch of variants rather than trying to make them all visible at the same time in one screenshot
  • ...and anything else you can think of!

* Minor differences in curve smoothness

For example, text rendered on macOS might appear slightly different from text rendered in a Linux CI environment. The same applies to thin lines, curves, and other graphical elements with anti-aliasing.
An example of this can be the below image which earlier caused tests to fail in CI because of different rendering environments.
Copy link
Contributor

Choose a reason for hiding this comment

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

We might want an additional sentence to reiterate that although this failed before, this is an example of something that should pass, and will pass using the method you describe below.

Copy link
Author

Choose a reason for hiding this comment

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

Sure

Copy link
Author

@Vaivaswat2244 Vaivaswat2244 Mar 20, 2025

Choose a reason for hiding this comment

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

@davepagurek, Have made the changes

Copy link
Contributor

@davepagurek davepagurek left a comment

Choose a reason for hiding this comment

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

Sorry for the delay in reviewing! This looks just about good to go, I just left some minor formatting comments and then we can merge!

p5.stroke(0);
p5.strokeWeight(1);
p5.line(10, 25, 40, 25);
screenshot('thin-line');
Copy link
Contributor

@davepagurek davepagurek Mar 27, 2025

Choose a reason for hiding this comment

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

I think screenshot doesn't do anything with arguments, maybe this and the other screenshot call could be done with a comment instead? e.g.

Suggested change
screenshot('thin-line');
screenshot(); // Screenshot with thin lines

Copy link
Author

Choose a reason for hiding this comment

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

Sure, got it.

It's important to note that the improved algorithm described above allows tests with acceptable platform-specific variations to pass correctly. Tests that previously failed due to minor rendering differences (like anti-aliasing variations or subtle text rendering differences) will now pass as they should, while still detecting actual rendering bugs.
For example, a test showing text rendering that previously failed on CI (despite looking correct visually) will now pass with the improved algorithm, as it can distinguish between meaningful differences and acceptable platform-specific rendering variations. This makes the test suite more reliable and reduces false failures that require manual investigation.

SOME BEST PRACTISES FOR WRITING VISUAL TESTS:
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can use a markdown subheading for this to fit stylistically with the rest of the document?

Suggested change
SOME BEST PRACTISES FOR WRITING VISUAL TESTS:
### Some best practices for writing visual tests:

Copy link
Author

Choose a reason for hiding this comment

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

Right. Will do

@davepagurek davepagurek merged commit 8279b97 into processing:dev-2.0 Mar 29, 2025
2 checks passed
@Vaivaswat2244 Vaivaswat2244 deleted the 7617 branch March 29, 2025 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants