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

Additional expectations #77

Closed
wants to merge 9 commits into from
Closed

Additional expectations #77

wants to merge 9 commits into from

Conversation

@jknowles
Copy link

@jknowles jknowles commented May 23, 2013

I think it would be more convenient if three additional expectations were available for numeric values, chiefly: is_less_than is_more_than and unequal. I have coded these up and written test for them to ensure they work.

I know this functionality is available by having the user write something like:

result <- foo(param1)
test1 <- result > expectation

test_that("Values are sensible", {
expect_true(test1)
})

But to me this is unsatisfying and it would be preferable for the user to do the following:

test_that("Values are sensible", {
expect_more_than(foo(param1), 100)
expect_less_than(foo(param1), 200)
expect_unequal(foo(param1), 159)
})

This makes testing even simpler and more straightforward as it allows tests to be written with floor and ceiling values, which are often easier to think of than exact equalities in R.

But, I understand this might be considered cluttering up the code base unnecessarily. I use these functions in my own local testthat, but thought I would send them upstream and see if they are worth inclusion in the package.

@hadley
Copy link
Member

@hadley hadley commented May 29, 2013

Thanks Jared - I'll take a look when I'm next working on testthat.

@jknowles
Copy link
Author

@jknowles jknowles commented Jun 4, 2013

Just added a quick additional test that allows the user to check actual value with a user specified tolerance toward the expected value.

@@ -0,0 +1,5 @@
.Rproj.user

This comment has been minimized.

@hadley

hadley Jun 7, 2013
Member

Can you please remove this file from the pull request?

#' expect_that(a, is_less_than(10))
#' expect_less_than(a, 10)
is_less_than <- function(expected, label=NULL, ...) {
find_expr <- function(name, env = parent.frame()) {

This comment has been minimized.

@hadley

hadley Jun 7, 2013
Member

Can you please remove find_expr?

#' a <- 9
#' expect_that(a, is_less_than(10))
#' expect_less_than(a, 10)
is_less_than <- function(expected, label=NULL, ...) {

This comment has been minimized.

@hadley

hadley Jun 7, 2013
Member

= needs spaces around it.

}
if (is.null(label)) {
label <- find_expr("expected")
}

This comment has been minimized.

@hadley

hadley Jun 7, 2013
Member

Can you please start the else on the same line as }

info = info, label = label)
}

#' Expectation: is returned value unequal to some specified value?

This comment has been minimized.

@hadley

hadley Jun 7, 2013
Member

Can you please remove this for now? It will get incorporated as part of the general negation of assertions.

This comment has been minimized.

@jknowles

jknowles Jun 7, 2013
Author

So expect_unequal and unequal should both be removed?

This comment has been minimized.

@hadley

hadley Jun 7, 2013
Member

Right

...), info = info, label = label)
}

#' Expectation: is returned value equal within a tolerance to some specified value?

This comment has been minimized.

@hadley

hadley Jun 7, 2013
Member

This is already achieveable with assert_equal - can you please remove?

@hadley
Copy link
Member

@hadley hadley commented Jun 7, 2013

As well as the smaller changes described inline, can you please add success messages in line with the new expectation function? Thanks!

@jknowles
Copy link
Author

@jknowles jknowles commented Jun 7, 2013

I'm not sure this did it. I couldn't quite figure out how to re-run all the package tests after making changes. Testing the testthat package is a bit meta and it's been awhile since I made the last changes and figured it out.

Hope this cleans things up a bit!

@hadley hadley closed this in 5d11e65 Jan 23, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants