`all=` argument for `expect_true()`/`expect_false()`

[MichaelChirico]

expect_true(all(.)) is a very common construction. Here's >10,000 hits on GitHub:

https://github.com/search?q=%22expect_true%28all%28%22&type=code

I propose adding all= as an argument to expect_true(). The advantage is that testthat can offer a better test failure interface than is currently available.

Compare:

test <- c(rep(TRUE, 10), FALSE)
expect_true(all(test))
# Error: all(test) is not TRUE

# `actual`:   FALSE
# `expected`: TRUE 

Not very helpful (NB: this is the same in 2e and 3e as of now). Proposed interface:

expect_true(test, all = TRUE)
# Error: test are not all TRUE

# `actual` is FALSE at index 11

I don't see this as often for expect_false(all(.)), but for consistency we could add the argument there. We could also add an any= argument to handle expect_true(any(.)), which is not uncommon, though maybe it's better served with something similar to expect_match: all = TRUE --> require all(), all = FALSE --> require any(), all = NULL --> current behavior, i.e. require identical(., TRUE).

---

For reference, this was triggered in particular by glancing through logs to try and debug this:

topepo/caret#1345

Our logs showing the current failure makes it a lot harder to think about what might be going wrong without actively debugging.

[hadley]

I think I'd something more like mode = c("one", "all", "any") (I don't love mode as a name, but I like the idea of making this an enumeration).

Maybe expect_true(x, mode = "all") could be equivalent to expect_equal(x, rep(TRUE, length(x)) so you get the a nice display without much extra effort?

[MichaelChirico]

I don't love mode as a name

Agree it collides too much with mode(). How about expect_true(x, required = "one") (or "any" or "all")?

Maybe expect_true(x, mode = "all") could be equivalent to expect_equal(x, rep(TRUE, length(x))

Nice! That makes sense. Though I'm not sure there's anything so slick for required = "any" (check that expect_equal(!x, rep(TRUE, length(x))) fails, and manipulate the output? seems clunky). Should we special-case any() with some custom logic, or just work it out for both any+all since the code will be similar for both cases?

[hadley]

I like required, but last night it occurred to me that required = "one" might convey the same meaning as required = "any". So maybe required = "exactly_one" or similar might be better? I think it's ok to be a bit long because it'll be the default so people mostly won't have to type it.

Agree that there's no obvious expect_equal() hack for any, so it might be better to just be consistent.

[olivroy]

Maybe a comment on that. (I know this is not a testthat bug)

This is also documented in all(), but still a dangerous practice :)

... zero or more logical vectors. Other objects of zero length are ignored, and the rest are coerced to logical ignoring any class.

I found a faulty test somewhere that made me cautious of expect_true(all(...)) pattern that I see in many tests.

Base R behavior can be somewhat counterintuitive.

Here is what I saw.

# Trying to test that all values are in c(0, 1)
expect_true(all(x == c(0, 1)))

I know there are now better ways to do that. expect_setequal(), expect_contains() etc. but many people are still relying that this will work.

Of course replacing == by %in% makes this a valid test.

https://github.com/rich-iannone/DiagrammeR/blob/3820ba25862b4af479d9d86879d34a310548029f/tests/testthat/test-get_paths.R#L88

However, I found out that for example if x <- integer(0) the test will pass silently.

I know there is probably not much testthat can do about this. but I also saw that in a scales PR. r-lib/scales#360 (comment) FWIW.

[hadley]

@olivroy IMO the danger there comes from ==, not all().

[MichaelChirico]

@olivroy that seems like something more appropriate for {lintr} to help with.

[MichaelChirico]

Was just musing about the limitations of expect_match()'s all= signature.

• expect_match(all = FALSE) is not exactly the clearest way to say any() match should pass -- adding any = to the signature would help, though we should always be wary of adding redundant arguments to the signature
• It would be nice to always recommend expect_match() as a replacement for expect_identical(grep(pattern, x), idx), but the latter test does contain more information (whether this is always a good thing depends on context). It might be nice for expect_match() to get an index= argument to allow requiring which elements of object should match regexp.
  • I get about 700 hits for expect_{equal,identical}({grep,str_detect}(: https://github.com/search?q=lang%3AR%20%2Fexpect_(equal%7Cidentical)%5C((grep%7Cstr_detect)%5C(%2F&type=code
• But then, maybe all 3 of these approaches can be subsumed into one argument; required = "all", required = "any", required = 1:3 looks decent.
• It would also, then, combined with this FR, get more in sync with expect_{true,false}.

[hadley]

I'd love a PR for this (maybe as part of TDD) — for now I think we can go with required = c("one", "any", "all"), and I think it's probably best to hand-write messages for each one.

One thing to think about is expect_true(1:10, required = "any") — I think you'd probably want an early failure if the input wasn't a logical vector. (I don't think it can be an error because that would conflict with the existing behaviour of expect_true(1:10)).