expect_warning() does not return condition

[lionel-]

class(testthat::expect_warning(warning("")))
#> [1] "character"

Unlike other expectations:

class(testthat::expect_error(stop("")))
#> [1] "simpleError" "error"       "condition"

class(testthat::expect_message(message("")))
#> [1] "simpleMessage" "message"       "condition"

class(testthat::expect_condition(signalCondition(rlang::cnd("foo"))))
#> [1] "foo"       "condition"

Tested with 3rd edition.

[lionel-]

That's because we return the return value of the expression rather than the condition, unless the return value is NULL:

library(testthat)

f <- function(out) {
  message("foo")
  out
}
g <- function(out) {
  warning("foo")
  out
}

(expect_message(f(NULL)))
#> <simpleMessage in message("foo"): foo
(expect_message(f("return value")))
#> [1] "return value"

(expect_warning(g(NULL)))
#> <simpleWarning in g(NULL): foo>
(expect_warning(g("return value")))
#> [1] "return value"

The difference in behaviour when the expression is message() vs warning() is because these signal functions have different return value in case of muffling: message() returns NULL whereas warning() returns the warning message.

[krlmlr]

We rely a lot on the original behavior in dm. Chances are that many other packages do too. Should we consistently return the input instead (except for expect_error() when there's no input to return)?

I think the examples aren't representative because a warning/message is rarely the last thing that a function does.

Interestingly, reprex gets it wrong here:

out <- testthat::expect_message({
  message("")
  "out"
})

class(out)
#> [1] "character"
out
#> [1] "out"

^{Created on 2021-05-17 by the reprex package (v2.0.0)}

With dev testthat, I see in the terminal:

d> out <- testthat::expect_message({ message(""); "out" })

d> class(out)
[1] "simpleMessage" "message"       "condition"    

d> out
<simpleMessage in message(""): 
>

[lionel-]

I think we should consistently return the condition because it is easy to use value expectations inside condition expectation whereas it's hard to both test and capture a condition. Also returning the condition composes well with expect_snapshot(). We'll need to assess revdep issues but hopefully it is moderate since it's a 3e change.

[krlmlr]

I have

expect_equal(
  expect_message(...),
  expect_message(...)
)

which is more difficult to transform.

How about a new set of expectations instead, e.g. expect_message_obj() ?

[krlmlr]

Or perhaps mark this behavior for edition 4 instead?

[lionel-]

This could be:

expect_message(out <- ...)
expect_message(exp <- ...)
expect_equal(out, exp)

Or perhaps mark this behavior for edition 4 instead?

We should probably look at revdep results before considering this?