testthat 3e by VisruthSK · Pull Request #1165 · stan-dev/cmdstanr

VisruthSK · 2026-03-24T17:37:12Z

Migrated to latest testthat edition.

Closes #1155.

jgabry · 2026-03-24T18:22:04Z

Thanks for working on this!

codecov-commenter · 2026-03-24T21:24:53Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.87%. Comparing base (5809552) to head (f7868cc).
⚠️ Report is 7 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1165      +/-   ##
==========================================
+ Coverage   90.85%   90.87%   +0.02%     
==========================================
  Files          14       15       +1     
  Lines        5924     5938      +14     
==========================================
+ Hits         5382     5396      +14     
  Misses        542      542

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

… potentially

VisruthSK · 2026-03-26T03:29:37Z

@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

VisruthSK · 2026-03-26T22:16:03Z

Looks like something is broken in R Core or pak which is causing macos devel to fail.

jgabry · 2026-03-26T22:39:13Z

Looks like something is broken in R Core or pak which is causing macos devel to fail.

Took a quick glance at the logs, I agree this seems likely. I’ll try to take a look at the actual PR tomorrow.

VisruthSK · 2026-03-26T22:40:28Z

Thanks! Looks like this (new) function in tools is broken, planning on submit a patch.

Got fixed, so should be gtg soon

jgabry

Mostly looks great, just a few comments/questions.

tests/testthat/helper-custom-expectations.R

tests/testthat/test-fit-vb.R

tests/testthat/test-install.R

jgabry · 2026-03-30T19:35:58Z

Got fixed, so should be gtg soon

Still seems broken (hitting the same error in other repos too)

VisruthSK · 2026-03-30T20:15:43Z

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

jgabry · 2026-03-30T20:55:53Z

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?

VisruthSK · 2026-03-31T02:59:00Z

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.

VisruthSK · 2026-04-01T01:02:32Z

There are still some snapshots since expect_known_output() was deprecated and the suggestion is to use a snapshot.

VisruthSK · 2026-04-01T14:48:04Z

tests/testthat/helper-custom-expectations.R

 expect_not_true <- function(...) expect_false(isTRUE(...))
+
+transform_print_snapshot <- function(x) {
+  vapply(x, function(line) {


These aren't needed for this PR, but left them in for the snapshot PR as they're helpful in normalizing results.

Is there a reason to put them in this PR as opposed to the snapshot PR where they'll actually be used? I guess I can look at them as part of this PR, but I would kind of prefer to review them as part of the PR where they are used, so I can see them in use when reviewing them.

VisruthSK · 2026-04-01T14:52:33Z

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

Looking good. Just a couple of comments/questions/suggestions. Might have more, but maybe not.

jgabry · 2026-04-01T16:39:24Z

tests/testthat/test-model-data.R


  # would error if fitting failed
-  expect_silent(fit$draws())
+  expect_no_error(fit$draws())


I think testthat 3e still supports expect_silent() right? Any reason to weaken this test to expect_no_error()?

jgabry · 2026-04-01T17:32:22Z

tests/testthat/helper-custom-expectations.R

    fail(sprint("Model executable '%s' does not exist after compilation.", mod$exe_file()))
  }
-  if(!is.null(before_mtime)) {
+  if(!is.null(before_mtime) && mtime_check_enabled) {


What's the motivation for adding the new mtime_check_enabled?

jgabry · 2026-04-01T18:08:05Z

tests/testthat/helper-models.R

+local({
+  stan_files <- dir(test_path("resources", "stan"), pattern = "\\.stan$", full.names = TRUE)
+  exe_files <- cmdstanr:::cmdstan_ext(cmdstanr:::strip_ext(stan_files))
+  existing_exe_files <- exe_files[file.exists(exe_files)]
+  if (length(existing_exe_files) > 0) {
+    unlink(existing_exe_files, force = TRUE)
+  }
+})
+


Seems useful, but was this added because we weren't starting from a clean slate before or just as an extra precaution?

Actually, we already have teardown-remove-files.R, which does something similar that runs after the tests.

I think maybe the best approach is to replace both the new code here and the teardown file with a single setup file that does both jobs in one place? I think both instead of one or the other because the pre-run cleanup is sufficient but the post-run teardown is nice especially if running tests locally.

What do you think?

So we could have a function defined in setup.R that cleans up the files and then call it twice in setup, deferring the second one? Something like this maybe:

cleanup_stan_exes() withr::defer(cleanup_stan_exes(), testthat::teardown_env())

jgabry · 2026-04-01T18:10:40Z

tests/testthat/test-json.R

-  json_output_df <- readLines(temp_file_mat)
-  expect_identical(json_output_df, json_output_mat)
+  expect_identical(readLines(temp_file_df), readLines(temp_file_mat))
+  announce_snapshot_file(name = "json-df-matrix.json")


Just curious why this is needed?

jgabry · 2026-04-01T18:14:26Z

tests/testthat/helper-custom-expectations.R

 expect_not_true <- function(...) expect_false(isTRUE(...))
+
+transform_print_snapshot <- function(x) {
+  vapply(x, function(line) {


Is there a reason to put them in this PR as opposed to the snapshot PR where they'll actually be used? I guess I can look at them as part of this PR, but I would kind of prefer to review them as part of the PR where they are used, so I can see them in use when reviewing them.

VisruthSK added 2 commits March 24, 2026 10:04

Moved to 3e and removed context calls

1e10511

LLMd testthat 3e syntax changes

5115c34

VisruthSK and others added 2 commits March 24, 2026 12:52

Tweak some minor testing things

a91cff8

Fixed tests

c98740e

VisruthSK and others added 22 commits March 24, 2026 14:40

Refresh test binaries

e1399ba

Use withr for tests

ce5f289

Fix LF issue in tests

2970907

Using more withr and testthat 3e features; setting up parallelization…

a46a125

… potentially

Stabilize parallel test runs

e5e7de5

Avoid pak local install in CI

c66a807

Serialize test model compilation

e2d2e5f

Run stateful tests sequentially

8d926bb

Stabilize OpenCL test

6797833

Trim unstable OpenCL checks

86bfac6

Run threaded tests sequentially

29d75db

Simplify test harness

1366910

Use repo pak on macOS devel

773ca2b

Use devel pak on macOS devel

159335c

Install local package outside pak on macOS devel

dfaee86

No parallel tests

09c3fcd

Cleaning tests up; more snapshots

6a01a93

More snapshots

faf24a8

Small changes

08ef444

Bump testthat requirement

b7cb687

Removed brittle snapshots

e55042f

Transform windows snapshots to remove .exe

d684bc3

More snapshots

d0348c6

VisruthSK marked this pull request as ready for review March 26, 2026 16:14

VisruthSK requested a review from jgabry March 26, 2026 22:16

jgabry reviewed Mar 27, 2026

View reviewed changes

tests/testthat/helper-custom-expectations.R Outdated Show resolved Hide resolved

tests/testthat/test-fit-vb.R Outdated Show resolved Hide resolved

tests/testthat/test-install.R Outdated Show resolved Hide resolved

VisruthSK added 2 commits March 31, 2026 09:59

Revert function

8e83d74

Removed snapshots

9431bca

VisruthSK marked this pull request as draft March 31, 2026 19:30

VisruthSK added 4 commits March 31, 2026 12:40

Fix a test regression

5618ad8

expect_known_output was deprecated so swapping those to equivalent snaps

6673e38

Deleted obviated answers directory

4d5b9aa

Use more withr functions to not rely on on.exit()

f7868cc

VisruthSK marked this pull request as ready for review April 1, 2026 01:03

VisruthSK commented Apr 1, 2026

View reviewed changes

VisruthSK requested a review from jgabry April 1, 2026 14:52

jgabry reviewed Apr 1, 2026

View reviewed changes

Uh oh!

Conversation

VisruthSK commented Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jgabry commented Mar 24, 2026

Uh oh!

codecov-commenter commented Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

VisruthSK commented Mar 26, 2026

Uh oh!

VisruthSK commented Mar 26, 2026

Uh oh!

jgabry commented Mar 26, 2026

Uh oh!

VisruthSK commented Mar 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jgabry left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jgabry commented Mar 30, 2026

Uh oh!

VisruthSK commented Mar 30, 2026

Uh oh!

jgabry commented Mar 30, 2026

Uh oh!

VisruthSK commented Mar 31, 2026

Uh oh!

VisruthSK commented Apr 1, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VisruthSK commented Apr 1, 2026

Uh oh!

jgabry left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

VisruthSK commented Mar 24, 2026 •

edited

Loading

codecov-commenter commented Mar 24, 2026 •

edited

Loading

VisruthSK commented Mar 26, 2026 •

edited

Loading