Use expect_snapshot() more
#1375
Conversation
| @@ -859,9 +859,8 @@ check_new_data <- function(req, object, new_data) { | |||
| step_cls <- class(object)[1] | |||
| step_id <- object$id | |||
| cli::cli_abort( | |||
| "The following required {cli::qty(col_diff)} column{?s} {?is/are} \ | |||
| missing from `new_data` in step '{step_id}': {col_diff}.", | |||
| class = "new_data_missing_column", | |||
There was a problem hiding this comment.
What was the reason for classing this error previously?
There was a problem hiding this comment.
this was from before expect_snapshot() so they were tested with expect_error(). Classed errors were used to make sure the correct error bubbled up. Instead of relying on regex matching.
However it is the only place we use it and only for testing purposes, and for that reason I think we are better off using the unclassed errors with expect_snapshot()
also, since we were matching on class, we never checked that the {cli} syntax was correct as we didn't see the error
i'm not opposed to classed errors. And they might even be useful if cases where {tune} could match on them.
There was a problem hiding this comment.
I'm okay with removing the error class if we are not using it. I see tests in the recipes extension packages that test for that error, but you probably know without checking if they test that a recipe step from that extension package is the one throwing the classed error. 👍
There was a problem hiding this comment.
I ran a revdepcheck. it is only an issue for package i maintain so i think it is fine :) thanks for pointing it out
| @@ -95,7 +95,10 @@ test_that("non-standard roles during bake/predict", { | |||
|
|
|||
| # This should require 'date' to predict. | |||
| # The error comes from hardhat, so we don't snapshot it because we don't own it. | |||
There was a problem hiding this comment.
I think this is a good principle to rely on, i.e. I would suggest not doing the change below and similar. If you want a more thorough test than "just make sure there is a error" you could see if it's a classed error and check for that error class here.
There was a problem hiding this comment.
I'm unresolving this because it seems like you might have hit "resolved" by accident?
There was a problem hiding this comment.
good catch! have reverted it now
Co-authored-by: Hannah Frick <[email protected]>
|
This pull request has been automatically locked. If you believe you have found a related problem, please file a new issue (with a reprex https://reprex.tidyverse.org) and link to this issue. |
to close #1363
This PR:
expect_warning()andexpect_message()toexpect_snapshot()expect_error()and switched toexpect_snapshot(error = TRUE)expect_error(..., NA)toexpect_no_error(). Can't doexpect_no_condition()due to dplyr_regroup signalexpect_error()toexpect_snapshot(error = TRUE)No bad snapshots were found in the switch 🙌