Include test path in snaphot hints

[hadley]

If you're not testing the current directory/package.

Fixes #1577

[hadley]

@schloerke could you please take a look and double check that this meets your needs?

[hadley]

Need same logic in snapshot_review_hint()

[schloerke]

And the base reporter hints as well, please:

testthat/R/reporter-check.R

Lines 147 to 148 in 71a33cd

	"* Run `testthat::snapshot_accept()` to accept all changes.",
	"* Run `testthat::snapshot_review()` to review all changes."

. Thank you!

[hadley]

@schloerke I don't think it makes sense to change that line — it is called when you're running R CMD check so the path is never going to be useful.

[schloerke]

it is called when you're running R CMD check so the path is never going to be useful.

Correct. Sorry for the false lead!

[hadley]

@schloerke if the updated code doesn't help, can you please give me an example of how you're running the tests, and then I can iterate.

[schloerke]

With the latest...

Two odd things:

• The file suggestion is double encoding the variant
• The test path is a full path instead of a relative path. (Happy to have it working, but this comment suggests it should be relative)

PR:

* Run `testthat::snapshot_review("mytest/mac-4.5/mytest", "/Users/barret/Documents/git/rstudio/shinycoreci/inst/apps/001-hello/tests/testthat")` to review the change.

Expected:

* Run `testthat::snapshot_review("mac-4.5/mytest", "./inst/apps/001-hello/tests/testthat")` to review the change.

[schloerke]

@schloerke if the updated code doesn't help, can you please give me an example of how you're running the tests, and then I can iterate.

• Checkout rstudio/shinycoreci
• Install dev dependencies w/ {pak}
• Within R, run tests with shinytest2::test_app("inst/apps/001-hello")

[hadley]

Where do I get shinyjster from?

[hadley]

@schloerke I figured it out and made a bunch of fixes. Let me know how this version goes 😄

[schloerke]

Outputs are tied directly to the test now. Thank you.

---

CRAN - shinytest2::test_app("inst/apps/001-hello/")

Failure (test-mytest.R:12:3): Migrated shinytest test: mytest.R
Snapshot of `file` to 'mytest/002.png' has changed
Run `testthat::snapshot_review('mytest/')` to review changes
Backtrace:
    ▆
 1. └─app$expect_screenshot() at test-mytest.R:12:3
 2.   └─shinytest2:::app_expect_screenshot_and_variant(...)
 3.     └─shinytest2:::app_expect_screenshot(self, private, ...)
 4.       └─shinytest2:::app__expect_snapshot_file(...)
 5.         ├─base::withCallingHandlers(...)
 6.         └─testthat::expect_snapshot_file(...)

Failure (test-mytest.R:15:3): Migrated shinytest test: mytest.R
Snapshot of `file` to 'mytest/004.png' has changed
Run `testthat::snapshot_review('mytest/')` to review changes
Backtrace:
    ▆
 1. └─app$expect_screenshot() at test-mytest.R:15:3
 2.   └─shinytest2:::app_expect_screenshot_and_variant(...)
 3.     └─shinytest2:::app_expect_screenshot(self, private, ...)
 4.       └─shinytest2:::app__expect_snapshot_file(...)
 5.         ├─base::withCallingHandlers(...)
 6.         └─testthat::expect_snapshot_file(...)

Failure (test-mytest.R:19:3): Migrated shinytest test: mytest.R
Snapshot of `file` to 'mytest/006.png' has changed
Run `testthat::snapshot_review('mytest/')` to review changes
Backtrace:
    ▆
 1. └─app$expect_screenshot() at test-mytest.R:19:3
 2.   └─shinytest2:::app_expect_screenshot_and_variant(...)
 3.     └─shinytest2:::app_expect_screenshot(self, private, ...)
 4.       └─shinytest2:::app__expect_snapshot_file(...)
 5.         ├─base::withCallingHandlers(...)
 6.         └─testthat::expect_snapshot_file(...)

---

PR - shinytest2::test_app("inst/apps/001-hello/")

Failure (test-mytest.R:12:3): Migrated shinytest test: mytest.R
Snapshot of `file` has changed.
* Run `testthat::snapshot_review("mytest/002.png", "inst/apps/001-hello/tests/testthat")` to review the change.
Backtrace:
    ▆
 1. └─app$expect_screenshot() at test-mytest.R:12:3
 2.   └─shinytest2:::app_expect_screenshot_and_variant(...)
 3.     └─shinytest2:::app_expect_screenshot(self, private, ...)
 4.       └─shinytest2:::app__expect_snapshot_file(...)
 5.         ├─base::withCallingHandlers(...)
 6.         └─testthat::expect_snapshot_file(...)

Failure (test-mytest.R:15:3): Migrated shinytest test: mytest.R
Snapshot of `file` has changed.
* Run `testthat::snapshot_review("mytest/004.png", "inst/apps/001-hello/tests/testthat")` to review the change.
Backtrace:
    ▆
 1. └─app$expect_screenshot() at test-mytest.R:15:3
 2.   └─shinytest2:::app_expect_screenshot_and_variant(...)
 3.     └─shinytest2:::app_expect_screenshot(self, private, ...)
 4.       └─shinytest2:::app__expect_snapshot_file(...)
 5.         ├─base::withCallingHandlers(...)
 6.         └─testthat::expect_snapshot_file(...)

Failure (test-mytest.R:19:3): Migrated shinytest test: mytest.R
Snapshot of `file` has changed.
* Run `testthat::snapshot_review("mytest/006.png", "inst/apps/001-hello/tests/testthat")` to review the change.
Backtrace:
    ▆
 1. └─app$expect_screenshot() at test-mytest.R:19:3
 2.   └─shinytest2:::app_expect_screenshot_and_variant(...)
 3.     └─shinytest2:::app_expect_screenshot(self, private, ...)
 4.       └─shinytest2:::app__expect_snapshot_file(...)
 5.         ├─base::withCallingHandlers(...)
 6.         └─testthat::expect_snapshot_file(...)

---

PR -withr::with_dir("inst/apps/001-hello/", {shinytest2::test_app()})

Failure (test-mytest.R:12:3): Migrated shinytest test: mytest.R
Snapshot of `file` has changed.
* Run `testthat::snapshot_review("mytest/002.png")` to review the change.
Backtrace:
    ▆
 1. └─app$expect_screenshot() at test-mytest.R:12:3
 2.   └─shinytest2:::app_expect_screenshot_and_variant(...)
 3.     └─shinytest2:::app_expect_screenshot(self, private, ...)
 4.       └─shinytest2:::app__expect_snapshot_file(...)
 5.         ├─base::withCallingHandlers(...)
 6.         └─testthat::expect_snapshot_file(...)

Failure (test-mytest.R:15:3): Migrated shinytest test: mytest.R
Snapshot of `file` has changed.
* Run `testthat::snapshot_review("mytest/004.png")` to review the change.
Backtrace:
    ▆
 1. └─app$expect_screenshot() at test-mytest.R:15:3
 2.   └─shinytest2:::app_expect_screenshot_and_variant(...)
 3.     └─shinytest2:::app_expect_screenshot(self, private, ...)
 4.       └─shinytest2:::app__expect_snapshot_file(...)
 5.         ├─base::withCallingHandlers(...)
 6.         └─testthat::expect_snapshot_file(...)

Failure (test-mytest.R:19:3): Migrated shinytest test: mytest.R
Snapshot of `file` has changed.
* Run `testthat::snapshot_review("mytest/006.png")` to review the change.
Backtrace:
    ▆
 1. └─app$expect_screenshot() at test-mytest.R:19:3
 2.   └─shinytest2:::app_expect_screenshot_and_variant(...)
 3.     └─shinytest2:::app_expect_screenshot(self, private, ...)
 4.       └─shinytest2:::app__expect_snapshot_file(...)
 5.         ├─base::withCallingHandlers(...)
 6.         └─testthat::expect_snapshot_file(...)

[schloerke]

Q: The hint has changed from "Review the test file snapshots" to "Review this specific snapshot only", which causes work (delete the file name or many copy/paste's) to review more than one snapshot in a single test file. While the new code is accurate, it makes reviewing more time consuming.

I'd advocate for still returning the variant name for review.

Expected:

• testthat::snapshot_review("mytest/", "inst/apps/001-hello/tests/testthat")
  Not:
• testthat::snapshot_review("mytest/002.png", "inst/apps/001-hello/tests/testthat")

Suggested change

	id <- c(file, name)
	id <- file

---

Or would it be possible to have the final comments suggest to review the test file's snaps.

[hadley]

Ooooooh, I did wonder about that, but I thought that was the existing behaviour too. I removed the variant because it didn't work, but I think that's because it doesn't work when both file and variant are supplied.

(There's something about the code paths here that I find extremely confusing.)