Mavrogiannis-Ioannis / dsmmR

GNU General Public License v3.0
0 stars 1 forks source link

Add automated tests #2

Open Bisaloo opened 7 months ago

Bisaloo commented 7 months ago

One important way to ensure your package is behaving as intended and producing the expected results is to add automated tests.

This is part of https://github.com/openjournals/joss-reviews/issues/5572 as JOSS policy states:

Authors are strongly encouraged to include an automated test suite covering the core functionality of their software.

Good resources to get started with automated tests in R packages:

Please let me know if you'd like additional assistance or guidance to set this up.

Mavrogiannis-Ioannis commented 7 months ago

Hello @Bisaloo. There are extended automated tests in place, that run when a function is used. You can see them mostly in the utils.R file. These are automatically used when the examples are run, when the package is being checked with Cran etc. So, if there is no error, they work perfectly. Please Le me know if this is okay with you. Best regards, Ioannis

Bisaloo commented 7 months ago

Hi, thanks for your answer. I'd recommend migrating them to the standard testing location at some point but I believe this is not a blocking issue for the JOSS publication.

Could you however increase the code coverage? It seems some portions of the code are not tested:

covr::package_coverage(type = "examples")

dsmmR Coverage: 61.12% R/generic_functions.R: 51.10% R/parametric_estimation.R: 55.00% R/utils.R: 57.60% R/nonparametric.R: 76.83% R/parametric.R: 79.07% R/fitmodel.R: 81.30%

Mavrogiannis-Ioannis commented 7 months ago

Thank you for your response. Can you show me one example where the code fails to recognize a mistake, so that I know what to look for? Thank you a lot, Ioannis

Bisaloo commented 7 months ago

I think we are talking about two different things.

From what I understand, you are talking about input checking: ensuring that the user gets a meaningful error message if they pass invalid parameter.

I'm talking about automated tests that automatically verify that the code is working as intended and that future updates to the code don't accidentally introduce a bug that could go undetected.

Or am I misunderstanding?

Mavrogiannis-Ioannis commented 7 months ago

Okay I understand. Yes, they are different things. However there is a small overlap, as I understand it. All the test cases (that a normal user would come across) seem to be included in the examples... So most of the bugs would show up there.sa

What you propose is of course the most robust way to go about it, but it will require a lot of coding. Do you think we have enough right now?

Also, which functions should we first make automated test for? Personally I think the fit function should be first, then the simulate function, then the get_kernel function, then the parametric non/ parametric and finally the utils.R file.

Thank you for your input and your constant support. Ioannis

Bisaloo commented 7 months ago

Okay I understand. Yes, they are different things. However there is a small overlap, as I understand it. All the test cases (that a normal user would come across) seem to be included in the examples... So most of the bugs would show up there.sa

What you propose is of course the most robust way to go about it, but it will require a lot of coding. Do you think we have enough right now?

Agreed on both points. Your current approach indirectly serves as tests and I don't think there is a need to switch right now. I do encourage you to do it at some point but this is not a blocker for JOSS.

Also, which functions should we first make automated test for? Personally I think the fit function should be first, then the simulate function, then the get_kernel function, then the parametric non/ parametric and finally the utils.R file.

You should investigate which portions of the code are never reached by your examples. This can be done with the covr package. For example, the following snippet with list the lines that examples never execute:

covr::package_coverage(type = "examples") |> covr::zero_coverage()
Mavrogiannis-Ioannis commented 6 months ago

Hello @Bisaloo , thank you again for this helpful comment. I run the code, and it gave me more insight about what causes the coverage being so "low", and also which parts of the code the examples never reach.

So it makes sense that the above are not reached by the examples; the examples are well-constructed, and showcase the normal usage, not explicitly the extreme testing under the hood.

I will look into this further, but perhaps when I decide to implement automated tests.

Thank you again for giving insight into this aspect of the code! Respectfully, Ioannis

Bisaloo commented 5 months ago

Hi @Mavrogiannis-Ioannis, thanks for checking!

I agree it's good enough at this point but I strongly encourage you to revisit in the future. A proper testing framework such as the testthat R package, and in particular this category of functions, would help you write tests more easily.

So it makes sense that the above are not reached by the examples; the examples are well-constructed, and showcase the normal usage, not explicitly the extreme testing under the hood.

I understand your approach but there is value in testing edge cases as well. You do want to verify that the function returns 1. an error with 2. a sensible error message when incorrect input is passed by the user. It's easy, and no so uncommon, to trip here because of typos, etc. Something that you want to result in an error may actually slip through without you noticing.

Bisaloo commented 4 months ago

Hi @Mavrogiannis-Ioannis, reviewing this issue after @strevezas' comments, I realize that you do check that functions run without erroring but you do not check that they return the correct output, and that this output is not accidentally modified from one version to the other.

I think this is a serious issue enough that it needs to be fixed before acceptance.