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

Assigning inside expect_condition() does not work #998

Closed
torockel opened this issue Mar 26, 2020 · 3 comments · Fixed by #1068
Closed

Assigning inside expect_condition() does not work #998

torockel opened this issue Mar 26, 2020 · 3 comments · Fixed by #1068
Labels
bug an unexpected problem or unintended behavior expectation 🙀

Comments

@torockel
Copy link
Contributor

Hi testthat team,
It seems like assigning does not work inside expect_condition() but does work inside expect_message() and expect_warning():

library(testthat)
foo_message <- function() {
  message("a message")
  5
}

expect_message(a <- foo_message())
a
#> [1] 5

expect_condition(b <- foo_message())
b
#> Error in eval(expr, envir, enclos): object 'b' not found


foo_warning <- function() {
  warning("a warning")
  5
}

expect_warning(c <- foo_warning())
c
#> [1] 5

expect_condition(d <- foo_warning())
d
#> Error in eval(expr, envir, enclos): object 'd' not found

Created on 2020-03-26 by the reprex package (v0.3.0)

I don’t know, if this difference is intended. However, I find it sometimes useful to do stuff like this, which is inspired by the examples of expect_message:

expect_warning(c <- foo_warning())
expect_equal(c, 5)

Created on 2020-03-26 by the reprex package (v0.3.0)

@hadley
Copy link
Member

hadley commented Apr 13, 2020

Hmmm, I'm not sure that the current behaviour is wrong. Note that the problem isn't actually assignment:

library(testthat)
a <- 1
expect_condition(a <- 2, NA)
a
#> [1] 2

Created on 2020-04-13 by the reprex package (v0.3.0)

It's that expect_condition() stops running any code after it captures the first condition. expect_message() and expect_warning() work differently as they capture all messages and warnings so evaluate all code.

@torockel
Copy link
Contributor Author

Thank you for your explanation. My expectation of the function expect_condition() was wrong. Lead by your very interesting book (https://adv-r.hadley.nz/conditions.html), I thought expect_condition() was a more general form of expect_message(), expect_warning() and expect_error()). So, I came to the (wrong) conclusion that it would behave like these functions. However, it seems to be behave different to all three. Even expect_error() does not stop after a message, which I first expected after reading your reply and because expect_error() and expect_condition() are documented together:

library(testthat)

foo_message <- function() {
  message("a message")
  5
}

expect_error(d <- foo_message(), NA)
#> a message
d
#> [1] 5

Created on 2020-04-17 by the reprex package (v0.3.0)

I seem to be not the only one, who misunderstood expect_condition() (#972). Maybe just add the sentence you wrote in your reply (“expect_condition() stops running any code after it captures the first condition.”) to the documentation of expect_condition()?

@hadley
Copy link
Member

hadley commented Jun 30, 2020

I've changed my mind: this isn't a documentation problem, it's a bug that's worth fixing.

hadley added a commit that referenced this issue Jun 30, 2020
@hadley hadley added bug an unexpected problem or unintended behavior expectation 🙀 and removed documentation labels Jun 30, 2020
hadley added a commit that referenced this issue Jul 3, 2020
`expect_error()`, `expect_warning()`, `expect_message()`, and `expect_condition()`  all work the same way - they capture the first condition that matches regexp and/or class, and allow everything else to bubble up. This slightly changes the behaviour in case of failures (e.g. you'll get an error instead of a failure), but shouldn't affect currently succesful tests, and is overall much easier to reason about.

These functions now all warn about unused arguments in ..., and the all argument has been deprecated.

Fixes #998. Fixes #1052.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug an unexpected problem or unintended behavior expectation 🙀
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants