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

feature request: add expect_na function #291

Closed
richierocks opened this issue Sep 2, 2015 · 7 comments
Closed

feature request: add expect_na function #291

richierocks opened this issue Sep 2, 2015 · 7 comments

Comments

@richierocks
Copy link

You can test for TRUE or FALSE using expect_true and expect_false respectively. For completeness, it would be nice to have expect_na for testing for missing values.

At the moment, you can do this with expect_equal(x, NA), but this requires a little care since different NAs are treated as different. For example expect_equal(NA, NA_character_) will fail. expect_na should treat them as the same (or have an argument to specify whether or not they should be considered the same).

@hadley
Copy link
Member

hadley commented Sep 24, 2015

What would you expect expect_na(c(NA, TRUE)) to do?

@richierocks
Copy link
Author

In my opinion

  1. All five missing values (NA, NA_real_, etc.) should pass.

  2. Values that don't have length one should fail. You could document or have an example to say that vector return values should be combined with any or all, or use the more general expect_equal.

    The alternate behaviour is to work like if and throw a warning

    the condition has length > 1 and only the first element will be used

    I don't think that this is right though. It's easier to work with if tests simply pass or fail rather than throw warnings.

  3. Recursive return values like list(NA) and data.frame(NA) should fail.

  4. The string "NA" should fail. You might need to be careful about coercing to logical.

  5. Attributes are OK, For example, c(na = NA) and structure(NA, class = "whatever") and matrix(NA) should pass. (Be careful about testing with identical.)

expect_true, expect_false, and expect_na form a triumvirate, so you need to make sure that the behaviour is consistent across all three functions.

@hadley
Copy link
Member

hadley commented Sep 25, 2015

Hmmm, if expect_true, expect_false and expect_na form a triumvirate, then the definition of expect_na should use identical(x, NA)

@richierocks
Copy link
Author

Maybe identical(x, NA) || identical(x, NA_real_) || identical(x, NA_character_) || identical(x, NA_integer_) || identical(x, NA_complex_)
then.

Though an option for all three to ignore differences in attributes would be
nice. Possibly worth looking at is_identical_to_na in assertive.base as a
parallel.

https://bitbucket.org/richierocks/assertive.base/src/35037c57c024860f19fbc624b385c75b52b38f35/R/is-identical-to-true-false-na.R?fileviewer=file-view-default

On 25 September 2015 at 14:52, Hadley Wickham notifications@github.com
wrote:

Hmmm, if expect_true, expect_false and expect_na form a triumvirate, then
the definition of expect_na should use identical(x, NA)


Reply to this email directly or view it on GitHub
#291 (comment).

Regards,
Richie

Learning R http://shop.oreilly.com/product/0636920028352.do
4dpiecharts.com

@hadley
Copy link
Member

hadley commented Sep 25, 2015

That just doesn't feel terribly compelling/systematic to me. You might as well do expect_equal(is.na(x), TRUE)

@richierocks
Copy link
Author

Yes, you can always use expect_equal, so this feature is pure syntactic sugar (just like expect_true). I think it's useful sugar though - testing weird edge cases is important so having an NA return value is (or should be) moderately common.

@hadley
Copy link
Member

hadley commented Sep 28, 2015

In that case I think you'd be better testing the type of the NA value too.

@hadley hadley closed this as completed Sep 28, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants