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_error() regexpr matching parent errors #1493

Closed
romainfrancois opened this issue Nov 17, 2021 · 7 comments · Fixed by #1499
Closed

expect_error() regexpr matching parent errors #1493

romainfrancois opened this issue Nov 17, 2021 · 7 comments · Fixed by #1499
Milestone

Comments

@romainfrancois
Copy link
Contributor

As part of doing the rev dep checks for dplyr 1.0.8 tidyverse/dplyr#6080 there are some packages that start to fail because expect_error() does not match the pattern, because the message is now in a parent condition.

library(testthat)
library(rlang)
#> 
#> Attaching package: 'rlang'
#> The following objects are masked from 'package:testthat':
#> 
#>     is_false, is_null, is_true

f <- function() {
  abort("hello")
}
g <- function() {
  withCallingHandlers(f(), error = function(cnd) {
    abort("bonjour", parent = cnd)
  })
}

expect_error(g(), "Hello")
#> Error: `g()` threw an error with unexpected message.
#> Expected match: "Hello"
#> Actual message: "bonjour"
tryCatch(g(), error= print)
#> <error/rlang_error>
#> Error:
#>   bonjour
#> Caused by error in `f()`:
#>   hello
#> Backtrace:
#>  1. base::tryCatch(g(), error = print)
#>  5. global g()
#>  7. global f()

Created on 2021-11-17 by the reprex package (v2.0.1.9000)

@romainfrancois
Copy link
Contributor Author

For example, in package tbrf:

test_that("tbr_binom returns error", {
  df <- data.frame(
    date = sample(seq(as.Date('2000-01-01'), as.Date('2005/12/30'), by = "day"), 10),
    value = rbinom(10, 1, 0.25)
  )

  expect_error(df %>% tbr_binom(x = value,
                                   tcolumn = date,
                                   unit = "year",
                                   n = 5,
                                   alpha = 0.1),
                  "unit must be one of years, months, weeks, days, hours, minutes, seconds")
})

now being:

[master] 220.85 MB > df <- data.frame(
+     date = sample(seq(as.Date('2000-01-01'), as.Date('2005/12/30'), by = "day"), 10),
+     value = rbinom(10, 1, 0.25)
+ )
[master] 220.85 MB > df %>% tbr_binom(x = value,
+                  tcolumn = date,
+                  unit = "year",
+                  n = 5,
+                  alpha = 0.1)
Error in `mutate()`:
  Problem while computing `temp = purrr::map(...)`.
Caused by error in `tbr_binom_window()`:
  unit must be one of years, months, weeks, days, hours, minutes, seconds
Run `rlang::last_error()` to see where the error occurred.

@romainfrancois
Copy link
Contributor Author

Another example in srvyr gergness/srvyr#135

── Failure (test_survey_statistics.r:583:13): survey_STATISTIC functions fail with x being a character ──
`summarise(dstrataGrp, survey_ratio(api99, name))` threw an error with unexpected message.
Expected match: "Character vectors not allowed in survey functions, should be used as a grouping variable."
Actual message: "Problem while computing `..1 = survey_ratio(api99, name)`.\nℹ The error occurred in group 1: awards = No."
Backtrace:
     ▆
  1. ├─testthat::expect_error(...) at test_survey_statistics.r:583:12
  2. │ └─testthat:::quasi_capture(...)
  3. │   ├─testthat .capture(...)
  4. │   │ └─base::withCallingHandlers(...)
  5. │   └─rlang::eval_bare(quo_get_expr(.quo), quo_get_env(.quo))
  6. ├─dplyr::summarise(dstrataGrp, survey_ratio(api99, name))
  7. ├─srvyr:::summarise.grouped_svy(...)
  8. │ ├─dplyr::summarise(.data$variables, !!!.dots, .groups = .groups)
  9. │ └─dplyr:::summarise.grouped_df(.data$variables, !!!.dots, .groups = .groups)
 10. │   └─dplyr:::summarise_cols(.data, dplyr_quosures(...), caller_env = caller_env())
 11. │     ├─base::withCallingHandlers(...)
 12. │     └─dplyr:::map(quosures, summarise_eval_one, mask = mask)
 13. │       └─base::lapply(.x, .f, ...)
 14. │         └─dplyr FUN(X[[i]], ...)
 15. │           └─mask$eval_all_summarise(quo)
 16. └─srvyr::survey_ratio(api99, name)
 17.   └─srvyr:::stop_for_factor(denominator)
 18.     └─base::stop(...)

@lionel-
Copy link
Member

lionel- commented Nov 17, 2021

Since the parent messages are printed on error, it makes sense to match them. I think we should add a inherit argument that defaults to TRUE. When set, parent classes and messages would be matched.

@romainfrancois
Copy link
Contributor Author

Also Tplyr :

── Failure (Line 8): all test tables can be built without errors or warnings ─────────────────────
`build(t7)` threw an error with unexpected message.
Expected match: "object 'col_i' not found"
Actual message: "Problem while computing `col_i = fct_expand(...)`."
Backtrace:
  1. testthat::expect_error(build(t7), "object 'col_i' not found")
 21. col_i

@hadley
Copy link
Member

hadley commented Nov 24, 2021

@lionel- do you want to do this in #1491?

@hadley hadley added this to the v3.1.1 milestone Nov 24, 2021
@lionel-
Copy link
Member

lionel- commented Nov 25, 2021

@hadley I think it would be better in a separate PR. The logic in #1491 will be conditional whereas matching parent conditions should be unconditional in order to preserve backwards compatibility. I'll send a PR tomorrow.

@hadley
Copy link
Member

hadley commented Nov 25, 2021

Ok, makes sense. Thanks!

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 a pull request may close this issue.

3 participants