-
Notifications
You must be signed in to change notification settings - Fork 129
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
58 test configs #59
58 test configs #59
Conversation
Codecov Report
@@ Coverage Diff @@
## master #59 +/- ##
==========================================
- Coverage 100% 99.65% -0.35%
==========================================
Files 33 33
Lines 1059 1144 +85
==========================================
+ Hits 1059 1140 +81
- Misses 0 4 +4
Continue to review full report at Codecov.
|
@AlexAxthelm I really appreciate your work on this. It may be a while before I can take a close look, but it is definitely a high priority for drake. Thank you. |
@AlexAxthelm This is great food for thought. You led me to more ideas about what the tests should look like, and I will share them in #58. Just to clarify: solving #58 will require a lot of tedious re-architecting, and there are easier, more pressing issues for drake: in particular, #56, #54, #55, and maybe #60. It would be unfair to ask you to fix #58 before I merge #56. If you use something like |
I have prepared a complete solution to #74, but I will still leave this PR open in case you still want to make improvements after the next release from me. |
Can we close this PR? 4.1.0 is released and on its way to CRAN, and |
Absolutely. Looking forward to the new testing. |
Great! See 'tests/extra/multiple.R'. Feel free to submit a separate PR/issue with different design suggestions. |
This PR introduces a method for testing multiple configurations of options without too much code overhead. Hopefullly this will turn into a solution for issue #58.
I wrapped the whole testfile (I've created
test-config_basic.R
for now) in afor
loop, with the loop iterating over a nested list of options. The options that I've setup intest.R
include the environment, the parallelism methods, number of jobs, and if if should be run on CRAN or not. Currently running on travis and appveyor key off the CRAN field, but that could be a change if we want more thorough testing for non-parLapply
parallelisms.So far, it is working for the
parLapply
, andmclapply
(on supported systems), and I can confirm that it does actually skip themclapply
tests forjobs > 1
on Windows,The bad news is that right now it fails loudly for
Makefile
. (On both windows and Linux). Haven't had a chance to chase that down properly yet.Goes without saying, but please do not merge yet.