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

inital unittests for survSAKK #2

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

SAKK-vsomas
Copy link

Adding unitests for the survSAKK Package, with 91% test coverage.

@aghaynes
Copy link
Member

aghaynes commented Aug 8, 2024

Thanks for trying out the platform @SAKK-vsomas!

A couple of notes, which might or might not be important as I guess that you're just trying out the system/process for now...

  • it seems that you've added the same tests as you already have in the package... Thats not really the intention... The idea would be to add additional tests that are missing from the package. I.e. you've identified a case that the package authors haven't tested, and want/need to cover that hole.
  • where you install the package in setup.R, you need to provide the code that will actually work to install. Currently your code assumes that the package is on CRAN, which it isn't. If I try to run your tests, it will fail due to the install.
  • In practice, the info.txt file would need to be more detailed than you've currently written - a prose version of the tests that are performed.

@SAKK-vsomas
Copy link
Author

Thank you for your feedback. Can you guide me on the next steps we should take to ensure the validation process? We aim to complete this validation before the official release at the SCTO Meeting in September.
Currently, the package is not yet available on CRAN, which is our next step. At the moment, we have provided detailed setup instructions and documentation for the package on the website: https://sakk-statistics.github.io/survSAKK/

@aghaynes
Copy link
Member

aghaynes commented Aug 8, 2024

Firstly, the validation platform isn't yet fully up and running, any won't be before the meeting in September... that said, I do appreciate that you're testing the infrastructure!!

Validating a package is difficult to do on your own. The risk assessment that you entered in the pkg_validation repo should really have been done by someone not involved in the development of the package, on the grounds that, as an author, you are biased. Further, bear in mind that a validation is in the eyes of the beholder - I might validate it for a different purpose than you. E.g. I might only care about the figure, but you might want to validate the test etc that you can also add...

In this case, as the output from the function is a figure, I'm not 100% sure how to validate it. Checking for error messages is one thing (which you already do in your package, so isn't needed here), but the actual thing to check would be the figure itself. There is a package called vdiffr which you might want to use for testing (via expect_doppelganger). I've not tried it, but perhaps vdiffr tests also work within our structure (i hope so...).

If you implement the expect_doppelganger tests in your package (which would be worthwhile for you to do anyway IMO), I would think that there is no additional need for tests through the platform. The SOP says that if test code exists within the package that is sufficient, it can be used instead of tests within the platform. (i may have to add some machinery to allow this... it probably doesnt work at the moment)

As far as installing your package is concerned, there are a couple of routes you could go (most difficult first):

  1. get your package on CRAN
  2. setup an SAKK r-universe (very easy to do! See CTU Berns here, or the proper documentation here)
  3. ask whether your package could be integrated in the CTU Bern r-universe (I could do that, and would probably have done so once you officially release it in september anyway 😉)
  4. add the necessary code to install via devtools or remotes

Maybe you can also look at the other bullet points in my first comment and try to address them...

I hope that helps!

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.

2 participants