JOSS review - Automated tests

[craig-willis]

Review issue: openjournals/joss-reviews#4707
Branch reviewed: release-0.2.4

Problem
JOSS strongly recommends using Automated tests, which this repo supports through the use of testthat and Github actions. I expect that the current implementation meets this requirement, but do have some comments:

• The completeness of the tests is unclear with the current implementation.
• Github action output suggests a potential problem with package conflicts.
• Automated tests are not documented (outside of the Github action configuration).

Test Coverage
I'm not very familiar with R test automation, but I was able to combine testthat with covr to get a brief coverage report:

covr::package_coverage(
  type = "none",
  code = "testthat::test_check(
'dtrackr'
)")

dtrackr Coverage: 56.18%
R/dot.R: 13.51%
R/dtrackr.R: 60.72%

Without looking at the test implementations in detail, this suggests that not all of the dtrackr functions are currently tested. JOSS makes no recommendations for automated test coverage rates.

Package conflicts
The test output shows conflicts between dplyr and dtrackr functions. I may be misunderstanding, but shouldn't dtrackr be load after tidyverse to test the overridden dtrackr methods?

Documentation
It took a little effort to figure out how to run the automated tests. For a contributor or user, it might be helpful to have a section in the documentation briefly describing the test automation approach (testthat), Github action, as well as how to install and run locally (see my comment about dependencies in #12).

[robchallen]

Package conflicts:

You are right, that was a mistake, however as the dtrackr methods add S3 methods on top of the dplyr functions it actually didn't make any difference (although could have done). Anyway 've fixed this issue.

Documentation:

Sorry that was an effort to set up. I've added a section in the README about running the tests and the CI setup with github workflows.

Coverage:

One of the so far little used functions in the R/dot.R script was not correctly tested, due to a missing example script. This I have fixed. Usefully this threw up a couple of issues, paritcularly saving content to png files, which I have addressed.

In terms of the other coverage, test_check checks the testthat unit tests only. For 'dtrackr' a lot of the automated testing functionality is in the examples in the man pages which are run when aR CMD check is done either manually or by the github workflows. Additionally the vignettes are also a third level of testing, again triggered by a R CMD check. This is better documented in the README file

When the 3 types of test are combined together the coverage is much improved:

covr::package_coverage(type="all")
# dtrackr Coverage: 79.50%
# R/dot.R: 78.12%
# R/dtrackr.R: 79.63%

I've included the man page examples functional tests into the testthat framwork, and together there are 90 unit and functional tests:

✔ |  51       | examples [17.6 s]                                                                                                                                                                                                                                       
✔ |   2       | group_by [1.7 s]                                                                                                                                                                                                                                        
✔ |  11       | p_comment [2.4 s]                                                                                                                                                                                                                                       
✔ |   7       | p_exclude [2.3 s]                                                                                                                                                                                                                                       
✔ |   5       | p_group_modify [1.4 s]                                                                                                                                                                                                                                  
✔ |   5       | p_include [2.2 s]                                                                                                                                                                                                                                       
✔ |   4       | p_others [1.5 s]                                                                                                                                                                                                                                        
✔ |   5       | p_status [2.1 s]                                                                                                                                                                                                                                        

══ Results ═════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════
Duration: 31.3 s

[ FAIL 0 | WARN 0 | SKIP 0 | PASS 90 ]

WHOA - stupendous code.

Some of the functions that are not tested are very thin wrappers around the dplyr functions - essentially simple pass through code. Test coverage could still be improved and it is an open issue on the tracker.

I'll work towards integrating the covr output into the package in the future. I believe there is a badge for this but I haven't used this before.

[robchallen]

Updated branch: joss-fixes-0.2.4.9000 (and main)

As I implemented a new set of functionality for #14 I also extended the functional testing (in the form of additional examples). This has pushed coverage up a bit:

covr::package_coverage(type=“all”): dtrackr Coverage: 83.60%, R/dot.R: 78.12%, R/dtrackr.R: 84.08%

[craig-willis]

Revised branch: joss-fixes-0.2.4.9000

Everything works for me as described above and in #12 (comment). Not a big deal, but you could easily add the covr command to the README, since it works now with a single additional dependency.

[robchallen]

Just to update on this I found out the right magic incantation to get the covr output hooked up to github actions and the whole thing displayed as a badge on the README. simple when you know how :-)