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 replace argument to expect_snapshot() #1345

Closed
hadley opened this issue Mar 3, 2021 · 7 comments · Fixed by #1414
Closed

Add replace argument to expect_snapshot() #1345

hadley opened this issue Mar 3, 2021 · 7 comments · Fixed by #1414
Labels
feature a feature request or enhancement snapshot 📷

Comments

@hadley
Copy link
Member

hadley commented Mar 3, 2021

For when you have sensitive or random components:

  expect_snapshot(
    gargle_oauth_sitrep(tmp_cache)
    replace = c("{path}" = tmp_cache)
  )
@jennybc
Copy link
Member

jennybc commented Mar 19, 2021

I ended up needing to do quite a bit of this in gargle, because of temp paths that vary, token hashes that vary by OS or R version, random backoff waiting times, etc.

Observations to guide implementation:

  • Sometimes you know the random or sensitive thing in advance (e.g. a temp path you've made and stored and used throughout the test). Then you can get away with sub(random_thing, replacement, snapshot_content, fixed = TRUE), which is nice.
  • More often, you need a regular expression. In fact, I often needed look ahead and look behind, so perl = TRUE needs to be possible.
  • Sometimes you need multiple replacements for 1 snapshot and they could be a mix of fixed string and regex.
  • Sometimes you are eliminating a matched expression, i.e. replacing with "".

@jennybc
Copy link
Member

jennybc commented Mar 19, 2021

Here's a real example of the basic form I used that also happens to illustrate most of the above:

out <- capture.output(
    gargle_oauth_sitrep(tmp_cache),
    type = "message"
)
out <- sub(tmp_cache, "{path to gargle oauth cache}", out, fixed = TRUE)
out <- sub("[[:xdigit:]]{7}[.]{3}", "{hash...}", out)
expect_snapshot(
  writeLines(out)
)

One big bummer about the workaround is that the code in your snapshot ends up being completely uninformative (writeLines(out)), so you really have to rely on the test name (or use a more informative name than out). It will be much nicer to have this implemented properly in expect_snapshot().

@hadley
Copy link
Member Author

hadley commented Mar 19, 2021

I think we'd default to perl = TRUE, and if you needed fixed for some reason, you'd have to manually escape the regexp (which I don't think should happen very often).

Could we just disallow replacing with ""? You could always just do xxxxx instead?

@jennybc
Copy link
Member

jennybc commented Mar 19, 2021

Could we just disallow replacing with ""? You could always just do xxxxx instead?

Well I have a piece of text that may or may not appear, so no. (Obviously one could also examine the problem from the other direction and rethink the message.)

@hadley
Copy link
Member Author

hadley commented Mar 19, 2021

Ooooh I didn't think of that case.

@hadley hadley added feature a feature request or enhancement snapshot 📷 labels Jul 7, 2021
@hadley
Copy link
Member Author

hadley commented Jul 7, 2021

May be better to leave completely generic:

expect_snapshot(
  gargle_oauth_sitrep(tmp_cache),
  redact = function(x) {
    x <- sub(tmp_cache, "{path to gargle oauth cache}", x, fixed = TRUE)
    x <- sub("[[:xdigit:]]{7}[.]{3}", "{hash...}", x)
    x
  }
)

@jennybc
Copy link
Member

jennybc commented Jul 7, 2021

Here's where things have evolved to in googledrive:

https://github.com/tidyverse/googledrive/blob/6c8f5dd8469689a458ae9d67ad8a359cd293fe25/tests/testthat/helper.R#L35

So I have these two scrubbers and use them in many tests. I can imagine this being a common pattern. So, yes, it would be nice to have a way to apply custom scrubbers inside expect_snapshot().

hadley added a commit that referenced this issue Jul 8, 2021
hadley added a commit that referenced this issue Jul 12, 2021
To make it easier to scrub sensitive and stochastic output

Fixes #1345
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature a feature request or enhancement snapshot 📷
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants