Missing unit tests - Githubissues

kdpeterson51 / mbir

GNU General Public License v2.0

1 stars 2 forks source link

Missing unit tests #2

Closed kellieotto closed 5 years ago

kellieotto commented 6 years ago

This is detailed feedback for your JOSS review openjournals/joss-reviews#746.

R packages should have a tests/ directory containing unit tests. Hadley Wickham’s R packages book has a whole chapter explaining how to do this.

Better yet, you could set up a continuous integration system with Travis CI and add a badge to your README.md that shows that your package passes all of the tests.

kdpeterson51 commented 6 years ago

@kellieotto, thank you for your patience. mbir has successfully with Travis CI.

Please inform me if there is anything you may need from me.

Again, thank you.

kellieotto commented 6 years ago

Great, Travis CI looks like it's working!

Your package still needs comprehensive unit tests for its functions. I'll point you to some resources on how to do this. Please let me know if you need more information.

http://r-pkgs.had.co.nz/tests.html https://journal.r-project.org/archive/2011-1/RJournal_2011-1_Wickham.pdf https://testthat.r-lib.org/index.html

kdpeterson51 commented 6 years ago

@kellieotto @leeper, a tests folder has now been updated.

kellieotto commented 6 years ago

Good start. I hate to nitpick but these unit tests aren't comprehensive. They check that your functions are internally coherent with each other, but only for one example each. I'd like to see tests for more (ideally all) of your functions, more than one test for each, and testing that the outputs of your functions match some external reference (i.e. not just that they match each other).

leeper commented 6 years ago

I agree.

kellieotto commented 6 years ago

I see that you've added some new tests. These look great. Again, I'd like to see tests for more (ideally all) of your functions and testing that the outputs of your functions match some external reference.

leeper commented 6 years ago

Just to keep track, I see tests of the following:

[x] aipe_smd
[x] boot_test
[x] corr
[ ] corr_diff
[x] corr_test
[ ] es_convert
[ ] odds
[ ] prop
[x] smd
[x] smd_test
[x] ss_corr
[x] ss_odds
[x] ss_smd
[ ] swc_ind

It would be good to see tests of the remaining functions.

leeper commented 6 years ago

@kdpeterson51 Just a ping. It would be great to see this final issue of testing resolved, as we're very close to being able to accept this.

kellieotto commented 5 years ago

Looks like the functions with unchecked boxes can be checked off now.