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.

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

Initial version of snapshot testing #173

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

spirali
Copy link
Contributor

@spirali spirali commented Nov 15, 2024

Hi,

This PR contains an initial version of snapshot testing.

By design it contains only a minimal API and usage inspired by vello snapshot tests.
The ambition of this PR is not to do any test coverage, it contains only two tests for demonstration purposes of the testing environment.

All the bells and whistles (html report, image diffs, not loading fonts in every test) are now missing to make the code more straightforward.

Usage

$ cargo test

If a test fails, you can compare images in parley/tests/current (images created by the current test)
and parley/tests/snapshots (the accepted versions).

If you think that everything is ok, you can start tests as follows:

$ PARLEY_TEST="accept" cargo test

It will update snapshots of the failed tests.

How it works

It reuses tiny-skia-renderer and renders the testing layout into an image and compares it pixel by pixel.

This PR also adds fonts into the tests, to have tests independent on the system fonts. I have chosen DejaVu fonts, but it was a random pick.

Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

Thank you - it will be great to get some testing landed.

However, We definitely shouldn't import several megabytes of fonts - it's pretty much a worst-case scenario for git; I'd suggest using only two fonts files; presumably we do want to test multi-font cases.
We also need to follow the license for whatever fonts we do import.

@spirali
Copy link
Contributor Author

spirali commented Nov 15, 2024

We definitely shouldn't import several megabytes of fonts.

We also need to follow the license for whatever fonts we do import.

The fonts are used only in tests, Parley itself can be build without it.

In the last change I have added license of the fonts.

@spirali
Copy link
Contributor Author

spirali commented Nov 15, 2024

Thank you - it will be great to get some testing landed.

However, We definitely shouldn't import several megabytes of fonts - it's pretty much a worst-case scenario for git; I'd suggest using only two fonts files; presumably we do want to test multi-font cases. We also need to follow the license for whatever fonts we do import.

Ok, I will try to prune it. However it would not be that bad for git, these files should only rarely changed, so it just a few constant megabytes.

I have also attached more variants so we can test e.g. different weights, because variable weighted font does not for me in Parley (I do not know if it a feature or a bug).

@spirali
Copy link
Contributor Author

spirali commented Nov 15, 2024

I have removed (and squashed) other variants and left just DejaVu Sans and DejaVu Sans Bold (= 2x 700KiB)

@xorgy
Copy link
Member

xorgy commented Nov 17, 2024

It might make more sense to use a pinned version of a distro font package in the eventual CI (maybe Noto?), and just have it spit out a useful message when you don't have those fonts installed.

@spirali
Copy link
Contributor Author

spirali commented Nov 18, 2024

I have completely removed fonts from repository & added a script that downloads Noto Fonts.

@spirali
Copy link
Contributor Author

spirali commented Nov 18, 2024

Note: CI is now failing, because font download is not part of CI scripts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants