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

Proper successor for expect_is(M, "matrix")? #1448

Open
MichaelChirico opened this issue Sep 15, 2021 · 6 comments
Open

Proper successor for expect_is(M, "matrix")? #1448

MichaelChirico opened this issue Sep 15, 2021 · 6 comments
Labels
expectation 🙀 feature a feature request or enhancement

Comments

@MichaelChirico
Copy link
Contributor

expect_is() is deprecated; the deprecation message mentions successors:

Sys.setenv(TESTTHAT_EDITION = 3)
expect_is(matrix(1:10), "matrix")
# Warning message:
# `expect_is()` was deprecated in the 3rd edition.
# ℹ Use `expect_type()`, `expect_s3_class()`, or `expect_s4_class()` instead

However, all of those fail where expect_is() succeeded:

expect_type(matrix(1:10), "matrix")
# Error: matrix(1:10) has type 'integer', not 'matrix'.
expect_s3_class(matrix(1:10), "matrix")
# Error: matrix(1:10) is not an S3 object
expect_s4_class(matrix(1:10), "matrix")
# Error: matrix(1:10) is not an S4 object

The core issue here is that is.object(matrix(1:10)) is FALSE even though class(matrix(1:10)) returns "matrix" or c("matrix", "array") because matrices in R somewhat strangely don't have a class set. That means testthat:::isS3(matrix(1:10)) returns FALSE.

It seems the only thing to do is:

expect_true(is.matrix(M))

Is this known / intentional behavior? I don't see anything mentioned in the documentation.

@hadley
Copy link
Member

hadley commented Sep 15, 2021

Yeah, I don't think of a matrix being an S3 class, so none of the existing helpers is a good fit. If for some reason I wanted to check something was a matrix, I'd probably do expect_equal(length(dim(x)), 2), but wouldn't you normally provide a stronger assertion about the actual dimensions?

@MichaelChirico
Copy link
Contributor Author

The length(dim(.)) check would also pass a data.frame... the same applies for just checking exact dimensions. is.matrix() allows checking a function returns the right "type" of m x n object, e.g. it makes sure we get as.matrix(mtcars) and not mtcars.

@hadley
Copy link
Member

hadley commented Sep 21, 2021

Hmmmm, good point. But is there much benefit of expect_matrix(M) over expect_true(is.matrix(M)). I suppose you'd get A slightly nicer error message?

Maybe we could consider a generalisation of #1423 and have something like:

expect_vector(x, n = 10)
expect_matrix(m, ncol = 3, nrow = 10, type = "character")
expect_array(m, dims = c(1, 5, 10))
expect_dataframe(df, ncol = 3, nrow = 50, names = c("a", "b", "c"), ptypes = list(a = character()))`

expect_dataframe() would need a bunch of work, and expect_vector() doesn't quite fit expect_matrix() because it uses vctrs terminology/definitions rather than base, but we could probably figure that out.

@MichaelChirico
Copy link
Contributor Author

Related: #1423 :)

@hadley hadley added expectation 🙀 feature a feature request or enhancement labels Dec 21, 2021
@flying-sheep
Copy link

flying-sheep commented Nov 14, 2023

The same applies to things like expect_is(x, 'call'). Checking for type alone, like expect_type(., "language") don’t cut it, as the docs for call/is.call explain:

is.language() is also true for calls (but also for symbols and expressions where is.call() is false).

I think what’s missing is expect_mode(). expect_type(), expect_s3_class(), and expect_s4_class() are not enough to cover R’s messy type system.

See the docs for inherits (expect_is’s backend):

If the object does not have a class attribute, it has an implicit class, notably "matrix", "array", "function" or "numeric" or the result of typeof(x) (which is similar to mode(x)), but for type "language" and mode "call", where the following extra classes exist for the corresponding function calls: if, while, for, =, <-, (, {, call.

I’m not even sure if expect_mode() would cut it to e.g. check if something is a (

@hadley
Copy link
Member

hadley commented Nov 14, 2023

@flying-sheep IMO most of that is out of scope for testthat because I'd say it's mostly there for historical reasons. I don't think many people rely on inherits(quote({}), "{"), and if they do, they can always use it with expect_true().

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