Review use of expect_snapshot() and friends
#2532
Conversation
* Don't use it just to suppress output * Convert `expect_snapshot_error()` to `expect_snapshot()` Fixes #2528
| @@ -45,7 +45,7 @@ test_that("articles don't include header-attrs.js script", { | |||
| pkg <- as_pkgdown(test_path("assets/articles")) | |||
| withr::defer(clean_site(pkg, quiet = TRUE)) | |||
|
|
|||
| expect_snapshot(path <- build_article("standard", pkg)) | |||
| suppressMessages(path <- build_article("standard", pkg)) | |||
There was a problem hiding this comment.
I wish there could be an option set with withr, so that the code wouldn't be nested inside supressMessages().
There was a problem hiding this comment.
We might get there one day, but I think that's still quite a lot of refactoring away.
| @@ -66,39 +66,46 @@ test_that("default reference includes all functions", { | |||
| }) | |||
|
|
|||
| test_that("errors well when a content entry is empty", { | |||
| meta <- yaml::yaml.load( "reference:\n- title: bla\n contents:\n - aname\n - ") | |||
| pkg <- as_pkgdown(test_path("assets/reference"), override = meta) | |||
| meta <- yaml::yaml.load("reference:\n- title: bla\n contents:\n - aname\n - ") | |||
There was a problem hiding this comment.
no need to load, since local_pkgdown_site() can use meta as either character or list.
In another test file I saw
pkg <- local_pkgdown_site(test_path("assets/articles"), "
template:
bootstrap: 5
")which is more readable than the \n, although it might be awkward with the indentation.
There was a problem hiding this comment.
if not using the same "style" as above, I think I'd find seeing the list directly more readable than the \n.
There was a problem hiding this comment.
Ah good catch; I forgot to update this older style.
* Don't use it just to suppress output * Convert `expect_snapshot_error()` to `expect_snapshot()` Fixes r-lib#2528
expect_snapshot_error()toexpect_snapshot()Fixes #2528