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

all= argument for expect_true()/expect_false() #1836

Open
MichaelChirico opened this issue Jul 25, 2023 · 6 comments
Open

all= argument for expect_true()/expect_false() #1836

MichaelChirico opened this issue Jul 25, 2023 · 6 comments
Labels
expectation 🙀 feature a feature request or enhancement

Comments

@MichaelChirico
Copy link
Contributor

MichaelChirico commented Jul 25, 2023

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
Copy link
Member

hadley commented Jul 26, 2023

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?

@hadley hadley changed the title FR: all= argument for expect_true()/expect_false() all= argument for expect_true()/expect_false() Jul 26, 2023
@hadley hadley added feature a feature request or enhancement expectation 🙀 labels Jul 26, 2023
@MichaelChirico
Copy link
Contributor Author

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
Copy link
Member

hadley commented Jul 27, 2023

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

This comment was marked as outdated.

@hadley

This comment was marked as outdated.

@MichaelChirico

This comment was marked as outdated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
expectation 🙀 feature a feature request or enhancement
Projects
None yet
Development

No branches or pull requests

3 participants