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

Custom comparison function for expect_snapshot_file() #1378

Closed
nbenn opened this issue May 13, 2021 · 5 comments
Closed

Custom comparison function for expect_snapshot_file() #1378

nbenn opened this issue May 13, 2021 · 5 comments

Comments

@nbenn
Copy link
Contributor

nbenn commented May 13, 2021

I have been struggling a bit with trying to get some tests on plot outputs to pass on gh action runs. With a bit of trial and error, I believe I managed to get both my local machine and the actions VM produce images that are identical on a pixel level, however non-identical wrt the current comparison function

compare_file_binary <- function(old, new) {
  old <- brio::read_file_raw(old)
  new <- brio::read_file_raw(new)
  identical(old, new)
}

A potentially worthwhile addition to the expect_snapshot_file() API could be an argument for specifying a custom comparison function. For my use-case for example

compare_img_data <- function(old, new) {
  old <- magick::image_read(old)
  new <- magick::image_read(new)
  identical(magick::image_data(new), magick::image_data(old))
}

would do the trick. This could make expect_snapshot_file() much more powerful than it currently is. For example, again using the magick package, a user could test for image similarity by thresholding the result of magick::image_compare_dist(), etc.

My suggestion would be to add a minor change to expect_snapshot_file(), something along the lines of nbenn@9f91064. I'm happy to expand this to a proper PR if this is of interest to others as well.

@lionel-
Copy link
Member

lionel- commented Jun 15, 2021

This suggestion makes sense but I would perhaps deprecate the binary argument and export the comparison functions instead. Otherwise binary is ignored when compare is supplied, which is not ideal.

@nbenn
Copy link
Contributor Author

nbenn commented Jun 15, 2021

Sure, even better! I made my suggestion to not break with the current API. But given that this is clearly marked experimental, this makes total sense.

Thanks for considering my suggestion.

Let me know if you want me to change and submit a PR.

@hadley
Copy link
Member

hadley commented Jun 15, 2021

Yeah, that sounds reasonable to me. It'd be worth a quick github search or expect_snapshot_file() just to double check that it isn't going to affect a bunch of existing code (which I doubt).

lionel- pushed a commit that referenced this issue Jun 16, 2021
@lionel-
Copy link
Member

lionel- commented Jun 16, 2021

These packages are using binary = : rmarkdown, vmr, dm, pkglite, reportfactory. I'll deprecate the argument so they can update whenever they are working on a new release.

@nbenn I have cherry-picked your commit in #1400 so I can go forward with the release. Thanks for this!

@nbenn
Copy link
Contributor Author

nbenn commented Jun 16, 2021

Thanks!

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

No branches or pull requests

3 participants