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

Report for snapshot testing (diff included) #185

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

Conversation

spirali
Copy link
Contributor

@spirali spirali commented Nov 27, 2024

This PR adds reporting to tests.

Originally I wanted to integrate reporting directly into the testing framework,
but at the end, I have create a separate tool https://github.com/spirali/image_diff_review
and put a simple script into the Parley repository.

The main reasons are:

  • AFAIK there is no way how to call a code at the end of all tests in Rust (at least not on stable).
  • Snapshot reporting can be useful for other repositories, so there will be no code duplication across more projects.

Installation:

cargo install image_diff_review

Usage:

If one or more tests fails, you may run /parley/tests/make_report.sh that creates a report.html file
that looks like this:

image

@DJMcNab
Copy link
Member

DJMcNab commented Nov 28, 2024

Some thoughts:

  1. This would be an excellent use case for the xtask pattern (https://github.com/matklad/cargo-xtask).
    • This would have a few nice features, including automatic nextest usage and report generation in one command, plus automatic nice versioning of image_diff_review.
  2. Would you be interested in moving image_diff_review into Linebender directly? (We'd probably prefer it to be MIT/Apache 2.0 licensed in that case; probably using the semi-standard no-copyright notice version of the MIT license and an AUTHORS file)
  3. It would be extremely nice, if quite meta, to make image_diff_review itself a Xilem application. This is not in scope for this PR. That would mean we could have the blessing capability directly in the report, plus some light dogfooding advantages. That would make the xtask work less advantageous, as we wouldn't want automatic installation.

@spirali
Copy link
Contributor Author

spirali commented Nov 28, 2024

  1. I will look at xtasks, I was not aware of this project, but it seems useful here.
  2. I have no problem to move image_diff_review into Linebender directly including relicensing.
  3. The current version produce a static HTML file. My next goal was to add some interactivity by starting a simple HTTP server & simple JS in the report. But having a Xilem frontend could be also nice.

@spirali spirali force-pushed the test-report branch 4 times, most recently from 521d855 to 88929a8 Compare November 29, 2024 15:41
@spirali
Copy link
Contributor Author

spirali commented Nov 29, 2024

I have added cargo xtask_image_diff_report that creates the report.

@DJMcNab
Copy link
Member

DJMcNab commented Nov 29, 2024

Thank you! The new in-repository version looks really good.

Would you mind opening a thread about image_diff_review in #linebender? I think that's probably the next steps here. The questions are:

  1. Do we want to have this kind of tooling?
  2. Are we happy to move the crate into Linebender?
  3. Is this the form we want this to take?

My preference would be to nerd-snipe someone into writing a Xilem application, as that would have the greatest upside for us - it would allow us to validate the development of Xilem. But I do think the html version is still useful to have; I'll review again next week when we see what the outcome of that thread is.

From a quick scan, it looks good. One thing I did notice is that this seems to add a dependency on arbitrary, which is quite surprising; I wonder if we can trim some features of the transitive dependencies?

@spirali
Copy link
Contributor Author

spirali commented Nov 29, 2024

Thanks. I will start the thread.

Many dependencies comes from the "image" crate with default features, by default it is able to load various formats.
In the next release, I will enable just png, that strips down many dependencies, including arbitrary.

@spirali
Copy link
Contributor Author

spirali commented Nov 30, 2024

Dependency tree was stripped down.
I have temporarily moved dependency to github repo to avoid making release to reflect comments. I will move it back to crate.io when the PR will be reviewed.

@spirali
Copy link
Contributor Author

spirali commented Dec 4, 2024

I did some refactoring of xtask CLI and added detection of dead snapshots.

@DJMcNab DJMcNab enabled auto-merge December 5, 2024 09:06
@DJMcNab DJMcNab disabled auto-merge December 5, 2024 09:06
@spirali
Copy link
Contributor Author

spirali commented Dec 17, 2024

The xtask is now migrated to Kompari.

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.

2 participants