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

Extend expect_length to use an S4 object's length method, if exists #564

Merged
merged 10 commits into from Oct 2, 2017

Conversation

Projects
None yet
2 participants
@nealrichardson
Contributor

nealrichardson commented Jan 24, 2017

expect_length is a clean, convenient shortcut for expect_equal(length(x), n), but in its current implementation, only objects that are one of R's internal array types are whitelisted. An S4 object that doesn't inherit from one of those types is forbidden, even if it has a length method defined. Aside from the inconvenience of having to fall back to expect_equal, this limitation increases the chances of writing a misleading test because, as ?length says, "[a]ll other objects (including functions) have length one."

This pull request extends expect_length to work with S4 objects that have a length method. The additional check examines whether selectMethod selects the default .Primitive("length") (which always returns 1) for an S4 object. If a length method other than the primitive is set or inherited, expect_length will proceed. Other seemingly more natural ways of checking for the existence of a method don't work: existsMethod doesn't follow inheritance and hasMethod always returns TRUE for S4 objects because the primitive method is found.

This pull request also contains a commit that updates the junit test fixture for what I am guessing is a change in the 'xml2' package. When I reset my fork to upstream/master, it failed on Travis. I suspect an 'xml2' change caused the failure because the most recent release of that package was after the most recent commit to the master branch of 'testthat'.

@@ -15,8 +15,8 @@ expect_length <- function(object, n) {
stopifnot(is.numeric(n), length(n) == 1)
lab <- label(object)
if (!is_vector(object)) {
fail(sprintf("%s is not a vector.", lab))
if (!has_length(object)) {

This comment has been minimized.

@hadley

hadley Oct 1, 2017

Member

I'd rather simply remove this check all together. Would you mind doing that?

@nealrichardson

This comment has been minimized.

Contributor

nealrichardson commented Oct 2, 2017

Check removed.

# Default for S4 objects that don't inherit a length method: always length 1
expect_success(expect_length(A(x=1:5, y=3), 1))
# B does has a length method defined

This comment has been minimized.

@hadley

hadley Oct 2, 2017

Member

This mostly tests of the implementation of length(). I think you only need the test for ExpectLengthB

This comment has been minimized.

@nealrichardson

nealrichardson Oct 2, 2017

Contributor

Tests pruned.

@hadley

hadley approved these changes Oct 2, 2017

Looks good - last thing is to add a bullet to NEWS briefly describing the change and crediting yourself.

@hadley hadley merged commit d489ba3 into r-lib:master Oct 2, 2017

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@hadley

This comment has been minimized.

Member

hadley commented Oct 2, 2017

Thanks!

@nealrichardson

This comment has been minimized.

Contributor

nealrichardson commented Oct 2, 2017

No, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment