Conversation
|
Thanks for working on this! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1165 +/- ##
==========================================
+ Coverage 90.85% 91.14% +0.29%
==========================================
Files 14 15 +1
Lines 5924 6065 +141
==========================================
+ Hits 5382 5528 +146
+ Misses 542 537 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@jgabry do you know why macos devel would be failing in the R dep setup? Doesn't seem to happen in main--could this be a cache/GHA error? https://github.com/stan-dev/cmdstanr/actions/runs/23573417706/job/68640740296 |
Took a quick glance at the logs, I agree this seems likely. I’ll try to take a look at the actual PR tomorrow. |
|
Thanks! Looks like this (new) function in tools is broken, planning on submit a patch. Got fixed, so should be gtg soon |
jgabry
left a comment
jgabry
left a comment
There was a problem hiding this comment.
Mostly looks great, just a few comments/questions.
Still seems broken (hitting the same error in other repos too) |
|
I see builds failing, but I think the specific error that I ran into should be fixed. I think the C API changed so some packages aren't building (abind in posterior, lazyeval in bayesplot). I don't think there are any other deps which are problematic on devel for cmdstanr, so hopefully this run will go through |
It's still failing in other recent cmdstanr PRs. This one is from today: https://github.com/stan-dev/cmdstanr/actions/runs/23760099930/job/69225022781?pr=1166 Not a big deal, we can just wait for it to sort itself out I guess? |
|
Missed that. Not sure why its still failing since the code is fixed in source--maybe the R install is cached? Waiting it out seems good to me. |
|
There are still some snapshots since expect_known_output() was deprecated and the suggestion is to use a snapshot. |
|
I swapped a number of things to use withr functions instead of base R so that tests don't have to manually clean up after themselves with a bunch of on.exit() calls. withr is already a suggested import so I see no reason not to swap. |
jgabry
left a comment
jgabry
left a comment
There was a problem hiding this comment.
Looking good. Just a couple of comments/questions/suggestions. Might have more, but maybe not.
| mtime_check_enabled <- FALSE | ||
| if(length(mod$exe_file()) > 0 && file.exists(mod$exe_file())) { | ||
| original_mtime <- file.mtime(mod$exe_file()) | ||
| suppressWarnings(Sys.setFileTime(mod$exe_file(), original_mtime - 10)) | ||
| before_mtime <- file.mtime(mod$exe_file()) | ||
| mtime_check_enabled <- before_mtime < original_mtime |
There was a problem hiding this comment.
A couple more mtime_check_enabled to cleanup
jgabry
left a comment
jgabry
left a comment
There was a problem hiding this comment.
Seems ready to me if everything passes. @VisruthSK anything else you wanted to change?
|
Nope will start the snapshot PR after this is merged |
3 participants
Migrated to latest testthat edition.
Closes #1155.