Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1170 +/- ##
==========================================
+ Coverage 91.14% 91.25% +0.10%
==========================================
Files 15 15
Lines 6065 6070 +5
==========================================
+ Hits 5528 5539 +11
+ Misses 537 531 -6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jgabry
left a comment
There was a problem hiding this comment.
Thank you! At first glance this looks good. I'll take another look soon to take a closer look at exactly which tests we're converting to snapshots.
For now a question: if we set cran=FALSE I assume the snapshot tests should still run on CI, right? If we do eventually submit to CRAN we can't run most of these tests because CRAN won't have CmdStan installed and probably won't let us install it on CRAN systems just to run our tests. So we'd probably end up doing what RStudio does with their tensorflow package:
https://github.com/rstudio/tensorflow/blob/main/tests/testthat.R
|
Ah yes, forgot about that. Will fix that in a bit. |
There was a problem hiding this comment.
I think this is pretty much good to go. I made one small change request and suggested adding one code comment, but otherwise I've come around to the idea that switching to snapshots for these print tests is probably for the best, despite losing the explicitness of the assertions in the old tests.
Co-authored-by: Jonah Gabry <jgabry@gmail.com>
|
Yes the main thing now is that we need to be very protective of the snaps and ensure they aren't changed spuriously. If the API does change its a lot easier to update tests. |
Yeah definitely. We need to make sure no snapshot updates are snuck into large PRs without realizing it. |
Swap some tests to use snapshots. Should be discussed.