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

Preserve empty lines in snapshot output #1524

Merged
merged 11 commits into from
Jan 6, 2022
Merged

Preserve empty lines in snapshot output #1524

merged 11 commits into from
Jan 6, 2022

Conversation

hadley
Copy link
Member

@hadley hadley commented Jan 3, 2022

Fixes #1509

@hadley
Copy link
Member Author

hadley commented Jan 3, 2022

The root cause is that some conditions implicitly add a trailing new line, and some implicitly swallow a trailing new line. This now matches the behaviour I see on the console.

@gaborcsardi
Copy link
Member

FWIW this PR does not fix the test cases in cli. After removing the temporary cli::expect_snapshot() helper function now I get:

testthat::test_local(filter = "contain")
✔ | F W S  OK | Context
✖ | 1       6 | containers [0.3s]
────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
Failure (test-containers.R:78:3): margin is squashed
Snapshot of code has changed:
     old                 | new
[23] "  "                | "  "                [23]
[24] "  "                | "  "                [24]
[25] "  "                | "  "                [25]
[26] "  until here"      - "  "                [26]
[27] "  "                - "  until here"      [27]
[28] "  "                | "  "                [28]
[29] "  "                | "  "                [29]
[30] "  no space, still" - "  "                [30]
[31] "  "                | "  "                [31]
[32] "  "                - "  no space, still" [32]
 ... ...                   ...                 and 5 more ...

Run `snapshot_accept('containers')` if this is a deliberate change

Is this expected?

@hadley
Copy link
Member Author

hadley commented Jan 4, 2022

@gaborcsardi do you mind boiling that down to a more minimal reprex? I find it hard to reason about exactly what should occur given all the \r.

@gaborcsardi
Copy link
Member

This test was actually not broken in version 3.1.1, but this PR breaks it. It is not about \r but messages that consist of only \n characters.

Example

library(testthat)
testthat::test_that("snapshots and whitespace", {
  local_edition(3)
  msgs <- list("start\n", "\n\n", "until here\n", "\n\n",
    "still\n", "\n\n", "again\n")
  expect_snapshot(local({
    for (m in msgs) message(m, domain = NA, appendLF = FALSE)
  }))
})

testthat 3.1.1 output

i Current value:
Code
  local({
    for (m in msgs) message(m, domain = NA, appendLF = FALSE)
  })
Message <simpleMessage>
  start

  until here

  still

  again

testthat #1524 output

i Current value:
Code
  local({
    for (m in msgs) message(m, domain = NA, appendLF = FALSE)
  })
Message <simpleMessage>
  start


  until here


  still


  again

@hadley
Copy link
Member Author

hadley commented Jan 5, 2022

I think this PR is correct, because when I run this at the console I see:

start


until here


still


again

But I think evaluate or knitr gets it wrong because this is the reprex:

msgs <- list("start\n", "\n\n", "until here\n", "\n\n",
             "still\n", "\n\n", "again\n")
local({
  for (m in msgs) message(m, domain = NA, appendLF = FALSE)
})
#> start
#> 
#> until here
#> 
#> still
#> 
#> again

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

@hadley
Copy link
Member Author

hadley commented Jan 5, 2022

i.e. this is wrong:

msgs <- c("line 1", "\n", "line 4")
local({
  for (m in msgs) message(m)
})
#> line 1
#> 
#> line 4

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

@hadley hadley merged commit 8866385 into main Jan 6, 2022
@hadley hadley deleted the empty-lines branch January 6, 2022 22:47
jennybc added a commit to tidyverse/readr that referenced this pull request Jan 25, 2022
jennybc added a commit to tidyverse/readr that referenced this pull request Jan 25, 2022
* Let's get a baseline before introducing real change

I predict test failure, due to changes in testthat 3.1.2.

* Re-establish the organization, with printing tests in the printing section

* Update col_spec tests (at least for edition 2)

* Update col spec test snapshots for edition 1

* Update first edition snapsots for whitespace

I guess I'm seeing the effects of

r-lib/testthat#1509
r-lib/testthat#1524
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