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

58 test configs #59

Closed
wants to merge 2 commits into from
Closed

Conversation

AlexAxthelm
Copy link
Collaborator

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 a for loop, with the loop iterating over a nested list of options. The options that I've setup in test.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, and mclapply (on supported systems), and I can confirm that it does actually skip the mclapply tests for jobs > 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.

@codecov-io
Copy link

Codecov Report

Merging #59 into master will decrease coverage by 0.34%.
The diff coverage is 95.29%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
R/test.R 96.22% <95.29%> (-3.78%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f10e1c7...bb1fb50. Read the comment docs.

@wlandau-lilly
Copy link
Collaborator

@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.

@wlandau-lilly
Copy link
Collaborator

@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 testrun() in your tests of #56, and if you fix what is not working on your Windows 10 rig, I will accept your RNG seed PR.

@wlandau-lilly
Copy link
Collaborator

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.

@wlandau-lilly
Copy link
Collaborator

Can we close this PR? 4.1.0 is released and on its way to CRAN, and test_with_dir() is working great.

@AlexAxthelm
Copy link
Collaborator Author

Absolutely. Looking forward to the new testing.

@AlexAxthelm AlexAxthelm closed this Sep 2, 2017
@wlandau-lilly
Copy link
Collaborator

Great! See 'tests/extra/multiple.R'. Feel free to submit a separate PR/issue with different design suggestions.

@AlexAxthelm AlexAxthelm deleted the 58-Test_configs branch September 12, 2017 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants