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

Add support for check = 'equal', 'identical', 'equivalent' #16

Closed
wants to merge 0 commits into from

Conversation

@harvey131
Copy link
Contributor

harvey131 commented Dec 20, 2018

Add support, tests, and documentation for the check argument to microbenchmark to allow it to be a string 'equal', 'identical', or 'equivalent'.

The user will not have to create a separate "my_check" function for common checks.

@joshuaulrich

This comment has been minimized.

Copy link
Owner

joshuaulrich commented Dec 21, 2018

I think this is a great suggestion, thanks! A few notes:

  • Please keep functional and non-functional (e.g. whitespace) changes in separate commits. Combining them makes the diff larger and distract from the changes that need to be reviewed. That said, please avoid making whitespace-only changes. ;-)
  • Please explain all changes in the commit message. You made two changes to unit tests that are not related to this feature, but did not explain why.
  • Use a branch as the base for your pull request. Basing your PR on 'master' can make it difficult for you to update your fork if your pull request isn't incorporated verbatim (e.g. the commits are squashed into a single merge commit).

If you give me push access to your PR, I can make these changes and merge. Thanks again!

@harvey131

This comment has been minimized.

Copy link
Contributor Author

harvey131 commented Dec 24, 2018

I made the changes to the two unrelated test cases because they were not passing, and I think they were not correct. Do they pass for you? They are not related to the pull request.

As a separate proposal, would you be open to accepting a pull request with the existing tests converted a testthat file, so the tests can be run within an rstudio package easily?

@joshuaulrich

This comment has been minimized.

Copy link
Owner

joshuaulrich commented Dec 24, 2018

@harvey131

This comment has been minimized.

Copy link
Contributor Author

harvey131 commented Dec 24, 2018

OK, I don't see how this test should work; it should expect an error.

test_unit_int <- function()
{
out <- try(print(microbenchmark(NULL, unit=4)), silent = TRUE)
stopifnot(!inherits(out, "try-error"))
}
test_unit_int()

microbenchmark(NULL, unit=4)
Error in match.arg(unit) : 'arg' must be NULL or a character vector

@joshuaulrich

This comment has been minimized.

Copy link
Owner

joshuaulrich commented Dec 24, 2018

It looks like I was wrong. That test is incorrect, and does error. I don't understand why it doesn't cause an error with R CMD check. I'll get RUnit set up so there can be some confidence in the tests.

@joshuaulrich joshuaulrich force-pushed the harvey131:master branch from 7f13f97 to aa2c1a1 Dec 24, 2018
joshuaulrich added a commit that referenced this pull request Dec 24, 2018
The previous tests weren't actually testing anything, as evidenced by
the test_unit_int() function. It should have failed because it was
expecting an error, but it did not fail.

Thanks to @harvey131 for the report and patch!

See #16.
@joshuaulrich

This comment has been minimized.

Copy link
Owner

joshuaulrich commented Dec 24, 2018

I'm not sure why GitHub decided to close this... I force-pushed my updated changes to your repo before merging, so you wouldn't haven't to deal with upstream differences with master.

So, despite this saying it's "closed", it really is merged.

@harvey131

This comment has been minimized.

Copy link
Contributor Author

harvey131 commented Sep 13, 2019

Would it be possible to push this updated revision onto cran? Thanks!

@joshuaulrich

This comment has been minimized.

Copy link
Owner

joshuaulrich commented Sep 18, 2019

Yes, thanks for the reminder! I've added it to my to-do list for this weekend.

@joshuaulrich

This comment has been minimized.

Copy link
Owner

joshuaulrich commented Sep 24, 2019

microbenchmark 1.4-7 is on its way to CRAN.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.