Add wrap arg to test_dir for consistency with test_file (#586)
#597
Conversation
R/test-files.R
Outdated
| @@ -27,17 +27,20 @@ test_env <- function() { | |||
| #' @return the results as a "testthat_results" (list) | |||
| #' @export | |||
| test_dir <- function(path, filter = NULL, reporter = default_reporter(), | |||
| env = test_env(), ..., encoding = "unknown", load_helpers = TRUE) { | |||
| env = test_env(), ..., encoding = "unknown", | |||
| load_helpers = TRUE, wrap = TRUE) { | |||
There was a problem hiding this comment.
Do we need to document the new argument?
There was a problem hiding this comment.
No because it is inherited from test_file (@inheritParams). You can see it in the Rd file: https://github.com/hadley/testthat/pull/597/files#diff-f540ef3582dc78a36153378451f7b979.
There was a problem hiding this comment.
Oh, I did miss the @inheritParams...
tests/testthat/test-test_dir.R
Outdated
| ## not recorded in return value, even when `wrap=TRUE` (see #596") | ||
|
|
||
| w.wrap <- capture.output( | ||
| test_dir('test_dir', filter='bare-expectations', wrap=TRUE) |
There was a problem hiding this comment.
Could you please add space around =?
There was a problem hiding this comment.
Sure, will address this evening. I've tried to do it as much as possible (e.g. in the actual function definition), but old habits die hard.
There was a problem hiding this comment.
Thanks -- and double quotes ", please ;-)
tests/testthat/test-test_dir.R
Outdated
| ## test this because this test itself is in a `test_that` block, and further | ||
| ## since the whole test suite is effectively run with `wrap=TRUE` it is not | ||
| ## possible to test this at all. I ran this outside of the `test_that` call | ||
| ## and it worked as expected. |
There was a problem hiding this comment.
We could use the callr package for this. @gaborcsardi: Do you have examples for running callr from a testthat test? Does it work for R CMD check?
There was a problem hiding this comment.
Yes, you can do that, e.g. https://github.com/gaborcsardi/filelock/blob/master/tests/testthat/test.R#L27-L32
But it is kind of a big step, and brings in some dependencies as well, so I am not sure if it is worth it.
There was a problem hiding this comment.
@krlmlr let me know if you want me to do this.
There was a problem hiding this comment.
I guess we don't need it, because you will be using it all the time ;-)
|
@krlmlr, Alright, done. Also, merged in your master branch changes so the tests now pass! Let me know if there is anything else I need to do here. |
tests/testthat/test-test_dir.R
Outdated
| ## possible to test this at all. I ran this outside of the `test_that` call | ||
| ## and it worked as expected. | ||
|
|
||
| skip("untestable within test suite") |
There was a problem hiding this comment.
Forgot to mention that we don't need the skip() if we're not planning to implement a test. The rest looks good to me.
There was a problem hiding this comment.
Okay, so I'll just delete the test altogether then. I'll do that this evening.
There was a problem hiding this comment.
Oh, the test before skip() looks fine to me.
There was a problem hiding this comment.
Sorry, that's what I meant.
|
@krlmlr, I removed the skipped test. Let me know if there is anything else that needs doing. |
|
I'd rather fully understand your problem before we consider a PR. |
|
Actually I think it'll be easier for me to finish this off. |
I hope it is not too presumptuous of me to submit this PR despite negative feedback in #586. I am just hoping that because the change:
test_fileandtest_dirbroken by 37cc0d0you will consider it. Unfortunately for me I'm probably one of the very few people affected by 37cc0d0 since I happen to maintain a package that uses
withCallingHandlersintestthattests, outside oftest_thatblocks. If 37cc0d0 gets pushed to CRAN I have no way to workaround this anymore.A possible alternative to this would be to set the
test_filewrapparameter default value viagetOption, which would provide another possible work-around.Sorry for being obnoxious about this, but submitting this as a PR in the hopes you will consider it is a lot easier than figuring out how to take the ~800 tests in my package out of
testthat.Fix #586.