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

expect_snapshot_file() should have some convention for varying file names #1143

Closed
hadley opened this issue Aug 18, 2020 · 11 comments · Fixed by #1446
Closed

expect_snapshot_file() should have some convention for varying file names #1143

hadley opened this issue Aug 18, 2020 · 11 comments · Fixed by #1446
Labels
feature a feature request or enhancement snapshot 📷

Comments

@hadley
Copy link
Member

hadley commented Aug 18, 2020

So that (e.g.) you can generate snap-windows.png, snap-osx.png, and snap-linux.png on separate machines, and not have the others automatically deleted.

@hadley hadley added feature a feature request or enhancement snapshot 📷 labels Aug 18, 2020
@krlmlr

This comment has been minimized.

@arnaud-feldmann
Copy link

arnaud-feldmann commented Nov 23, 2020

I agree with that, and it's the same even for R versions. The point is that :

  • making snapshot is cool because it catches even small modifcations.
  • but catching small modifications makes also tests fragile hence it would be very useful to have some easy way to accept those modifications

With context I can use a simple trick by example context(if (R.version$major>=4 &&R.version$minor>=1.0) "plots-R-4-1" else "plots-R-4-0") at the begining of a file for vdiffr.
context being deprecated, that won't be possible anymore. Hence I'd love testthat 3.0 to provide a fine way to make the "I'm okay with that" easy without having to skip.

@beevabeeva

This comment has been minimized.

@hadley
Copy link
Member Author

hadley commented Jan 25, 2021

Maybe a suffix argument would be sufficient?

@cderv

This comment has been minimized.

@arnaud-feldmann

This comment has been minimized.

@hadley hadley changed the title expect_snapshot file should have some convention for varying file names expect_snapshot_file() should have some convention for varying file names Jul 8, 2021
@hadley
Copy link
Member Author

hadley commented Jul 8, 2021

(I now think variant is probably a better name than suffix)

Let's just flesh out what might happen in a little more detail, using expect_snapshot() as an example. Say you have this test:

test_that("can access nickname", {
  expect_snapshot(
    version$nickname,
    variant = as.character(getRversion()[, 1:2])
  )
})

And when you run it on your local machine it generates:

# can access nickname (4.0)

    Code:
      version$nickname
    Output:
      [1] "Shake and Throw"

If run on CI that uses 4.0, then the test works as usual. What happens if the test is run on 4.1? Currently:

  • The snapshot can access nickname (4.0) would be deleted
  • You'd get a warning about creating a new snapshot

I think both of those things need to change slightly:

  • Any snapshots where the variant doesn't match need to be left alone (so maybe the variant should become an h2?)
  • Instead of a warning, you should get a skip (since those are reported more aggressively, and seem like a better fit here)

It feels like we're adding some hierarchy to the snapshots — you'd only delete a snapshot if no children were created. This suggests that maybe for expect_snapshot_file() the overall snapshot should use a directory? And maybe we could do it without an extra argument, so you'd activate this behaviour by using a subdirectory in the path? OTOH that would make it less consistent with expect_snapshot() which will need a separate argument.

cc @lionel- in case he has any thoughts from his vdiffr experience.

@lionel-
Copy link
Member

lionel- commented Jul 9, 2021

That was my thought as well, instead of file suffixes this could be a folder prefix. This way we can reuse the existing snapshot mechanism without much change. UI-wise this could be implemented using a global option like testthat.snapshot_prefix that could be set globally from a helper- file or locally with local_options().

@hadley
Copy link
Member Author

hadley commented Jul 11, 2021

@lionel- Sounds like folder prefix is the way to go. So the code below would create _snaps/bar/foo.md and _snaps/bar/foo/baz.png.

# test-foo.R
test_that("a test", {
  expect_snapshot(variant = "bar")
  expect_snapshot_file("baz.png", variant = "bar")
})

And my previous example:

r_version <- function() as.character(getRversion()[, 1:2])

test_that("can access nickname", {
  expect_snapshot(version$nickname, variant = r_version())
})

Would yield _snaps/4.0/foo.md:

# can access nickname

    Code:
      version$nickname
    Output:
      [1] "Shake and Throw"

While one global variant name would make the implementation much simpler, I think it'll be a little too restrictive in practice. You might want some tests that are split out out by (e.g.) OS and others by R version, and think you'll want to keep the majority of tests variant free, so you'd then still need some way to manually opt in/out for each snapshot.

@hadley
Copy link
Member Author

hadley commented Jul 11, 2021

When you use a variant, there's no way that testthat can automatically delete outdated snaps, because there's no way for one session to know the full set of possible variants. That means only variants that correspond to test files that no longer exist can be deleted.

And it's not that we should change from a warn to a skip, it's that a test that only generates warnings should automatically generate an empty test skip (i.e. #1416).

@hadley
Copy link
Member Author

hadley commented Sep 13, 2021

Maybe it's worth supplying a vector of all variants?

hadley added a commit that referenced this issue Sep 16, 2021
Internal changes centre around new FileSnap object that manages all snapshots (including variants) for a test file. Includes new approach for snapshot cleanup that is much simpler and better tested.

Fixes #1143
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.

6 participants