search

tuomaseerola / onsetsync

Onset synchrony functions
Other
2 stars 0 forks source link

Automated tests - JOSS review #5

Open mrocamora opened 1 year ago

mrocamora commented 1 year ago

@tuomaseerola According to editor (@faroit) automated test are preferred but not absolutely mandatory if step-by-step instructions are provided. In this case however, he suggests to add a minimal set of automation (e.g. package installation, running).

tuomaseerola commented 1 year ago

Indeed. The first 3 code block cover this (#1 is install, #2 is loading data, #3 is visualising the onsets). Moreover, the README.md does this also. Unit-tests have also been added to the package to make sure development does not break calculations, see: https://github.com/tuomaseerola/onsetsync/tree/R1/tests/testthat

faroit commented 1 year ago

@tuomaseerola I am not that familiar with the R ecosystem but it looks like there are best practices for R when it comes to automated testing https://github.com/bradleyboehmke/unit-testing-r

Moreover, the readme has instructions how to run the software but I fail to see where actual tests are called. Can you let us know where this is happening, please?

tuomaseerola commented 1 year ago

@faroit This is valuable idea and while the devtools::check will run all code automatically and report conflicts and errors, an explicit unit-testing would make the actual calculation more reliable against version changes and other issues. We have now added a standard 'testthat' workflow for unit testing for (1) reading data, (2) asynchrony calculation, (3) plotting, and (3) periodicity. These now report positively:

devtools::test() ℹ Testing onsetsync ✔ | F W S OK | Context ✔ | 2 | 01-read-data
✔ | 2 | 02-asynchrony
✔ | 1 | 03-figures
✔ | 1 | 04-periodicity
══ Results ══════════════ Duration: 0.3 s

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

The unit tests are now included in the new release (code is in the \tests subfolder). https://github.com/tuomaseerola/onsetsync/tree/R1/tests/testthat