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

Unit tests for pilot3utils function #13

Merged
merged 16 commits into from
Feb 29, 2024
Merged

Conversation

robertdevine
Copy link
Collaborator

@robertdevine robertdevine commented Feb 13, 2024

Using testthat( ) for automated testing of pilot3utils function. (CTRL+ SHIFT+L followed by CTRL+SHIFT+T)

@laxamanaj, @bms63 - still tidying testthat( ) for nest_rowlabels( ) and efficacy models( ) functions.
Modified all the R program names to *.r to meet regulatory specification regarding file name extensions for programs.

@robertdevine robertdevine added the enhancement New feature or request label Feb 13, 2024
@robertdevine robertdevine self-assigned this Feb 13, 2024
@robertdevine robertdevine linked an issue Feb 13, 2024 that may be closed by this pull request
@bms63
Copy link
Collaborator

bms63 commented Feb 13, 2024

Looking good!! Can you do devils::test-coverage() ?

image

@bms - Once the nest_rowlabels( ) and efficacy_models( ) unit tests are committed, test-covr will be nearly 100% - the example.r is just a stub used if someone wishes to run test_file( ) for programs along a specific path. thx.

@laxamanaj, @bms63 - any reason the lint.yml is setup this way in pilot3-utilities? lmk thx.
We'll add another unit test to get helpers.r to 100% test-covr. thx.

@bms63
Copy link
Collaborator

bms63 commented Feb 14, 2024

I just grabbed lint checker from admiral. It can be re-configured if needed or simpler r-lib one can be used

@bms63
Copy link
Collaborator

bms63 commented Feb 14, 2024

Hey Robert - sorry...styler and lintr are not aligned at the moment. We have been experiencing this in admiral - pharmaverse/admiral#2309

@bms63, @laxamanaj - successful through lintr - let's add the unit test for nest_rowlabels( ) and efficacy_models( ) then all set!

@robertdevine
Copy link
Collaborator Author

robertdevine commented Feb 14, 2024

@laxamanaj, @bms63 - a testthat example for nest_rowlabels( ) in Tplyr table is a challenge to find - possibly non-existent.
The pattern pilot3utils unit testing for nest_rowlabels( ) (i.e. primary output) involves passing in a 9x8 tibble of summary data and then check expects against the 12x7 tibble output using testthat including pval outputs in the testthat grouping. thx.

image

@bms63
Copy link
Collaborator

bms63 commented Feb 14, 2024

We could either do snapshot testing or just call it a day!!

https://testthat.r-lib.org/articles/snapshotting.html

Thanks @bms63, expect_snapshot( ) and expect_snapshot_value( ) seem appropriate because it supports an accept workflow. Calling it a day. Thx, Ben.

@bms63, @laxamanaj, @SHAESEN2 - the PASS snapshots can then be pushed to the repository as a point of truth for each of the functions then .gitignore _snaps/ so Pilot3 unit test snapshots are captured during the regulatory review as a one off. Maybe update the snapshots in _snaps when there is a new semantic version for the pilot3utils package and the battery of unit tests run again. thx.

@bms63 , @laxamanaj, @SHAESEN2 - updated the DESCRIPTION file with the testthat(3e) setting. thx.

@bms63 , @laxamanaj, @SHAESEN2, @kaz462 - all of Pilot3 Team and R Consortium Team and FDA Review Team will be happy to know the pilot3utils unit testing snapshots for the efficacy models function - the Pilot3 primary output in unit tests matches the (...wait for it.....) Pilot1 primary output. Hats off to Pilot3 Team and R Consortium Team and FDA Review Team. Just about done on this issue, we have about 100 unit tests and sample snapshots stored in _snaps directory. thx.

image

image

Copy link
Collaborator

@SHAESEN2 SHAESEN2 Feb 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we need to rename the files to make it traceable eg in this file it seems you are testing adam format functions, rather than just functions

@SHAESEN2 - yes, the filenames are autogenerated and can be changed to make traceable to a requirements traceability matrix (RTM) for pilot3utils.

The admiral package has excellent test coverage already for the functions used in pilot3. The focus in this issue has been to unit test the functions defined in the pilot3utils package source. thx.

The unit test file names can be whatever pilot3 team wishes. With test_dir( ) every source file in the
specified directory gets tested with a "dot" output representing a function 'PASS' (see below). The _snaps
directory can be used for snapshotting test results for comparison. The testthat package creates test files in the
pilot3utils package using '.R' not '.r' extension - easy enough to change extension to match the regulatory specification. The traceability aspect to match the unit test to a specific pilot3 requirement is an excellent
point. One limitation (of using snapshots) is that snapshots are not aware if an output is correct - so an expect strategy comparing unit test output against 'correct' (point of truth) is necessary. thx.

image

The snapshots appear in the _snaps directory when the expect_snapshot unit test is run. Even a slight difference in expected outcome and previous test snapshot results in an error getting issued. An example using the pilot3utils nest_rowlabels function appears below using expect_snapshot and other testthat expect suggests.
It is pretty cool but the traceability matrix is a great idea then Pilot3 can link requirement to unit test PASS and store the snapshot in the repository.

image

image

Copy link
Collaborator

@SHAESEN2 SHAESEN2 Feb 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those four lines are not testing the efficacy models?

@SHAESEN2 - yes, preparing to commit the unit test for the efficacy_models function. thx.
The four lines provide a unit test to confirm pilot3utils testthat(3e) is used - 3e is required for using snapshots.

image

Copy link
Collaborator

@SHAESEN2 SHAESEN2 Feb 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this file we can include the character formats from before.

@SHAESEN2 - are you referring to the character formats here [earlier Issues 78, 87, and 91]. We will have to unit test those from the pilot3-adam repository.

Lmk, I could set it up testthat( ) in the pilot3-adam repository and unit test functions in the source where the 'xporter' and 'na' formatters are used from before. thx.

Robert Devine added 7 commits February 20, 2024 17:23
…plyr helper function nest_rowlabels() in pilot3utils.
…plyr helper function nest_rowlabels() in pilot3utils.
…ormat, helper, adam_functions in pilot3utils.
… the format, helper, adam_functions in pilot3utils.
@robertdevine
Copy link
Collaborator Author

@laxamanaj, @bms63, @SHAESEN2, @kaz462 - we did get a successful snapshot of the efficacy models unit test. The data needed to test the function will be put into _snaps tomorrow so the GH R CMD Check doesn't fail when running the unit test in the action step to validate the PR. [The character formats @SHAESEN2 refers to are in the pilot3-adam repository - the testthat( ) setup will need to be put in over there to cover the character formats from before (adsl.r, etc.).] We're just about completed on the pilot3utils function, ~100% coverage. thx.

Copy link
Collaborator

@bms63 bms63 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OMG @robertdevine way above the call of duty!! A Pilot Legend

image

Copy link
Collaborator

@bms63 bms63 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going to call it!! LGTM!!

@bms63 bms63 merged commit 3e4dfb1 into main Feb 29, 2024
5 checks passed
@bms63 bms63 deleted the 2-write-unit-tests-for-the-functions branch February 29, 2024 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Write unit tests for the functions
3 participants