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

Fix snapshots of conditions with empty lines #1511

Closed
wants to merge 1 commit into from

Conversation

gaborcsardi
Copy link
Member

It seems to me that the simplest is to treat snapshots of conditions differently. So for these we dot remove the empty lines now.

To have a workaround that works with both testthat 3.1.1 (removing the empty lines), record your test with testthat 3.1.0 or with the fix included in this PR (will be version 3.1.1.9001) and skip them on the problematic testthat versions. E.g. something like this in a helper.R file should work:

expect_snapshot <- function(...) {
  if (packageVersion("testthat") >= "3.1.1" &&
      packageVersion("testthat") <= "3.1.1.9000") {
    skip("testthat bug with snapshots")
  }
  testthat::expect_snapshot(...)
}

Closes #1509.

@hadley
Copy link
Member

hadley commented Dec 7, 2021

I think my mental model of this code must be missing something because I don't understand why an empty line corresponds to character()?

@gaborcsardi
Copy link
Member Author

I think it is because the message is a single \n with appendLF = FALSE (or an empty string with appendLF = TRUE) and this will end up being a character() when testthat breaks it apart into lines.

@hadley
Copy link
Member

hadley commented Jan 3, 2022

I think the root cause is this weirdness in strsplit():

strsplit("a\nb", "\n")[[1]]
#> [1] "a" "b"
strsplit("a\nb\n", "\n")[[1]]
#> [1] "a" "b"

strsplit("a\n", "\n")[[1]]
#> [1] "a"
strsplit("a", "\n")[[1]]
#> [1] "a"

strsplit("", "\n")[[1]]
#> character(0)
strsplit("\n", "\n")[[1]]
#> [1] ""

Created on 2022-01-03 by the reprex package (v2.0.1)

So I think a slightly cleaner fix will be in snapshot_lines(). I'll do it since I'll be working on testhat for a bit.

@hadley
Copy link
Member

hadley commented Jan 3, 2022

Closing in favour of #1524

@hadley hadley closed this Jan 3, 2022
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.

testthat 3.1.1 and empty lines in snapshots
2 participants