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

Add snapshot testing with insta #157

Open
wants to merge 1 commit into
base: oxidize
Choose a base branch
from

Conversation

cormacrelf
Copy link
Collaborator

@cormacrelf cormacrelf commented Oct 23, 2019

I wanted some tests to check that images were being included exactly as before for #68. So I added snapshot testing with insta, which is much simpler to set up and see differences with than the existing tex-outputs testing which can only report binary failure.

Old procedure was apparently (I couldn't find docs for it) to compile a .tex file separately with tectonic with a bunch of flags to dump logs, do it again with --format xdv, then separate assets into the assets folder, and if you wanted to change it, you had to put all those files together again...

Procedure to add a test case now:

  1. Add a .tex file to tests/snapshots.
  2. Add a test to mod snapshots under tests/tex-outputs.rs, using TestCase like the others, but executing it with .snap() instead of .go().
  3. Run the tests. The whole suite is a bit slow, so narrow it down with cargo test --test tex-outputs snapshots.
  4. cargo insta review after you've installed cargo-insta.

Read https://docs.rs/insta/ for more details on use, especially using INSTA_FORCE_PASS=1 to avoid bailing at the first sign of snapshot failure & ensure all .snap.new files are written, which is especially relevant as there will often be more than one failure within a test case when checking multiple passes of the output.

Some notes and questions:

  1. Binary files (i.e. PDFs) are stored as hex dumps so you can get a diff without breaking the terminal or being an invalid rust String. insta doesn't have anything suitable for binary files out of the box, but this seems to work well enough.
  2. The test harness will dump a test_name.preview.pdf in the snapshots directory, which you can visually check. These are gitignored.
  3. The .snap code is not exactly DRY with .go() but I don't think it matters too much.
  4. I put the .tex files in the same directory as the .snap files -- is that going to be annoying?
  5. Is this good enough to consider converting the other test cases? Probably not in the Rust fork.
  6. We should only very rarely have to review/accept diffs in PDF or XDV output, right? At least if we change the logging then that can be accepted.
  7. We already have the arxiv suite, which compares, if I am not mistaken, against mainline tectonic. A notable issue with snapshot testing is that the snapshots are created from the code as it was when you added them. Does this need a script for creating test cases with mainline tectonic as well?

@pkgw
Copy link
Collaborator

pkgw commented Oct 23, 2019

Very interesting! Unfortunately I'm super busy at work these days (that's what I've been saying to every Tectonic PR for the past six months or so ...) so I don't expect to be able to give this the review it deserves right now, but I'm very intrigued by the concept.

Based on what you've written ... what do you think the prospects would be for teaching Insta how to deal with binary files? I feel like it would be pretty nice to be able to keep everything binary instead of hexifying.

@cormacrelf
Copy link
Collaborator Author

teaching insta how to deal with binary files?

@pkgw its snapshot format has a YAML header, so it would need an extra sidecar file. Maybe not too hard? I could file an issue with them.

@cormacrelf cormacrelf removed the request for review from pkgw October 23, 2019 15:20
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.

None yet

2 participants