make exit conditions from running test_dir() explicit

[jameslamb]

Thanks for this great package!

I'd like to recommend a small (non-user-facing) change following the discussion in r-lib/testthat#912. Currently, devtools::test() implicitly relies on the fact that the default for argument stop_on_failure in testthat::test_dir() is FALSE.

The default behavior of devtools::test() is to return successfully if any tests fail, e.g. Rscript -e "devtools::test()" will return exit code 0 if you have failing tests.

Since stop_on_failure in testthat::test_dir() so meaningfully determines the behavior of devtools::test(), I think it would be valuable to supply stop_on_failure = FALSE explicitly in calls made by devtools.

I hope you'll consider this PR. Thanks!

[jimhester]

I think we can just call it with stop_on_failure = FALSE directly.

[jameslamb]

Sorry, could you elaborate Jim? What do you think should be changed?

My PR does propose calling testthat::test_dir() with stop_on_failure = FALSE directly.

[jimhester]

I think this can just be

testthat_args <- list(test_path, filter = filter, env = env, stop_on_failure = FALSE, ... = ...)

and I think you can remove the comment.

[jameslamb]

ha oh ok, got it! Was trying to be consistent with how you handle env. I can update it to just add this argument.

[jameslamb]

ok changed this in a215003. I also rebased this PR to the most recent state of master

[jameslamb]

I don't think the CI failures (specifically Travis) are a result of my PR here. I see this in the logs during setup

The command "sudo add-apt-repository -y "ppa:marutter/rrutter3.5"" failed and exited with 1 during .

[jimhester]

Great work, this package is now better thanks to you!