expect_snapshot() and chained errors

[romainfrancois]

From tidyverse/dplyr#6044

expect_snapshot() is currently not showing the chain of errors (the Caused by). The typical workaround is to use the expect_snapshot() + (expect_error()) pattern

test_that("arrange() gives meaningful errors", {
  expect_snapshot({
    (expect_error(tibble(x = 1, x = 1, .name_repair = "minimal") %>% arrange(x)))

    # error in mutate() step
    (expect_error(tibble(x = 1) %>% arrange(y)))
    (expect_error(tibble(x = 1) %>% arrange(rep(x, 2))))
  })
})

[romainfrancois]

As chain errors will imminently start to appear when rlang 1.0.0 is released, this would help package maintainers to curate the errors including their ancestry.

[lionel-]

Probably need to delegate "whole-error" (not sure how to call this) generation to rlang. Related to showing rlang's whole-error message in knitr.

[romainfrancois]

Should testthat offer the err() helper, as an alias to (expect_error(<...>)) ? It is starting to appear in a few places, e.g. apache/arrow#11534

[lionel-]

Probably not since using condition expectations inside expect_snapshot() is an advanced pattern. There is also a risk that people start using err() instead of expect_error() in other places too, reducing the overall consistency of expectations across packages.

[DavisVaughan]

We also considered if expect_error() should "know" that is is inside an expect_snapshot() call, and return the error object visibly if so, rather than invisibly, which would remove the need for the parenthesis. This seems worth exploring.

Maybe expect_snapshot() can call local_is_in_snapshot() which would set some option or environment variable to TRUE and expect_error() can look for that

[hadley]

I think we could implement (and maybe export?) in_snapshot() which uses the value of is.null(getOption("testthat.snapshotter")), and then expect_condition_matching() could conditionally invisible the results.

I briefly considered whether the same principle should apply to other expectations, but since they usually invisibly return the actual value (for piping), it doesn't seem useful.

@DavisVaughan do you want to do a PR?

[DavisVaughan]

Sure

[hadley]

I think the motivating issue here has been resolved by #1491; if you want to see the full chain of rlang errors, you just need to depend on the dev version of rlang. This will become the default display at some point in the future.

I think we can move away from the expect_snapshot({ (expect_error()) }) pattern. IIUC there are three main cases:

• If you want to inspect the error message, use expect_snapshot(error = TRUE).
• If you want to check the class, use expect_error(class = )
• If you want to expect the message and check the class, use expect_snapshot_error(class = ).

(The same logic will apply to the error call after #1517.)

For the arrow example, I think the code would be mostly cleaner by using expect_snapshot_error() or expect_snapshot(error = TRUE). And I'll add expect_snapshot_warning() so you can do the same thing for warnings (#1532)

Does that sound reasonable to everyone? (I'm closing this issue, but I'll open new issues if the discussion reveals that we're still missing pieces)

[hadley]

Does expect_snapshot_error()/expect_snapshot_warning() need a code = TRUE argument in order to also show the input code that generated the error/warning? Is that why we're often using expect_snapshot()?

[hadley]

Another option would be to allow error = "class" so that you could do:

foo <- function() {
  rlang::abort("This is an error", class = "myError")
}
expect_snapshot(foo(), error = "myError")

[lionel-]

Does expect_snapshot_error()/expect_snapshot_warning() need a code = TRUE argument in order to also show the input code that generated the error/warning? Is that why we're often using expect_snapshot()?

Yes that would help I think. Also they should take ... and pass them to expect_error() / expect_warning(). We should be able to occasionally add a hard check for both the class and the message.

To be honest, I find expect_error() inside expect_snapshot() more elegant (if we end up implementing Davis' idea in #1471 (comment)) as well as practical because it doesn't require multiple snapshot functions:

1. You build snapshots by composing existing testthat operations rather than having to know about different snapshot functions.

2. I like that there is a direct correspondence between expect_snaphot({ ... }) blocks and a snapshot file. Calls like expect_snapshot_error() are easier to miss when skimming through a test file.

That said these are not huge issues and I'd be fine using the condition-specific snapshotters as outlined above, since you seem to prefer them.

[hadley]

I really don't like conditionally making expect_snapshot() returning visibly or invisibly because it feels so magical. And the creation of helpers like err() seem to suggest that explicit () is not a popular option. So that makes me feel like expect_error() inside of expect_snapshot() is not a tenable solution.

[romainfrancois]

I have come to like using expect_error() inside expect_snapshot() because it does both things. If making expect_error() print when inside expect_snapshot() is too magical, maybe we could have an argument expect_error(visible = TRUE) or something, or a sibling to expect_error().

I also like to have many expect_error() calls and other things inside a single expect_snapshot().

[hadley]

@romainfrancois you don't like (expect_error(...))?

[romainfrancois]

I sort of "tolerate" it now. It's not as bad as I perceived it first, and it does allow for both expecting the error, and showing what it looks like.

The patten might be surprising to users who have not seen it before, and it might not be clear where to go from there because there's no documentation to read for ( like we could do for an explicit argument or a wrapper function.

[hadley]

Ok, I'm going to leave as is for this release but think about again the next time we do a minor release.