From 9dd397a0a9df8a6f77b799ce360e43f824ba187d Mon Sep 17 00:00:00 2001 From: "Zhian N. Kamvar" Date: Wed, 25 Jan 2023 09:58:59 -0800 Subject: [PATCH 01/16] always provision learners/setup.md This ensures that the setup section is always provisioned even if it does not appear in the config list. This also adds a test which explicitly sets up the learner config to a single markdown file and ensures that the setup.md is still built. This will fix https://github.com/carpentries/sandpaper/issues/386 --- R/utils-paths-source.R | 14 ++++++++ tests/testthat/test-build_markdown.R | 52 ++++++++++++++++------------ 2 files changed, 44 insertions(+), 22 deletions(-) diff --git a/R/utils-paths-source.R b/R/utils-paths-source.R index 3a7d22830..1a7858ea3 100644 --- a/R/utils-paths-source.R +++ b/R/utils-paths-source.R @@ -25,6 +25,17 @@ path_profiles <- function(inpath) { fs::path(home, "profiles") } + +provide_setup <- function(cfg, setup = "setup.md") { + if (is.null(cfg) || is.null(cfg$learners)) { + return(cfg) + } + if (get_slug(setup) %nin% get_slug(cfg$learners)) { + cfg$learners <- c(cfg$learners, setup) + } + return(cfg) +} + #' Get the full resource list of markdown files #' #' @param path path to the lesson @@ -114,6 +125,9 @@ get_resource_list <- function(path, trim = FALSE, subfolder = NULL, warn = FALSE if (use_subfolder) { names(res) <- subfolder } else { + lrn <- res[["learners"]] + stp <- fs::path_file(lrn[get_slug(lrn) %in% "setup"]) + cfg <- provide_setup(cfg, stp) subfolder <- c("episodes", "learners", "instructors", "profiles") } diff --git a/tests/testthat/test-build_markdown.R b/tests/testthat/test-build_markdown.R index cdf363869..d0cf56c49 100644 --- a/tests/testthat/test-build_markdown.R +++ b/tests/testthat/test-build_markdown.R @@ -11,15 +11,17 @@ writeLines(c( ), con = instruct ) +fs::file_copy(instruct, fs::path(tmp, "learners", "dimaryp.md")) set_globals(res) withr::defer(clear_globals()) } test_that("markdown sources can be built without fail", { - + suppressMessages(s <- get_episodes(tmp)) set_episodes(tmp, s, write = TRUE) + set_learners(tmp, "dimaryp.md", write = TRUE) expect_equal(res, tmp, ignore_attr = TRUE) # The episodes should be the only things in the directory e <- fs::dir_ls(fs::path(tmp, "episodes"), recurse = TRUE, type = "file") @@ -29,7 +31,7 @@ test_that("markdown sources can be built without fail", { skip_if_not(rmarkdown::pandoc_available("1.12.3")) # Accidentally rendered html live in their parent folders rmarkdown::render(instruct, quiet = TRUE) - expect_true(fs::file_exists(fs::path_ext_set(instruct, "html"))) + expect_true(fs::file_exists(fs::path_ext_set(instruct, "html"))) withr::local_options(list(sandpaper.handout = TRUE)) # It's noisy at first @@ -41,9 +43,13 @@ test_that("markdown sources can be built without fail", { # # Accidentaly rendered HTML is removed before building expect_false(fs::file_exists(fs::path_ext_set(instruct, "html"))) - expect_true(fs::file_exists(fs::path(res, "site", "built", "files", "code-handout.R"))) + build_path <- function(...) fs::path(res, "site", "built", ...) + expect_true(fs::file_exists(build_path("files", "code-handout.R"))) + expect_true(fs::file_exists(build_path("pyramid.md"))) + expect_true(fs::file_exists(build_path("dimaryp.md"))) + expect_true(fs::file_exists(build_path("setup.md"))) fs::file_delete(fs::path(res, "site", "built", "files", "code-handout.R")) - + }) test_that("changes in config.yaml triggers a rebuild of the site yaml", { @@ -51,7 +57,7 @@ test_that("changes in config.yaml triggers a rebuild of the site yaml", { skip_if_not(rmarkdown::pandoc_available("1.12.3")) yml <- get_path_site_yaml(res)$title expect_identical(yml, "Lesson Title") - cfg <- gsub("Lesson Title", "NEW: Lesson Title", + cfg <- gsub("Lesson Title", "NEW: Lesson Title", readLines(fs::path(res, "config.yaml"))) writeLines(cfg, fs::path(res, "config.yaml")) @@ -71,7 +77,7 @@ test_that("changes in config.yaml triggers a rebuild of the site yaml", { test_that("markdown sources can be rebuilt without fail", { - + # no building needed skip_on_os("windows") suppressMessages({ @@ -81,7 +87,7 @@ test_that("markdown sources can be rebuilt without fail", { }) }) expect_length(out, 0) - + withr::local_options(list(sandpaper.use_renv = FALSE)) # everything rebuilt expect_false(getOption("sandpaper.use_renv")) @@ -94,12 +100,12 @@ test_that("markdown sources can be rebuilt without fail", { }) test_that("modifying a file suffix will force the file to be rebuilt", { - + instruct <- fs::path(tmp, "instructors", "pyramid.md") instruct_rmd <- fs::path_ext_set(instruct, "Rmd") expect_true(fs::file_exists(instruct)) - # If we change a markdown file to an Rmarkdown file, + # If we change a markdown file to an Rmarkdown file, # that file should be rebuilt fs::file_move(instruct, instruct_rmd) expect_false(fs::file_exists(instruct)) @@ -131,14 +137,14 @@ test_that("Artifacts are accounted for", { # The artifacts are present in the built directory b <- c( # Generated markdown files - fs::path_ext_set(s, "md"), - "CODE_OF_CONDUCT.md", - "LICENSE.md", + fs::path_ext_set(s, "md"), + "CODE_OF_CONDUCT.md", + "LICENSE.md", "config.yaml", if (.Platform$OS.type != "windows") "renv.lock", "setup.md", # Folders - "data", + "data", "fig", "files", "index.md", @@ -146,15 +152,16 @@ test_that("Artifacts are accounted for", { "links.md", "learner-profiles.md", "md5sum.txt", - "pyramid.md" + "pyramid.md", + "dimaryp.md" ) a <- fs::dir_ls(fs::path(tmp, "site", "built")) expect_setequal(fs::path_file(a), b) b <- c( # Generated markdown files - fs::path_ext_set(s, "md"), - "CODE_OF_CONDUCT.md", - "LICENSE.md", + fs::path_ext_set(s, "md"), + "CODE_OF_CONDUCT.md", + "LICENSE.md", "config.yaml", if (.Platform$OS.type != "windows") "renv.lock", "setup.md", @@ -165,7 +172,8 @@ test_that("Artifacts are accounted for", { "links.md", "learner-profiles.md", "md5sum.txt", - "pyramid.md" + "pyramid.md", + "dimaryp.md" ) a <- fs::dir_ls(fs::path(tmp, "site", "built"), recurse = TRUE, type = "file") expect_setequal(fs::path_file(a), b) @@ -198,7 +206,7 @@ test_that("Output is not commented", { test_that("Markdown rendering does not happen if content is not changed", { skip_on_os("windows") - + suppressMessages({ expect_message(out <- capture.output(build_markdown(res)), "nothing to rebuild") }) @@ -277,7 +285,7 @@ test_that("dates are preserved in md5sum.txt", { test_that("Removing partially matching slugs will not have side-effects", { built_path <- path_built(res) - + fs::file_delete(fs::path(res, "instructors", "pyramid.md")) build_markdown(res, quiet = TRUE) h1 <- expect_hashed(res, "introduction.Rmd") @@ -289,7 +297,7 @@ test_that("Removing partially matching slugs will not have side-effects", { # The image should still exist pyramid_fig <- fs::path(built_path, "fig", "introduction-rendered-pyramid-1.png") expect_true(fs::file_exists(pyramid_fig)) - + }) test_that("setting `fail_on_error: true` in config will cause build to fail", { @@ -320,7 +328,7 @@ test_that("setting `fail_on_error: true` in config will cause build to fail", { # # 1. hammertime # 2. in the name of love - # + # # The first chunk is allowed to show the error in the document, the second # is not. When we check for the text of the second error, that confirms that # the first error is passed over From e49970379eeb96644adb04043d259261b71b9d7d Mon Sep 17 00:00:00 2001 From: "Zhian N. Kamvar" Date: Wed, 25 Jan 2023 10:01:41 -0800 Subject: [PATCH 02/16] update documentation for reserved_db() This makes it more clear that it's used by `build_site()` and not `build_markdown()` --- R/utils-built-db.R | 50 ++++++++++++++++++++++++++++++++++++---------- 1 file changed, 40 insertions(+), 10 deletions(-) diff --git a/R/utils-built-db.R b/R/utils-built-db.R index d36a0b9db..39a473339 100644 --- a/R/utils-built-db.R +++ b/R/utils-built-db.R @@ -15,11 +15,11 @@ get_hash <- function(path, db = fs::path(path_built(path), "md5sum.txt")) { #' Get the database of built files and their hashes #' #' @param db the path to the database. -#' @param filter regex describing files to include. +#' @param filter regex describing files to include. #' @return a data frame with three columns: #' - file: the path to the source file #' - checksum: the hash of the source file to generate the built file -#' - built: the path to the built file +#' - built: the path to the built file #' @keywords internal #' @seealso [build_status()], [get_hash()] get_built_db <- function(db = "site/built/md5sum.txt", filter = "*R?md") { @@ -34,14 +34,44 @@ get_built_db <- function(db = "site/built/md5sum.txt", filter = "*R?md") { return(files[are_markdown, , drop = FALSE]) } -#' Filter reserved files from the built db +#' Filter reserved markdown files from the built db +#' +#' This provides a service for `build_site()` so that it does not build files +#' that are used for aggregation, resource provision, or GitHub specific files +#' +#' @details +#' +#' There are three types of files that are reserved and we do not want to +#' propogate to the HTML site +#' +#' ## GitHub specific files +#' +#' These are the README and CONTRIBUTING files. Both of these files provide +#' information that is useful only in the context of GitHub +#' +#' ## Aggregation files +#' +#' These are files that are aggregated together with other files or have +#' content appended to them: +#' +#' - `index` and `learners/setup` are concatenated +#' - all markdown files in `profiles/` are concatenated +#' - `instructors/instructor-notes` have the inline instructor notes +#' concatenated. +#' +#' ## Resource provision files +#' +#' At the moment, there is one file that we use for resource provision and +#' should not be propogated to the site: `links`. This provides global links +#' for the lesson. It provides no content in and of itself. #' #' @param db the database from [get_built_db()] -#' @return a data frame, but a bit shorter +#' @return the same database with the above files filtered out #' @keywords internal -#' @seealso [get_built_db()] +#' @seealso [get_built_db()] that provides the database and [build_site()], +#' which uses the function reserved_db <- function(db) { - reserved <- c("index", "README", "CONTRIBUTING", "learners/setup", + reserved <- c("index", "README", "CONTRIBUTING", "learners/setup", "profiles[/].*", "instructors[/]instructor-notes[.]*", "links") reserved <- paste(reserved, collapse = "|") reserved <- paste0("^(", reserved, ")[.]R?md") @@ -53,7 +83,7 @@ write_build_db <- function(md5, db) write.table(md5, db, row.names = FALSE) #' Identify what files need to be rebuilt and what need to be removed #' #' This takes in a vector of files and compares them against a text database of -#' files with checksums. It's been heavily adapted from blogdown to provide +#' files with checksums. It's been heavily adapted from blogdown to provide #' utilities for removal and updating of the old database. #' #' @details @@ -72,10 +102,10 @@ write_build_db <- function(md5, db) write.table(md5, db, row.names = FALSE) #' @param rebuild if the files should be rebuilt, set this to TRUE (defaults to #' FALSE) #' @param write if TRUE, the database will be updated, Defaults to FALSE, -#' meaning that the database will remain the same. +#' meaning that the database will remain the same. #' @return a list of the following elements #' - *build* absolute paths of files to build -#' - *new* a new data frame with three columns: +#' - *new* a new data frame with three columns: #' - file the relative path to the source file #' - checksum the md5 sum of the source file #' - built the relative path to the built file @@ -166,7 +196,7 @@ build_status <- function(sources, db = "site/built/md5sum.txt", rebuild = FALSE, # also remove the files that no longer exist in the sources list. one <- one[match(sources, one$file), , drop = FALSE] # TODO: see if we can have rebuild be a vector matching the sources so that - # we can indicate a vector of files to rebuild. + # we can indicate a vector of files to rebuild. if (rebuild) { files = one[['file']] to_remove <- old[['built']] From 705a5ff597f1d38813e9c789a18f970288ecd208 Mon Sep 17 00:00:00 2001 From: "Zhian N. Kamvar" Date: Wed, 25 Jan 2023 10:02:18 -0800 Subject: [PATCH 03/16] trailing whitespace stripped by .editorconfig --- R/build_markdown.R | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/R/build_markdown.R b/R/build_markdown.R index 51b680bff..d96385fdd 100644 --- a/R/build_markdown.R +++ b/R/build_markdown.R @@ -3,17 +3,17 @@ #' In the spirit of {hugodown}, This function will build plain markdown files #' as a minimal R package in the `site/` folder of your {sandpaper} lesson #' repository tagged with the hash of your file to ensure that only files that -#' have changed are rebuilt. -#' +#' have changed are rebuilt. +#' #' @param path the path to your repository (defaults to your current working #' directory) #' @param rebuild if `TRUE`, everything will be built from scratch as if there #' was no cache. Defaults to `FALSE`, which will only build markdown files that -#' haven't been built before. -#' +#' haven't been built before. +#' #' @return `TRUE` if it was successful, a character vector of issues if it was #' unsuccessful. -#' +#' #' @keywords internal #' @seealso [build_episode_md()] build_markdown <- function(path = ".", rebuild = FALSE, quiet = FALSE, slug = NULL) { @@ -50,7 +50,7 @@ build_markdown <- function(path = ".", rebuild = FALSE, quiet = FALSE, slug = NU } # If the user accidentally used rmarkdown::render(), then they would end up - # with an html artifact in here and it will clog up the machinery. Best to + # with an html artifact in here and it will clog up the machinery. Best to # remove it at the source. remove_rendered_html(sources) @@ -66,7 +66,7 @@ build_markdown <- function(path = ".", rebuild = FALSE, quiet = FALSE, slug = NU # Copy the files to the assets directory ------------------------------------- artifacts <- get_artifacts(path, "episodes") to_copy <- vapply( - c("data", "files", "fig"), + c("data", "files", "fig"), FUN = function(i) enforce_dir(fs::path(episode_path, i)), FUN.VALUE = character(1) ) @@ -95,7 +95,7 @@ build_markdown <- function(path = ".", rebuild = FALSE, quiet = FALSE, slug = NU renv_check_consent(path, quiet, sources) # determine if we need to fail when errors are triggered fail_on_error <- this_metadata$get()[["fail_on_error"]] - # this is `error` in the knitr sense of `error = TRUE` means + # this is `error` in the knitr sense of `error = TRUE` means # fail_on_error = FALSE. error <- is.null(fail_on_error) || !fail_on_error # exclude files that do not need to be rebuilt From 777087eab751c519bf77249c96d2575c69763d43 Mon Sep 17 00:00:00 2001 From: "Zhian N. Kamvar" Date: Wed, 25 Jan 2023 10:02:45 -0800 Subject: [PATCH 04/16] add news --- NEWS.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/NEWS.md b/NEWS.md index 06d79ce28..454729564 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,12 @@ # sandpaper 0.11.4 +BUG FIX +------- + +* The setup page will always be provisioned for lessons regardless if it + exists in the `learners:` field in `config.yaml` + (reported: @zkamvar, #386; fixed: @zkamvar #387). + PANDOC ------ From c3e5a78ad2a643771d19245ac9bb2680085de893 Mon Sep 17 00:00:00 2001 From: "Zhian N. Kamvar" Date: Wed, 25 Jan 2023 10:11:35 -0800 Subject: [PATCH 05/16] refactor a test to make it easier to understand --- tests/testthat/test-build_markdown.R | 48 +++++++++++----------------- 1 file changed, 18 insertions(+), 30 deletions(-) diff --git a/tests/testthat/test-build_markdown.R b/tests/testthat/test-build_markdown.R index d0cf56c49..b10e36d32 100644 --- a/tests/testthat/test-build_markdown.R +++ b/tests/testthat/test-build_markdown.R @@ -131,52 +131,37 @@ test_that("modifying a file suffix will force the file to be rebuilt", { test_that("Artifacts are accounted for", { s <- get_episodes(tmp) - # No artifacts should be present in the directory - e <- fs::dir_ls(fs::path(tmp, "episodes"), recurse = TRUE, type = "file") - expect_equal(fs::path_file(e), s, ignore_attr = TRUE) # The artifacts are present in the built directory b <- c( - # Generated markdown files - fs::path_ext_set(s, "md"), "CODE_OF_CONDUCT.md", "LICENSE.md", - "config.yaml", - if (.Platform$OS.type != "windows") "renv.lock", "setup.md", - # Folders - "data", - "fig", - "files", "index.md", "instructor-notes.md", "links.md", "learner-profiles.md", - "md5sum.txt", "pyramid.md", - "dimaryp.md" - ) - a <- fs::dir_ls(fs::path(tmp, "site", "built")) - expect_setequal(fs::path_file(a), b) - b <- c( + "dimaryp.md", # Generated markdown files fs::path_ext_set(s, "md"), - "CODE_OF_CONDUCT.md", - "LICENSE.md", "config.yaml", if (.Platform$OS.type != "windows") "renv.lock", - "setup.md", - # Generated figures - paste0(fs::path_ext_remove(s), "-rendered-pyramid-1.png"), - "index.md", - "instructor-notes.md", - "links.md", - "learner-profiles.md", - "md5sum.txt", - "pyramid.md", - "dimaryp.md" + "md5sum.txt" ) + + # No artifacts should be present in the source dir -------------- + e <- fs::dir_ls(fs::path(tmp, "episodes"), recurse = TRUE, type = "file") + expect_equal(fs::path_file(e), s, ignore_attr = TRUE) + + # Testing for top-level artifacts ------------------------------- + folders <- c( "data", "fig", "files") + a <- fs::dir_ls(fs::path(tmp, "site", "built")) + expect_setequal(fs::path_file(a), c(folders, b)) + + # Testing for generated figures included ------------------------ + figs <- paste0(fs::path_ext_remove(s), "-rendered-pyramid-1.png") a <- fs::dir_ls(fs::path(tmp, "site", "built"), recurse = TRUE, type = "file") - expect_setequal(fs::path_file(a), b) + expect_setequal(fs::path_file(a), c(figs, b)) }) @@ -198,7 +183,10 @@ test_that("Output is not commented", { outid <- grep("[1]", ep, fixed = TRUE) output <- ep[outid[1]] fence <- ep[outid[1] - 1] + + # code output lines start with normal R indexing --------------- expect_match(output, "^\\[1\\]") + # code output fences have the output class --------------------- expect_match(fence, "^[`]{3}[{]?\\.?output[}]?") }) From 2ea36c01a18b50b862e2d4ac6f505bfa2c7a9e1b Mon Sep 17 00:00:00 2001 From: zkamvar Date: Wed, 25 Jan 2023 18:17:08 +0000 Subject: [PATCH 06/16] Document --- man/reserved_db.Rd | 38 ++++++++++++++++++++++++++++++++++---- 1 file changed, 34 insertions(+), 4 deletions(-) diff --git a/man/reserved_db.Rd b/man/reserved_db.Rd index ccd9a74b0..2f80e123e 100644 --- a/man/reserved_db.Rd +++ b/man/reserved_db.Rd @@ -2,7 +2,7 @@ % Please edit documentation in R/utils-built-db.R \name{reserved_db} \alias{reserved_db} -\title{Filter reserved files from the built db} +\title{Filter reserved markdown files from the built db} \usage{ reserved_db(db) } @@ -10,12 +10,42 @@ reserved_db(db) \item{db}{the database from \code{\link[=get_built_db]{get_built_db()}}} } \value{ -a data frame, but a bit shorter +the same database with the above files filtered out } \description{ -Filter reserved files from the built db +This provides a service for \code{build_site()} so that it does not build files +that are used for aggregation, resource provision, or GitHub specific files +} +\details{ +There are three types of files that are reserved and we do not want to +propogate to the HTML site +\subsection{GitHub specific files}{ + +These are the README and CONTRIBUTING files. Both of these files provide +information that is useful only in the context of GitHub +} + +\subsection{Aggregation files}{ + +These are files that are aggregated together with other files or have +content appended to them: +\itemize{ +\item \code{index} and \code{learners/setup} are concatenated +\item all markdown files in \verb{profiles/} are concatenated +\item \code{instructors/instructor-notes} have the inline instructor notes +concatenated. +} +} + +\subsection{Resource provision files}{ + +At the moment, there is one file that we use for resource provision and +should not be propogated to the site: \code{links}. This provides global links +for the lesson. It provides no content in and of itself. +} } \seealso{ -\code{\link[=get_built_db]{get_built_db()}} +\code{\link[=get_built_db]{get_built_db()}} that provides the database and \code{\link[=build_site]{build_site()}}, +which uses the function } \keyword{internal} From 8bff1434ec398656dc241793e192b7984b1fb5ad Mon Sep 17 00:00:00 2001 From: "Zhian N. Kamvar" Date: Wed, 25 Jan 2023 10:41:08 -0800 Subject: [PATCH 07/16] remove readme badge --- README.Rmd | 1 - README.md | 2 -- 2 files changed, 3 deletions(-) diff --git a/README.Rmd b/README.Rmd index 6311b0ca3..8f553629b 100644 --- a/README.Rmd +++ b/README.Rmd @@ -16,7 +16,6 @@ knitr::opts_chunk$set( [![R Universe](https://carpentries.r-universe.dev/badges/sandpaper)](https://carpentries.r-universe.dev/ui#builds) -[![R build status](https://github.com/carpentries/sandpaper/workflows/R-CMD-check/badge.svg)](https://github.com/carpentries/sandpaper/actions) [![Codecov test coverage](https://codecov.io/gh/carpentries/sandpaper/branch/main/graph/badge.svg)](https://codecov.io/gh/carpentries/sandpaper?branch=main) [![Lifecycle: experimental](https://img.shields.io/badge/lifecycle-experimental-orange.svg)](https://www.tidyverse.org/lifecycle/#experimental) [![CRAN status](https://www.r-pkg.org/badges/version/sandpaper)](https://CRAN.R-project.org/package=sandpaper) diff --git a/README.md b/README.md index cf3496c84..ea57c7b01 100644 --- a/README.md +++ b/README.md @@ -6,8 +6,6 @@ [![R Universe](https://carpentries.r-universe.dev/badges/sandpaper)](https://carpentries.r-universe.dev/ui#builds) -[![R build -status](https://github.com/carpentries/sandpaper/workflows/R-CMD-check/badge.svg)](https://github.com/carpentries/sandpaper/actions) [![Codecov test coverage](https://codecov.io/gh/carpentries/sandpaper/branch/main/graph/badge.svg)](https://codecov.io/gh/carpentries/sandpaper?branch=main) [![Lifecycle: From 82e82843d2500c4a585c38301e28ef1af2616d89 Mon Sep 17 00:00:00 2001 From: "Zhian N. Kamvar" Date: Wed, 25 Jan 2023 11:38:21 -0800 Subject: [PATCH 08/16] add some noise to tests --- R/test-fixtures.R | 5 +++-- tests/testthat/setup.R | 6 ++++-- tests/testthat/test-build_episode.R | 16 +++++++++------- 3 files changed, 16 insertions(+), 11 deletions(-) diff --git a/R/test-fixtures.R b/R/test-fixtures.R index 57d341804..6581be299 100644 --- a/R/test-fixtures.R +++ b/R/test-fixtures.R @@ -20,7 +20,8 @@ #' @keywords internal #' @rdname fixtures create_test_lesson <- function() { - if (interactive()) { + noise <- interactive() || Sys.getenv("CI") == "true" + if (noise) { cli::cli_status("{cli::symbol$arrow_right} Bootstrapping example lesson") } # We explicitly need the package cache for tests @@ -28,7 +29,7 @@ create_test_lesson <- function() { repodir <- fs::file_temp() fs::dir_create(repodir) repo <- fs::path(repodir, "lesson-example") - if (interactive()) { + if (noise) { cli::cli_status_update( "{cli::symbol$arrow_right} Bootstrapping example lesson in {repo}" ) diff --git a/tests/testthat/setup.R b/tests/testthat/setup.R index cdfdcf430..c68f9bdf7 100644 --- a/tests/testthat/setup.R +++ b/tests/testthat/setup.R @@ -6,7 +6,8 @@ rmt <- fs::file_temp(pattern = "REMOTE-") setup_local_remote(repo = tmp, remote = rmt, verbose = FALSE) - if (interactive()) { + noise <- interactive() || Sys.getenv("CI") == "true" + if (noise) { cli::cli_alert_info("Current RENV_PATHS_ROOT {Sys.getenv('RENV_PATHS_ROOT')}") cli::cli_alert_info("Current renv::paths$root() {renv::paths$root()}") cli::cli_alert_info( @@ -23,7 +24,8 @@ withr::defer({ rem <- remove_local_remote(repo = tf) # remove the test fixture and report res <- tryCatch(fs::dir_delete(tf), error = function() FALSE) - if (interactive()) { + noise <- interactive() || Sys.getenv("CI") == "true" + if (noise) { status <- if (identical(res, FALSE)) "could not be" else "successfully" cli::cli_alert_info("{.file {tf}} {status} removed") status <- if (is.character(rem)) "successfully" else "could not be" diff --git a/tests/testthat/test-build_episode.R b/tests/testthat/test-build_episode.R index bbd0df0e6..1608553a7 100644 --- a/tests/testthat/test-build_episode.R +++ b/tests/testthat/test-build_episode.R @@ -1,5 +1,7 @@ res <- tmp <- restore_fixture() +print(fs::dir_tree(res, recurse = 1)) + test_that("build_episode_html() returns nothing for an empty page", { @@ -16,7 +18,7 @@ test_that("build_episode functions works independently", { withr::local_options(list(sandpaper.use_renv = FALSE)) pkg <- pkgdown::as_pkgdown(file.path(tmp, "site")) expect_output(pkgdown::init_site(pkg)) - + skip_if_not(rmarkdown::pandoc_available("2.11")) # create a new file in extras @@ -35,16 +37,16 @@ test_that("build_episode functions works independently", { expect_equal(lines[[2]], "title: Fun times") from_r <- grep("This is coming from", lines) expect_match(lines[from_r], "This is coming from R (version|Under)") - + # Explicitly testing https://github.com/carpentries/sandpaper/issues/288 # If we specify a `new.env()`, then the S3 dispatch will not work, but when we - # default to `globalenv()`, the S3 dispatch works. + # default to `globalenv()`, the S3 dispatch works. expect_false(any(grepl("Error", lines))) expect_false(file.exists(file.path(tmp, "site", "docs", "fun.html"))) expect_false(file.exists(file.path(tmp, "site", "docs", "instructor", "fun.html"))) expect_output({ - build_episode_html(res, + build_episode_html(res, fun_file, page_back = fun_file, page_forward = fun_file, @@ -73,20 +75,20 @@ test_that("the chapter-links should be cromulent depending on the view", { expect_length(learn_links, 4L) expect_equal(xml2::xml_attr(learn_links, "href"), rep("fun.html", 4L)) - learn_note<- xml2::xml_find_all(learn, + learn_note<- xml2::xml_find_all(learn, ".//div[starts-with(@id, 'accordionInstructor')]") expect_length(learn_note, 0L) instruct_links <- xml2::xml_find_all(instruct, ".//a[@class='chapter-link']") expect_length(instruct_links, 4L) - expect_equal(xml2::xml_attr(instruct_links, "href"), + expect_equal(xml2::xml_attr(instruct_links, "href"), rep("../instructor/fun.html", 4L)) internal_instruct_links <- xml2::xml_find_all(instruct, ".//a[contains(text(), 'internal')]") expect_length(internal_instruct_links, 1L) expect_equal(xml2::xml_attr(internal_instruct_links, "href"), "fun.html") - instruct_note <- xml2::xml_find_all(instruct, + instruct_note <- xml2::xml_find_all(instruct, ".//div[starts-with(@id, 'accordionInstructor')]") expect_length(instruct_note, 1L) IN_lines <- trimws(xml2::xml_text(instruct_note)) From 484c7dee0f5ae6e4fe1d41024632b93d0212d855 Mon Sep 17 00:00:00 2001 From: "Zhian N. Kamvar" Date: Wed, 25 Jan 2023 12:24:46 -0800 Subject: [PATCH 09/16] update build_markdown test for updated knitr --- tests/testthat/test-build_markdown.R | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/testthat/test-build_markdown.R b/tests/testthat/test-build_markdown.R index b10e36d32..fa159fe80 100644 --- a/tests/testthat/test-build_markdown.R +++ b/tests/testthat/test-build_markdown.R @@ -38,7 +38,7 @@ test_that("markdown sources can be built without fail", { suppressMessages({ build_markdown(res, quiet = FALSE) %>% expect_message("Consent to use package cache (provided|not given)") %>% - expect_output("ordinary text without R code") + expect_output("pyramid") # chunk name from template episode }) # # Accidentaly rendered HTML is removed before building @@ -94,7 +94,7 @@ test_that("markdown sources can be rebuilt without fail", { suppressMessages({ build_markdown(res, quiet = FALSE, rebuild = TRUE) %>% expect_message("Consent to use package cache not given.") %>% - expect_output("ordinary text without R code") + expect_output("pyramid") # chunk name from template episode }) expect_false(getOption("sandpaper.use_renv")) }) From e7c378314fb318a5931f365f751e3b38e3f8ef93 Mon Sep 17 00:00:00 2001 From: "Zhian N. Kamvar" Date: Wed, 25 Jan 2023 12:28:39 -0800 Subject: [PATCH 10/16] update knitr tests for build_lesson --- tests/testthat/test-build_lesson.R | 46 +++++++++++++++--------------- 1 file changed, 23 insertions(+), 23 deletions(-) diff --git a/tests/testthat/test-build_lesson.R b/tests/testthat/test-build_lesson.R index ef5262fdd..4feb6532f 100644 --- a/tests/testthat/test-build_lesson.R +++ b/tests/testthat/test-build_lesson.R @@ -10,16 +10,16 @@ test_that("Lessons built for the first time are noisy", { # It's noisy at first suppressMessages({ - expect_output(build_lesson(tmp, preview = FALSE, quiet = FALSE), - "ordinary text without R code") + expect_output(build_lesson(tmp, preview = FALSE, quiet = FALSE), + "pyramid") # chunk name from example episode }) htmls <- read_all_html(sitepath) - expect_setequal(names(htmls$learner), - c("introduction", "index", "LICENSE", "CODE_OF_CONDUCT", "profiles", + expect_setequal(names(htmls$learner), + c("introduction", "index", "LICENSE", "CODE_OF_CONDUCT", "profiles", "instructor-notes", "key-points", "aio", "images") ) - expect_setequal(names(htmls$instructor), - c("introduction", "index", "LICENSE", "CODE_OF_CONDUCT", "profiles", + expect_setequal(names(htmls$instructor), + c("introduction", "index", "LICENSE", "CODE_OF_CONDUCT", "profiles", "instructor-notes", "key-points", "aio", "images") ) @@ -74,7 +74,7 @@ test_that("aio page can be rebuilt", { writeLines(as.character(html), aio) content <- get_content(aio, "section[@id='ephemeral']", pkg = pkg) expect_length(content, 1L) - + # rebuild the content and check if the section still exists... it shouldn't build_aio(pkg, pages = htmls, quiet = TRUE) content <- get_content(aio, "section[@id='ephemeral']", pkg = pkg) @@ -99,7 +99,7 @@ test_that("keypoints page can be rebuilt", { writeLines(as.character(html), keypoints) content <- get_content(keypoints, "section[@id='ephemeral']", pkg = pkg) expect_length(content, 1L) - + # rebuild the content and check if the section still exists... it shouldn't build_keypoints(pkg, pages = htmls, quiet = TRUE) content <- get_content(keypoints, "section[@id='ephemeral']", pkg = pkg) @@ -118,7 +118,7 @@ test_that("instructor-notes page can be rebuilt", { content <- get_content(html, "section[@id='aggregate-instructor-notes']/section") expect_length(content, 1L) expect_equal(xml2::xml_attr(content, "id"), "introduction") - expect_match(xml2::xml_text(xml2::xml_find_first(content[[1]], ".//p")), + expect_match(xml2::xml_text(xml2::xml_find_first(content[[1]], ".//p")), "Inline instructor notes") # add an ephemeral section and write it out @@ -126,7 +126,7 @@ test_that("instructor-notes page can be rebuilt", { writeLines(as.character(html), inotes) content <- get_content(inotes, "/section[@id='ephemeral']", pkg = pkg, instructor = TRUE) expect_length(content, 1L) - + # rebuild the content and check if the section still exists... it shouldn't build_instructor_notes(pkg, pages = htmls, quiet = TRUE) content <- get_content(inotes, "/section[@id='ephemeral']", pkg = pkg, instructor = TRUE) @@ -186,7 +186,7 @@ test_that("single files can be built", { suppressMessages({ rdr$render() %>% - expect_output("ordinary text without R code") %>% + expect_output("pyramid") %>% # chunk name from example episode expect_message("Output created: .*second-episode.html") }) @@ -215,7 +215,7 @@ test_that("single files can be re-built", { suppressMessages({ rdr$render() %>% - expect_output("ordinary text without R code") %>% + expect_output("pyramid") %>% # chunk name from example episode expect_message("Output created: .*second-episode.html") }) @@ -249,11 +249,11 @@ test_that("HTML files are present and have the correct elements", { # Div tags show up as expected expect_true(any(grepl(".div.+? class..callout challenge", ep))) - # figure captions show up from knitr - # (https://github.com/carpentries/sandpaper/issues/114) + # figure captions show up from knitr + # (https://github.com/carpentries/sandpaper/issues/114) expect_true(any(grepl("Sun arise each and every morning", ep))) expect_true(any(grepl( - ".div.+? class..callout challenge", + ".div.+? class..callout challenge", readLines(fs::path(sitepath, "second-episode.html")) ))) expect_true(any(grepl( @@ -267,8 +267,8 @@ test_that("files will not be rebuilt unless they change in content", { skip_if_not(rmarkdown::pandoc_available("2.11")) suppressMessages({ expect_failure({ - expect_output(build_lesson(tmp, preview = FALSE, quiet = FALSE), - "ordinary text without R code") + expect_output(build_lesson(tmp, preview = FALSE, quiet = FALSE), + "pyramid") # chunk name from example episode }) }) @@ -276,8 +276,8 @@ test_that("files will not be rebuilt unless they change in content", { suppressMessages({ expect_failure({ - expect_output(build_lesson(tmp, preview = FALSE, quiet = FALSE), - "ordinary text without R code") + expect_output(build_lesson(tmp, preview = FALSE, quiet = FALSE), + "pyramid") # chunk name from example episode }) }) @@ -332,10 +332,10 @@ test_that("aio page is updated with new pages", { expect_true(fs::file_exists(aio)) expect_true(fs::file_exists(iaio)) html <- xml2::read_html(aio) - content <- xml2::xml_find_all(html, + content <- xml2::xml_find_all(html, ".//div[contains(@class, 'lesson-content')]/section[starts-with(@id, 'aio-')]") expect_length(content, 2L) - expect_equal(xml2::xml_attr(content, "id"), + expect_equal(xml2::xml_attr(content, "id"), c("aio-introduction", "aio-second-episode")) }) @@ -362,7 +362,7 @@ test_that("episodes with HTML in the title are rendered correctly", { writeLines(se, fs::path(tmp, "episodes", "second-episode.Rmd")) suppressMessages({ - expect_output(build_lesson(tmp, preview = FALSE, quiet = FALSE), "ordinary text without R code") + expect_output(build_lesson(tmp, preview = FALSE, quiet = FALSE), "pyramid") }) h1 <- expect_hashed(tmp, "introduction.Rmd") @@ -371,7 +371,7 @@ test_that("episodes with HTML in the title are rendered correctly", { expect_true(fs::file_exists(fs::path(sitepath, "second-episode.html"))) expect_true(any(grepl( - "A bold title", + "A bold title", readLines(fs::path(sitepath, "second-episode.html")), fixed = TRUE ))) From c819662dd1b4305c9dc6a17b0f794eb94f090e86 Mon Sep 17 00:00:00 2001 From: "Zhian N. Kamvar" Date: Wed, 25 Jan 2023 12:32:09 -0800 Subject: [PATCH 11/16] update callr test for new knitr --- tests/testthat/test-utils-callr.R | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/testthat/test-utils-callr.R b/tests/testthat/test-utils-callr.R index a353c8b19..137a98c6a 100644 --- a/tests/testthat/test-utils-callr.R +++ b/tests/testthat/test-utils-callr.R @@ -9,7 +9,7 @@ fs::dir_create(src) fs::dir_create(out) copy_template("license", src, "test1.md") - writeLines("---\ntitle: a\n---\n\nHello from `r R.version.string`\n\n```{css}\n#| echo: false\n.my-class {padding: 25px};\n```\n", t2) + writeLines("---\ntitle: a\n---\n\nHello from `r R.version.string`\n\n```{css css-chunk}\n#| echo: false\n.my-class {padding: 25px};\n```\n", t2) withr::defer({ fs::dir_delete(src) fs::dir_delete(out) @@ -39,7 +39,7 @@ test_that("callr_build_episode_md() works with Rmarkdown", { path = t2, hash = NULL, workenv = new.env(), outpath = o2, workdir = fs::path_dir(o2), root = "", quiet = FALSE ) %>% - expect_output("inline R code") %>% + expect_output("css-chunk") %>% expect_message("processing file:") }) expect_true(fs::file_exists(o2)) From f5ab4b5b2a89e2de002c9c5f3f157a1617f67c2c Mon Sep 17 00:00:00 2001 From: "Zhian N. Kamvar" Date: Wed, 25 Jan 2023 12:35:43 -0800 Subject: [PATCH 12/16] update build_episode tests for knitr --- tests/testthat/examples/s3.Rmd | 2 +- tests/testthat/test-build_episode.R | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/testthat/examples/s3.Rmd b/tests/testthat/examples/s3.Rmd index 503f4a13f..36e390b81 100644 --- a/tests/testthat/examples/s3.Rmd +++ b/tests/testthat/examples/s3.Rmd @@ -12,7 +12,7 @@ this is an instructor note ::: ```{r} -#| name: example +#| label: example-chunk point <- function(x, y) { stopifnot(is.numeric(x), is.numeric(y)) structure(list(x = x, y = y), class = "point") diff --git a/tests/testthat/test-build_episode.R b/tests/testthat/test-build_episode.R index 1608553a7..16404b2db 100644 --- a/tests/testthat/test-build_episode.R +++ b/tests/testthat/test-build_episode.R @@ -29,7 +29,7 @@ test_that("build_episode functions works independently", { skip_on_os("windows") expect_output({ res <- build_episode_md(fun_file, workdir = dirname(fun_file)) - }, "inline R code fragments") + }, "example-chunk") # name of chunk in template episode expect_equal(basename(res), "fun.md") expect_true(file.exists(file.path(tmp, "site", "built", "fun.md"))) From cfdbe76b3931c87ded89ef81df9ba3315d9580b8 Mon Sep 17 00:00:00 2001 From: "Zhian N. Kamvar" Date: Wed, 25 Jan 2023 12:50:30 -0800 Subject: [PATCH 13/16] make sure knitr is at at least 1.42 for check --- DESCRIPTION | 1 + 1 file changed, 1 insertion(+) diff --git a/DESCRIPTION b/DESCRIPTION index b62f60343..f9524104e 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -67,6 +67,7 @@ Encoding: UTF-8 LazyData: true Config/testthat/edition: 3 Config/testthat/parallel: false +Config/Needs/check: knitr@>= 1.42 Roxygen: list(markdown = TRUE) RoxygenNote: 7.2.2 URL: https://carpentries.github.io/sandpaper/, https://github.com/carpentries/sandpaper/, https://carpentries.github.io/workbench/ From 95ed95dfff8b5bc011294e30cac68dd1f51ad527 Mon Sep 17 00:00:00 2001 From: "Zhian N. Kamvar" Date: Wed, 25 Jan 2023 12:52:51 -0800 Subject: [PATCH 14/16] oh setup-r-dependencies no lieky --- DESCRIPTION | 1 - 1 file changed, 1 deletion(-) diff --git a/DESCRIPTION b/DESCRIPTION index f9524104e..b62f60343 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -67,7 +67,6 @@ Encoding: UTF-8 LazyData: true Config/testthat/edition: 3 Config/testthat/parallel: false -Config/Needs/check: knitr@>= 1.42 Roxygen: list(markdown = TRUE) RoxygenNote: 7.2.2 URL: https://carpentries.github.io/sandpaper/, https://github.com/carpentries/sandpaper/, https://carpentries.github.io/workbench/ From 0b8e4794efe669202b9eebe3219410a07931c5c4 Mon Sep 17 00:00:00 2001 From: "Zhian N. Kamvar" Date: Wed, 25 Jan 2023 14:22:30 -0800 Subject: [PATCH 15/16] fix tests for ubuntu Ubuntu is not fast enough to catch the progressbar's output or has some other malfunction, so I'm checking that it has the output indicating that a file is being processed. --- tests/testthat/helper-processing.R | 7 +++++++ tests/testthat/test-build_episode.R | 5 +---- tests/testthat/test-build_lesson.R | 13 +++++++------ tests/testthat/test-build_markdown.R | 6 +++--- tests/testthat/test-utils-callr.R | 1 - 5 files changed, 18 insertions(+), 14 deletions(-) create mode 100644 tests/testthat/helper-processing.R diff --git a/tests/testthat/helper-processing.R b/tests/testthat/helper-processing.R new file mode 100644 index 000000000..650505a10 --- /dev/null +++ b/tests/testthat/helper-processing.R @@ -0,0 +1,7 @@ +# utility for checking the output of knotr +# +# @param object R object to evaluate +# @param file the filename to have things escaped +processing_ <- function (file) { + sprintf("processing file: .+?%s", sub("\\.", "\\\\.", basename(file))) +} diff --git a/tests/testthat/test-build_episode.R b/tests/testthat/test-build_episode.R index 16404b2db..6b35ba040 100644 --- a/tests/testthat/test-build_episode.R +++ b/tests/testthat/test-build_episode.R @@ -1,8 +1,5 @@ res <- tmp <- restore_fixture() -print(fs::dir_tree(res, recurse = 1)) - - test_that("build_episode_html() returns nothing for an empty page", { skip_if_not(rmarkdown::pandoc_available("2.11")) @@ -29,7 +26,7 @@ test_that("build_episode functions works independently", { skip_on_os("windows") expect_output({ res <- build_episode_md(fun_file, workdir = dirname(fun_file)) - }, "example-chunk") # name of chunk in template episode + }, processing_(fun_file)) expect_equal(basename(res), "fun.md") expect_true(file.exists(file.path(tmp, "site", "built", "fun.md"))) diff --git a/tests/testthat/test-build_lesson.R b/tests/testthat/test-build_lesson.R index 4feb6532f..095fdcc6a 100644 --- a/tests/testthat/test-build_lesson.R +++ b/tests/testthat/test-build_lesson.R @@ -11,7 +11,7 @@ test_that("Lessons built for the first time are noisy", { # It's noisy at first suppressMessages({ expect_output(build_lesson(tmp, preview = FALSE, quiet = FALSE), - "pyramid") # chunk name from example episode + processing_("introduction.Rmd")) # chunk name from example episode }) htmls <- read_all_html(sitepath) expect_setequal(names(htmls$learner), @@ -186,7 +186,7 @@ test_that("single files can be built", { suppressMessages({ rdr$render() %>% - expect_output("pyramid") %>% # chunk name from example episode + expect_output(processing_("second-episode.Rmd")) %>% expect_message("Output created: .*second-episode.html") }) @@ -215,7 +215,7 @@ test_that("single files can be re-built", { suppressMessages({ rdr$render() %>% - expect_output("pyramid") %>% # chunk name from example episode + expect_output(processing_("second-episode.Rmd")) %>% expect_message("Output created: .*second-episode.html") }) @@ -268,7 +268,7 @@ test_that("files will not be rebuilt unless they change in content", { suppressMessages({ expect_failure({ expect_output(build_lesson(tmp, preview = FALSE, quiet = FALSE), - "pyramid") # chunk name from example episode + processing_("second-episode.Rmd")) }) }) @@ -277,7 +277,7 @@ test_that("files will not be rebuilt unless they change in content", { suppressMessages({ expect_failure({ expect_output(build_lesson(tmp, preview = FALSE, quiet = FALSE), - "pyramid") # chunk name from example episode + processing_("introduction.Rmd")) }) }) @@ -362,7 +362,8 @@ test_that("episodes with HTML in the title are rendered correctly", { writeLines(se, fs::path(tmp, "episodes", "second-episode.Rmd")) suppressMessages({ - expect_output(build_lesson(tmp, preview = FALSE, quiet = FALSE), "pyramid") + expect_output(build_lesson(tmp, preview = FALSE, quiet = FALSE), + processing_("second-episode.Rmd")) }) h1 <- expect_hashed(tmp, "introduction.Rmd") diff --git a/tests/testthat/test-build_markdown.R b/tests/testthat/test-build_markdown.R index fa159fe80..c712d1a4f 100644 --- a/tests/testthat/test-build_markdown.R +++ b/tests/testthat/test-build_markdown.R @@ -38,7 +38,7 @@ test_that("markdown sources can be built without fail", { suppressMessages({ build_markdown(res, quiet = FALSE) %>% expect_message("Consent to use package cache (provided|not given)") %>% - expect_output("pyramid") # chunk name from template episode + expect_output(processing_("second-episode.Rmd")) }) # # Accidentaly rendered HTML is removed before building @@ -76,7 +76,7 @@ test_that("changes in config.yaml triggers a rebuild of the site yaml", { -test_that("markdown sources can be rebuilt without fail", { +test_that("markdown sources can be rebuilt without renv", { # no building needed skip_on_os("windows") @@ -94,7 +94,7 @@ test_that("markdown sources can be rebuilt without fail", { suppressMessages({ build_markdown(res, quiet = FALSE, rebuild = TRUE) %>% expect_message("Consent to use package cache not given.") %>% - expect_output("pyramid") # chunk name from template episode + expect_output(processing_("second-episode")) # chunk name from template episode }) expect_false(getOption("sandpaper.use_renv")) }) diff --git a/tests/testthat/test-utils-callr.R b/tests/testthat/test-utils-callr.R index 137a98c6a..90453d522 100644 --- a/tests/testthat/test-utils-callr.R +++ b/tests/testthat/test-utils-callr.R @@ -39,7 +39,6 @@ test_that("callr_build_episode_md() works with Rmarkdown", { path = t2, hash = NULL, workenv = new.env(), outpath = o2, workdir = fs::path_dir(o2), root = "", quiet = FALSE ) %>% - expect_output("css-chunk") %>% expect_message("processing file:") }) expect_true(fs::file_exists(o2)) From 1bf3f7d9d6e50d9bcc5430e39ce24c49d2af9788 Mon Sep 17 00:00:00 2001 From: "Zhian N. Kamvar" Date: Wed, 25 Jan 2023 14:28:53 -0800 Subject: [PATCH 16/16] make the helper code comment clearer --- tests/testthat/helper-processing.R | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/tests/testthat/helper-processing.R b/tests/testthat/helper-processing.R index 650505a10..0d1637d9a 100644 --- a/tests/testthat/helper-processing.R +++ b/tests/testthat/helper-processing.R @@ -1,7 +1,5 @@ -# utility for checking the output of knotr -# -# @param object R object to evaluate -# @param file the filename to have things escaped +# This helper allows me to properly check the output of knitr without running +# afoul of the weird escapes it has processing_ <- function (file) { sprintf("processing file: .+?%s", sub("\\.", "\\\\.", basename(file))) }