From 1c67dfa698e497a9153040f7752cff902b4f04c6 Mon Sep 17 00:00:00 2001 From: christophe dervieux Date: Fri, 24 Apr 2026 10:43:50 +0200 Subject: [PATCH 1/2] Fix snapshot_accept/review filtering with variant snapshots --- NEWS.md | 3 +++ R/snapshot-manage.R | 8 +++++++- tests/testthat/test-snapshot-manage.R | 29 ++++++++++++++++++++++++++- 3 files changed, 38 insertions(+), 2 deletions(-) diff --git a/NEWS.md b/NEWS.md index fca47541c..35859f5fc 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,8 @@ # testthat (development version) +* `snapshot_accept()`, `snapshot_reject()`, and `snapshot_review()` now correctly + find changed snapshots in variant subdirectories when filtering by test file + name (#2325). * `run_cpp_tests()` no longer accidentally reports that a test has been skipped (#2315). * `expect_setequal()` uses better wording in the results (@mcol, #2310). diff --git a/R/snapshot-manage.R b/R/snapshot-manage.R index 0c40b9741..0a6be46d9 100644 --- a/R/snapshot-manage.R +++ b/R/snapshot-manage.R @@ -207,7 +207,13 @@ snapshot_meta <- function(files = NULL, path = "tests/testthat") { # Match regardless of whether user include .md or not files <- c(files, paste0(files, ".md")) - out <- out[out$name %in% files | out$test %in% dirs, , drop = FALSE] + # Also match basename to handle variant snapshots (e.g. "variant/html.md") + out <- out[ + out$name %in% files | + basename(out$name) %in% files | + out$test %in% dirs, + , drop = FALSE + ] } out diff --git a/tests/testthat/test-snapshot-manage.R b/tests/testthat/test-snapshot-manage.R index 56df6d378..fcbaf91e6 100644 --- a/tests/testthat/test-snapshot-manage.R +++ b/tests/testthat/test-snapshot-manage.R @@ -79,10 +79,37 @@ test_that("can work with variants", { expect_snapshot(snapshot_accept(path = path)) expect_equal(dir(file.path(path, "_snaps", "foo")), "a.md") - # Can accept specified + # Can accept specified with fully qualified variant/name path <- local_snapshot_dir(c("foo/a.md", "foo/a.new.md")) expect_snapshot(snapshot_accept("foo/a", path = path)) expect_equal(dir(file.path(path, "_snaps", "foo")), "a.md") + + # Can accept by test file name alone (#2325) + path <- local_snapshot_dir(c("foo/a.md", "foo/a.new.md")) + suppressMessages(snapshot_accept("a", path = path)) + expect_equal(dir(file.path(path, "_snaps", "foo")), "a.md") + + # Can accept by test file name when multiple variants exist (#2325) + path <- local_snapshot_dir(c( + "variant1/a.md", "variant1/a.new.md", + "variant2/a.md", "variant2/a.new.md" + )) + suppressMessages(snapshot_accept("a", path = path)) + expect_equal( + dir(file.path(path, "_snaps"), recursive = TRUE), + c("variant1/a.md", "variant2/a.md") + ) + + # Filtering by test name doesn't affect other test files (#2325) + path <- local_snapshot_dir(c( + "foo/a.md", "foo/a.new.md", + "foo/b.md", "foo/b.new.md" + )) + suppressMessages(snapshot_accept("a", path = path)) + expect_equal( + dir(file.path(path, "_snaps"), recursive = TRUE), + c("foo/a.md", "foo/b.md", "foo/b.new.md") + ) }) test_that("snapshot_reject deletes .new files", { From 6187cbb7c2f41fc12b27cc4e644a36da48543a65 Mon Sep 17 00:00:00 2001 From: christophe dervieux Date: Fri, 24 Apr 2026 10:54:53 +0200 Subject: [PATCH 2/2] Adapt to guidelines and run air --- R/snapshot-manage.R | 7 +++---- tests/testthat/test-snapshot-manage.R | 20 ++++++++++++-------- 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/R/snapshot-manage.R b/R/snapshot-manage.R index 0a6be46d9..f27f437eb 100644 --- a/R/snapshot-manage.R +++ b/R/snapshot-manage.R @@ -209,10 +209,9 @@ snapshot_meta <- function(files = NULL, path = "tests/testthat") { # Also match basename to handle variant snapshots (e.g. "variant/html.md") out <- out[ - out$name %in% files | - basename(out$name) %in% files | - out$test %in% dirs, - , drop = FALSE + out$name %in% files | basename(out$name) %in% files | out$test %in% dirs, + , + drop = FALSE ] } diff --git a/tests/testthat/test-snapshot-manage.R b/tests/testthat/test-snapshot-manage.R index fcbaf91e6..c66543460 100644 --- a/tests/testthat/test-snapshot-manage.R +++ b/tests/testthat/test-snapshot-manage.R @@ -79,20 +79,22 @@ test_that("can work with variants", { expect_snapshot(snapshot_accept(path = path)) expect_equal(dir(file.path(path, "_snaps", "foo")), "a.md") - # Can accept specified with fully qualified variant/name + # Can accept specified path <- local_snapshot_dir(c("foo/a.md", "foo/a.new.md")) expect_snapshot(snapshot_accept("foo/a", path = path)) expect_equal(dir(file.path(path, "_snaps", "foo")), "a.md") - # Can accept by test file name alone (#2325) + # Can accept by test name alone (#2325) path <- local_snapshot_dir(c("foo/a.md", "foo/a.new.md")) suppressMessages(snapshot_accept("a", path = path)) expect_equal(dir(file.path(path, "_snaps", "foo")), "a.md") - # Can accept by test file name when multiple variants exist (#2325) + # Can accept across multiple variants (#2325) path <- local_snapshot_dir(c( - "variant1/a.md", "variant1/a.new.md", - "variant2/a.md", "variant2/a.new.md" + "variant1/a.md", + "variant1/a.new.md", + "variant2/a.md", + "variant2/a.new.md" )) suppressMessages(snapshot_accept("a", path = path)) expect_equal( @@ -100,10 +102,12 @@ test_that("can work with variants", { c("variant1/a.md", "variant2/a.md") ) - # Filtering by test name doesn't affect other test files (#2325) + # Only accepts matching test name (#2325) path <- local_snapshot_dir(c( - "foo/a.md", "foo/a.new.md", - "foo/b.md", "foo/b.new.md" + "foo/a.md", + "foo/a.new.md", + "foo/b.md", + "foo/b.new.md" )) suppressMessages(snapshot_accept("a", path = path)) expect_equal(