Make error from finding arguments in '...' in expect_condition (et al) more informative

[MichaelChirico]

Closes #2054. Closes #2053.

Initial proposal for how to update the code. I can add / update tests later on but this is (should be?) reviewable already.

[hadley]

Would you mind putting together a couple of examples just showing before/after errors?

[MichaelChirico]

Sure, here's the example from #2054

library(testthat)
local_edition(3)
expect_error(stop("hello"), msg="bye")

## CURRENT main
# Error in `expect_error()`:
# ! Can't specify `...` without `pattern`.
# Run `rlang::last_trace()` to see where the error occurred.

## check-dots-informative
Error in `expect_error()`:
! Found arguments in `...` to be passed to `grepl()`, but no `regexp`.
• First problematic argument:
ℹ msg
Run `rlang::last_trace()` to see where the error occurred.

Here's the adversarial one I cited in the comment:

expect_error(foo(), NULL, "error", 1)

## CURRENT main
# Error in `expect_error()`:
# ! Can't specify `...` without `pattern`.
# Run `rlang::last_trace()` to see where the error occurred.

## check-dots-informative
# Error in `expect_error()`:
# ! Found unnamed arguments in `...` to be passed to `grepl()`, but no `regexp`.
# Run `rlang::last_trace()` to see where the error occurred.

There are also some snapshot changes checked in now.

[MichaelChirico]

(aside, I used a devcontainer in my branch to get this while I'm away from a machine with a "normal" dev environment, that's here in case you'd also want to host that here: https://github.com/MichaelChirico/testthat/tree/devcontainer, see .devcontainer directory which is a copy of the same from {lintr})

[hadley]

I feel like the errors still need a little bit more context. I can't quite figure out how to put this concisely so I'm just going put it all in words and then we can try and make it more succinct.

You are not using the regexp argument, which means you're matching based on class, which doesn't take any extra arguments.
You, however, have supplied arguments which will be ignored, so is probably a mistake.

Framing it in terms of the argument you are not using is confusing, so maybe an error like this would be more clear:

expect_error(f(), class = "foo", 1)
#> Error in `expect_error():
#> * You can't supply extra arguments when matching only on `class`.

(The only is necessary here since it's technically possible, albeit unusual, to match on both class and regexp)

I guess we also need to consider the case where you're just testing for the presence of an error:

expect_error(f(), foo = 1)
#> Error in `expect_error():
#> * You can't supply extra arguments when testing for the presence of an error.

Do you think it's particularly important to describe exactly what unexpected arguments were supplied?

[MichaelChirico]

Hm, the part about class= is a bit of a curveball TBH (not that you're not 100% right).

I came at this slightly differently, which is encountering many users making the mistake of providing the wrong argument name when matching on message for what should have been named regexp.

IIRC the original context is that doing so used to just work despite the mistake, as the argument wound up in ... and then was ignored (IIRC?), so that actually the user was just testing expect_error(foo()) instead of expect_error(foo(), "exact wording"), which goes unnoticed because of course you wrote your test with a specific wording in mind, so no further investigation when the test succeeds.

It's possible #2054 is now obsolete, since it was related to a breaking change from 3.2.1.1 to 3.2.3 where that implicit behavior stopped working.

[hadley]

Ooooh, I see. That was not what I was thinking of.

[hadley]

@MichaelChirico take a look at this proposal and let me know what you think

[MichaelChirico]

looks good 👍

[MichaelChirico]

I'm not sure how actionable this is, OTOH, I think it's unlikely to arise organically anyway (this is very strange code). Not worth spending more time on it IMO.

[MichaelChirico]

LGTM, thanks!

[MichaelChirico]

ah, in case it matters, I just came across this admonition to continue avoiding ...names() until 4.1.3 😱

r-lib/lintr#2502

https://github.com/r-lib/lintr/blob/dc8f14996fc303d40f120eea6d56fb54187dbc9c/R/lint.R#L43