Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

snapshot_review() is not finding any files to review #1545

Closed
schloerke opened this issue Jan 7, 2022 · 9 comments · Fixed by #1546
Closed

snapshot_review() is not finding any files to review #1545

schloerke opened this issue Jan 7, 2022 · 9 comments · Fixed by #1546
Labels
bug an unexpected problem or unintended behavior snapshot 📷
Milestone

Comments

@schloerke
Copy link
Contributor

First half is with a variant. Second half is with no variant.
Both are failing to find any snaps to review.

Reprex:

library(testthat)
dir <- withr::local_tempdir()
#> Setting deferred event(s) on global environment.
#>   * Will be run automatically when session ends
#>   * Execute (and clear) with `withr::deferred_run()`.
#>   * Clear (without executing) with `withr::deferred_clear()`.
snapper <- testthat:::local_snapshotter(dir)

snapper$start_file("test", "test")
test_that("test", {
  local_edition(3)
  expect_snapshot_value("a", variant = "barret")
})
#> ── Warning (<text>:8:3): test ──────────────────────────────────────────────────
#> Adding new snapshot to variant 'barret':
#> "a"
snapper$end_file()
dir(dir, recursive = TRUE)
#> [1] "barret/test.md"

snapper$start_file("test", "test")
test_that("test", {
  local_edition(3)
  expect_snapshot_value("b", variant = "barret")
})
#> ── Failure (<text>:16:3): test ─────────────────────────────────────────────────
#> Snapshot of "b" has changed:
#> `old`: "a"
#> `new`: "b"
#> 
#> * Run `snapshot_accept('barret/test')` to accept the change
#> * Run `snapshot_review('barret/test')` to interactively review the change
#> Error:
#> ! Test failed
snapper$end_file()
dir(dir, recursive = TRUE)
#> [1] "barret/test.md"     "barret/test.new.md"


## Should review the "barret" snaps, but currently does not
snapshot_review("barret/test")
#> No snapshots to update
snapshot_review("barret/test", path = dir)
#> No snapshots to update





snapper$start_file("test", "test")
test_that("test", {
  local_edition(3)
  expect_snapshot_value("a", variant = NULL)
})
#> ── Warning (<text>:33:3): test ─────────────────────────────────────────────────
#> Adding new snapshot:
#> "a"
snapper$end_file()
dir(dir, recursive = TRUE)
#> [1] "barret/test.md"     "barret/test.new.md" "test.md"

snapper$start_file("test", "test")
test_that("test", {
  local_edition(3)
  expect_snapshot_value("b", variant = NULL)
})
#> ── Failure (<text>:41:3): test ─────────────────────────────────────────────────
#> Snapshot of "b" has changed:
#> `old`: "a"
#> `new`: "b"
#> 
#> * Run `snapshot_accept('test')` to accept the change
#> * Run `snapshot_review('test')` to interactively review the change
#> Error:
#> ! Test failed
snapper$end_file()
dir(dir, recursive = TRUE)
#> [1] "barret/test.md"     "barret/test.new.md" "test.md"           
#> [4] "test.new.md"

## Should also review the non-variant "test" snaps, but currently does not
snapshot_review("test")
#> No snapshots to update
snapshot_review("test", path = dir)
#> No snapshots to update

Created on 2022-01-07 by the reprex package (v2.0.0)

@hadley
Copy link
Member

hadley commented Jan 8, 2022

Reprex closer to what I'll test

library(testthat)
local_edition(3)

# variant -----------------------------------------------------------------
snapper <- testthat:::local_snapshotter()

snapper$start_file("test", "test")
expect_warning(expect_snapshot_value("a", variant = "barret"))
snapper$end_file()
dir(snapper$snap_dir, recursive = TRUE)

snapper$start_file("test", "test")
expect_failure(expect_snapshot_value("b", variant = "barret"))
snapper$end_file()
dir(snapper$snap_dir, recursive = TRUE)

## Should list the "barret" snaps, but currently does not
snapshot_meta(path = dir)

# no variant -----------------------------------------------------------------
snapper <- testthat:::local_snapshotter()

snapper$start_file("test", "test")
expect_warning(expect_snapshot_value("a", variant = NULL))
snapper$end_file()
dir(snapper$snap_dir, recursive = TRUE)

snapper$start_file("test", "test")
expect_failure(expect_snapshot_value("b", variant = NULL))
snapper$end_file()
dir(snapper$snap_dir, recursive = TRUE)

snapshot_meta(path = dir)

@hadley
Copy link
Member

hadley commented Jan 8, 2022

Hmmm at least part of the problem is that snapshot_meta() always adds _snaps to the path, so this reprex isn't reflecting what you should see interactively. I'll fix that first.

@hadley
Copy link
Member

hadley commented Jan 8, 2022

Even more minimal reprex:

library(testthat)

path <- testthat:::local_snapshot_dir(c("v/test", "v/test.new"))
snapshot_accept(path = path)
#> Updating snapshots:
#> * v/test.md

path <- testthat:::local_snapshot_dir(c("test", "test.new"))
snapshot_accept(path = path)
#> Updating snapshots:
#> * test.md

Created on 2022-01-07 by the reprex package (v2.0.1)

@hadley
Copy link
Member

hadley commented Jan 8, 2022

Are you seeing this in real life?

@schloerke
Copy link
Contributor Author

Are you seeing this in real life?

Yes.

After updating to the latest from main this afternoon, I could not review any snapshots within shinytest2 development (with or without variants).

@hadley
Copy link
Member

hadley commented Jan 8, 2022

With expect_snapshot() or expect_snapshot_file()?

@hadley
Copy link
Member

hadley commented Jan 8, 2022

Oooh, does snapshot_review() work? Is it just snapshot_review("specific-file") that doesn't?

@hadley
Copy link
Member

hadley commented Jan 8, 2022

The root problem is that snapshot_accept("foo") might mean accept _snaps/foo.new.md or _snaps/foo/*.new*

@schloerke
Copy link
Contributor Author

schloerke commented Jan 8, 2022

From my terminal... (which is not a reprex, 😜 )

❯❯ dev_load(); shiny::runTests("tests/testthat/apps/x")
ℹ Loading shinytest2
✔ | F W S  OK | Context
✔ |         8 | app-click [3.6s]
✔ |         1 | app-init-args [3.3s]
✔ |         2 | app-variant [4.0s]
✖ | 3       6 | app [7.7s]
─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
Failure (test-app.R:19:3): basic website example works using shinytest
Snapshot of `file` to 'app/st2-001.png' has changed
Run `testthat::snapshot_review('app')` to review changes
Backtrace:
 1. app$expect_screenshot()
      at test-app.R:19:2
 2. shinytest2::app_expect_screenshot_and_variant(...)
      at shinytest2.nosync/R/app-driver.R:281:6
 3. shinytest2::app_expect_screenshot(self, private, ...)
      at shinytest2.nosync/R/app-driver-expect-screenshot.R:105:2
 4. shinytest2::app__expect_snapshot_file(...)
      at shinytest2.nosync/R/app-driver-expect-screenshot.R:85:2
 5. testthat::expect_snapshot_file(...)
      at shinytest2.nosync/R/expect-snapshot.R:33:2

Failure (test-app.R:24:3): basic website example works using shinytest
Snapshot of `x` has changed (variant 'mac-4.0'):
`old`: "Hello Hadley!"
`new`: "<div id=\"greeting\" class=\"shiny-text-output shiny-bound-output\" aria-live=\"polite\">Hello Hadley!</div>"

* Run `snapshot_accept('mac-4.0/app')` to accept the change
* Run `snapshot_review('mac-4.0/app')` to interactively review the change
Backtrace:
 1. app$expect_html("#greeting", outer_html = TRUE)
      at test-app.R:24:2
 2. shinytest2::app_expect_html(...)
      at shinytest2.nosync/R/app-driver.R:158:6
 3. self$expect_script(...)
      at shinytest2.nosync/R/app-driver-expect-script.R:128:2
 4. shinytest2::app_expect_script(...)
      at shinytest2.nosync/R/app-driver.R:179:6
 5. shinytest2::app__expect_snapshot_value(...)
      at shinytest2.nosync/R/app-driver-expect-script.R:47:2
 6. testthat::expect_snapshot_value(...)
      at shinytest2.nosync/R/expect-snapshot.R:8:2

Failure (test-app.R:39:3): basic website example works using testthat
Snapshot of `tmpfile` to 'app/manual-screenshot.png' has changed
Run `testthat::snapshot_review('app')` to review changes
─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────

══ Results ══════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════════
Duration: 18.7 s

[ FAIL 3 | WARN 0 | SKIP 0 | PASS 17 ]
Error in testthat.R
Error: Test failures

Error: Shiny App Test Failures detected in
* x/tests/testthat.R

✖ shinytest2@no_wait_* 320.4 MiB (8s)
❯❯ testthat::snapshot_review('app', "tests/testthat/apps/x/tests/testthat")
No snapshots to update

✔ shinytest2@no_wait_* 275.2 MiB (1s)
❯❯ withr::with_dir("tests/testthat/apps/x", testthat::snapshot_review("app"))
No snapshots to update

✔ shinytest2@no_wait_* 320.4 MiB
❯❯ dir("tests/testthat/apps/x/tests/testthat/_snaps", recursive = TRUE)
 [1] "app-click/x-001_.png"             "app-click/x-001.json"             "app-click/x-002_.png"
 [4] "app-click/x-002.json"             "app-init-args/init-001.json"      "app-variant/variant-001_.new.png"
 [7] "app-variant/variant-001_.png"     "app-variant/variant-001.json"     "app/manual-screenshot.new.png"
[10] "app/manual-screenshot.png"        "mac-4.0/app.md"                   "mac-4.0/app.new.md"
[13] "mac-4.0/app/st2-001.new.png"      "mac-4.0/app/st2-001.png"          "mac-4.0/app/st2-002_.new.png"
[16] "mac-4.0/app/st2-002_.png"         "mac-4.0/app/st2-002.json"         "mac-4.0/app/st2-003_.png"
[19] "mac-4.0/app/st2-003.json"

This shows that using snapshot_review(files=) does not work.


Does snapshot_review() work?

Yes! Skipping the files argument lets the code work.

❯❯ testthat::snapshot_review(path = "tests/testthat/apps/x/tests/testthat")
Starting Shiny app for snapshot review
ℹ Use Ctrl + C to quit
shiny devmode - Using full shiny javascript file. To use the minified version, call `options(shiny.minified = TRUE)`
This message is displayed once every 8 hours.
shiny devmode - Turning on shiny autoreload. To disable, call `options(shiny.autoreload = FALSE)`
This message is displayed once every 8 hours.
^C

❯❯ withr::with_dir("tests/testthat/apps/x", testthat::snapshot_review())
Starting Shiny app for snapshot review
ℹ Use Ctrl + C to quit
^C

hadley added a commit that referenced this issue Jan 8, 2022
@hadley hadley added bug an unexpected problem or unintended behavior snapshot 📷 labels Jan 8, 2022
@hadley hadley added this to the v3.1.2 milestone Jan 8, 2022
hadley added a commit that referenced this issue Jan 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug an unexpected problem or unintended behavior snapshot 📷
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants