Conversation
|
This is ready for review now! |
|
This looks great!! Thanks so much for doing this, Dave!! I added a few comments to tests/testthat/test-template.R about ways to make the testing a little more comprehensive. I think we may as well try to tackle these now and hopefully it won't be too much more work, but if there are any roadblocks let us know and we can push those off for later. |
|
Thanks! I had already started working locally on knitting the empty report also (no problems!) and the word versions, so will add those, will try adding bama/nab, and will see if I can get the snapshot artifact upload to work as well. |
|
great! I'll keep an eye out for updates. |
|
This is looking great!! For example, I can download the test snapshot artifacts generated in draft PR #169 by going to https://github.com/FredHutch/VISCtemplates/actions/runs/9701586982/job/26775548607?pr=169#step:8:394 and downloading and then reviewing the pdf and docx files. Yay! |
|
Yup! It's almost there, and the snapshots are getting uploaded in #169 like you say! The only thing I'm still looking into is having just the snapshots uploaded in the artifact, not the whole results tree when tests fail like what's happening now in this branch https://github.com/FredHutch/VISCtemplates/actions/runs/9701659385/job/26775775183?pr=168. There's either something I'm not fully understanding about the r-lib/actions yaml or a bug in how that works when tests fail. Hopefully more TBA soon, thanks for approving but let's hold off on merging for now. |
|
sounds good!! |
|
I'm pretty convinced now that this is happening because of the upstream yml. We'll see if they're amenable to a PR that would let us have the flexibility to always upload snapshots, even when tests fail. They already have that functionality built into the results artifact upload, just not for snapshots. |
|
sounds good, Dave! is it worth merging this PR while we wait on the r-lib folks? or is there anything you want to wait on? |
|
Yep! I'll add a few more code comments today, then merge it, then add an issue/reminder for tweaking the CI if/when upstream changes their yml. |
|
Leaving the branch here for now in case I need to refer to it during yml discussion with r-lib/actions upstream. |
|
Deleting branch since there hasn't been movement on this from upstream. Will resurrect later as needed. |
This PR sets up an automated unit test for successfully knitting a generic report PDF across all platforms. This will help with future unit test writing, and gets us a lot farther along on #136. The test is also the reprex for #159, so it will greatly help me iterate on debugging that on statsrv once I merge these changes into the #159 branch.
The CI runs on this PR are failing, but for a very good reason: They're successfully catching the 'Lonely item' latex bug across all platforms. See #169 (but don't merge it!) for a demonstration that once I combine these with Kellie's bugfix, the generic reports successfully knit on all the CI platforms.
Along the way, I had to make a few changes to a few functions to enable non-interactive use (see #136 (comment)).
Changes in this PR:
create_visc_project(),use_visc_report(), anduse_visc_methods()now have an additional argumentinteractivethat is alwaysTRUEby default, with the same behavior and UI as before. Settinginteractive = FALSEproduces the same results as before, but without any interactive side effects like automatic Rstudio window/project switching, interactive user confirmations, interactive user questions, or console outputs. The new arguments have been documented for the 3 functions.challenge_visc_name(),challenge_directory(), andchallenge_visc_report()also now have an interactive option that defaults toTRUEbut can be set toFALSEwith the same effect. These helpers are called from within the 3 exported functions above.herepackage withrprojrootto facilitate unit testing.herewasn't flexible enough to handle test knitting an Rmd file in a project package from within the VISCtemplates unit tests.