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

Proposed expectations: expect_nrow(), expect_ncol(), expect_dim() #1423

Open
MichaelChirico opened this issue Aug 5, 2021 · 4 comments · May be fixed by #1469
Open

Proposed expectations: expect_nrow(), expect_ncol(), expect_dim() #1423

MichaelChirico opened this issue Aug 5, 2021 · 4 comments · May be fixed by #1469
Assignees
Labels
expectation 🙀 feature a feature request or enhancement

Comments

@MichaelChirico
Copy link
Contributor

testthat::expect_length() is convenient for testing the size of vectors.

A natural extension would seem to be similar expectations for data.frames/matrices/arrays, where expectations like

expect_nrow(filter(DF), 10L)
expect_ncol(transform(DF), 5L)
expect_dim(transformation(array), c(1L, 5L, 6L))

Would come in handy. For list-extensions like data.frame, of course expect_length works, but an expect_ncol would also be more readable for this case.

Does that make sense to include in testthat itself? Happy to file the PR if so.

From a quick glance at our particular subset of CRAN, I see 413 usages of expect_equal(nrow(, 98 for expect_equal(ncol(, and 130 of expect_equal(dim(, so it's clearly used a decent amount already by downstreams.

@MichaelChirico
Copy link
Contributor Author

MichaelChirico commented Sep 8, 2021

Alternatively we might try something like expect_shape(x, .) with signature

expect_shape = function(x, dim, length, nrow, ncol) {}

And dim can be fudged to work as length for NULL-dim input. That would unify things a bit:

expect_length(x, 10)
# equivalent to
expect_shape(x, 10)

expect_equal(dim(transformation(array)), c(1, 5, 6))
# becomes
expect_shape(transformation(array), c(1, 5, 6))

expect_equal(nrow(filter(DF)), 10)
# becomes
expect_shape(DF, nrow = 10)

And so on.

Maybe the signature could be

expect_shape = function(x, shape, dim, length, nrow, ncol) {}

instead so that explicitly setting dim= doesn't unintentionally match on length(), only on dim().

@hadley
Copy link
Member

hadley commented Sep 8, 2021

My main concern is whether this is a "slippery slope" — i.e. does adding expect_shape() imply there are a large number of other expectations we also need to add? (Or that other folks might argue we should add). But I think expect_shape() seems defensible because it's around testing a key attribute that is really built-in into base R.

OTOH, I think expect_dim() would be a little easier to explain, since while I like the notion of shape, it's not something that currently exists in base R, so would need to be explained somewhere. But that would imply separate expect_nrow() and expect_ncol() which I don't love just because it increases the API surface by 3 functions for relatively rarely used cases. So all-in-all, I think I would prefer expect_shape() with multiple arguments.

Maybe it would be better to leave length as a separate expectation, then drop dim in favour of a single shape argument? You can always use expect_equal(dim(x), ...) if you want to match exactly, and this has some precedence in that expect_s3_class() is not always sufficiently precise so you use expect_equal(class(x), ...).

So I think I'm suggesting expect_shape = function(x, shape, nrow, ncol) {}. If that sounds good, a PR would be much appreciated.

@MichaelChirico
Copy link
Contributor Author

MichaelChirico commented Oct 4, 2021

SGTM! I'll work on a PR. If it's possible to assign to me, feel free to do so.

@MichaelChirico
Copy link
Contributor Author

As noted at r-lib/vctrs#1470, we could re-use expect_vector() in some cases:

testthat::expect_vector(matrix(1:10, 2, 5), ptype = rbind(integer(5)), size = 2L)

But that's pretty abstruse IMO; the proposed expect_shape() would be much clearer.

@MichaelChirico MichaelChirico linked a pull request Oct 4, 2021 that will close this issue
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

Successfully merging a pull request may close this issue.

2 participants