Updates for testthat 3rd edition (#38)

Re-enable CI for the third edition Set 3rd edition Punt on some weird (public()?) related new failures No more extraneous warnings use a helper for pre-testthat 3.0 The helper isn't available within public() remove a silly typo of safe_untrace(untrace(...)) Suppress all of the extraneous messages during tests install libcurl on ubuntu + 3.3 Proper yaml Fix CRLF issue on GHA (#37) use a single warning call with expect_header PR comments, add `quiet_context()`, test all messages emmitted (in 3e) clean up some tests oops We shouldn't need this anymore 📰 News polishing Co-authored-by: Neal Richardson <neal.p.richardson@gmail.com>
nealrichardson · Oct 20, 2020 · 96e9ea6 · 96e9ea6
1 parent e63111f
commit 96e9ea6
Show file tree

Hide file tree

Showing 22 changed files with 169 additions and 100 deletions.
diff --git a/.github/workflows/testthat-editions.yaml b/.github/workflows/testthat-editions.yaml
@@ -13,7 +13,7 @@ jobs:
       matrix:
         config:
         - { os: ubuntu-20.04, r: 'release', testhat_e: '2',  rspm: "https://packagemanager.rstudio.com/cran/__linux__/focal/latest" }
-        # - { os: ubuntu-20.04, r: 'release', testhat_e: '3',  rspm: "https://packagemanager.rstudio.com/cran/__linux__/focal/latest" }
+        - { os: ubuntu-20.04, r: 'release', testhat_e: '3',  rspm: "https://packagemanager.rstudio.com/cran/__linux__/focal/latest" }
 
     env:
       R_REMOTES_NO_ERRORS_FROM_WARNINGS: true

diff --git a/DESCRIPTION b/DESCRIPTION
@@ -42,3 +42,4 @@ Language: en-US
 RoxygenNote: 7.0.2
 Roxygen: list(markdown = TRUE)
 VignetteBuilder: knitr
+Config/testthat/edition: 3
diff --git a/NEWS.md b/NEWS.md
@@ -1,8 +1,17 @@
 # httptest 3.3.0.9000 (under development)
-* changed the filename `helper.R` to `setup.R` to comply with `testthat`'s latest recommendations (#44, @maelle).
-* Mocking PUT and POST with a body consisting of only `httr::upload_file` no longer leaves a file connection open.
+
+## Changes for compatibility with the forthcoming testthat 3rd edition
+
+* `expect_header()` now emits a single warning with all of the headers that are included with the call (instead of one warning per header). This makes catching multiple headers easier and prevents excess warnings when using testthat 3e (#38, @jonkeane)
+* Quiet an extraneous message about untracing `curl::form_file()` (#35, @dmenne)
+* Numerous internal testing changes to ensure compatibility with testthat 3e
+
+## Other fixes and enhancements
+
+* Mocking PUT and POST with a body consisting of only `httr::upload_file()` no longer leaves a file connection open.
 * Mock files with special characters in the filename are now correctly found (#33, @natbprice)
 * Switch continuous integration to use GitHub Actions (#36, @jonkeane)
+* The test file `helper.R` was renamed to `setup.R` to comply with `testthat`'s latest recommendations (#44, @maelle)
 
 # httptest 3.3.0
 

diff --git a/R/expect-header.R b/R/expect-header.R
@@ -25,9 +25,8 @@
 expect_header <- function (..., ignore.case=TRUE) {
     tracer <- quote({
         heads <- req$headers
-        for (h in names(heads)) {
-            warning(paste(h, heads[h], sep=": "), call.=FALSE)
-        }
+        msgs <- lapply(names(heads), function(h) paste(h, heads[h], sep=": "))
+        warning(msgs, call.=FALSE)
     })
     with_trace("request_prepare", exit=tracer, where=add_headers, expr={
         expect_warning(..., ignore.case=ignore.case)

diff --git a/R/trace.R b/R/trace.R
@@ -59,7 +59,7 @@ quietly <- function (expr) {
 #' @return Nothing; called for its side effects
 #' @export
 stop_mocking <- function () {
-    safe_untrace(untrace(curl::form_file))
+    safe_untrace(curl::form_file)
     invisible(safe_untrace("request_perform", add_headers))
 }
 

diff --git a/README.md b/README.md
@@ -48,8 +48,8 @@ Adding `with_mock_api()` to your tests is straightforward. Given a very basic te
 
 ```r
 test_that("Requests happen", {
-    expect_is(GET("http://httpbin.org/get"), "response")
-    expect_is(
+    expect_s3_class(GET("http://httpbin.org/get"), "response")
+    expect_s3_class(
         GET("http://httpbin.org/response-headers",
             query=list(`Content-Type`="application/json")),
         "response"
@@ -62,8 +62,8 @@ if we wrap the code in `with_mock_api()`, actual requests won't happen.
 ```r
 with_mock_api({
     test_that("Requests happen", {
-        expect_is(GET("http://httpbin.org/get"), "response")
-        expect_is(
+        expect_s3_class(GET("http://httpbin.org/get"), "response")
+        expect_s3_class(
             GET("http://httpbin.org/response-headers",
                 query=list(`Content-Type`="application/json")),
             "response"

diff --git a/tests/testthat/setup.R b/tests/testthat/setup.R
@@ -52,3 +52,21 @@ skip_on_R_older_than <- function (version) {
         skip(paste("Requires R >=", version))
     }
 }
+
+# A quiet version of httr's content
+quiet_content <- function(...) {
+    suppressMessages(content(...))
+}
+
+testthat_transition <- function(old, new) {
+    is_3e <- tryCatch(testthat::edition_get() == 3, error = function(e) FALSE)
+    if (is_3e) {
+        eval(new, envir = parent.frame())
+    } else {
+        eval(old, envir = parent.frame())
+    }
+}
+
+# assign to global to be used inside of `public()` calls
+third_edition <<- tryCatch(testthat::edition_get() == 3, error = function(e) FALSE)
+
diff --git a/tests/testthat/test-capture-requests.R b/tests/testthat/test-capture-requests.R
@@ -1,5 +1,3 @@
-context("capture_requests")
-
 d <- tempfile()
 dl_file <- tempfile()
 webp_file <- tempfile()
@@ -34,7 +32,7 @@ test_that("We can record a series of requests (a few ways)", {
           ))
     ## Test the contents of the .R files
     teapot <- source(file.path(d, "httpbin.org/status/418.R"))$value
-    expect_is(teapot, "response")
+    expect_s3_class(teapot, "response")
     expect_identical(teapot$status_code, 418L)
     ## Make sure that our .html file has HTML
     expect_true(any(grepl("</body>",
@@ -71,11 +69,11 @@ test_that("We can then load the mocks it stores", {
     ## Compare the HTML as text because the parsed HTML (XML) object has a
     ## C pointer that is different between the two objects.
     expect_identical(
-      enc2native(content(m2, "text")),
+      enc2native(quiet_content(m2, "text")),
       enc2native(content(r2, "text"))
     )
 
-    expect_true(grepl("</body>", content(m2, "text")))
+    expect_true(grepl("</body>", quiet_content(m2, "text")))
     expect_identical(content(m3), content(r3))
     expect_identical(content(m4), content(r4))
     expect_identical(content(m5), content(r5))
@@ -140,7 +138,7 @@ with_mock_api({
         expect_true(setequal(dir(d3, recursive=TRUE),
             c("example.com/get.R", "api/object1.R", "httpbin.org/status/204.R")))
         response <- source(file.path(d3, "example.com/get.R"))$value
-        expect_is(response, "response")
+        expect_s3_class(response, "response")
         expect_identical(content(response),
             content(GET("http://example.com/get/")))
     })

diff --git a/tests/testthat/test-content-type.R b/tests/testthat/test-content-type.R
@@ -1,5 +1,3 @@
-context("Content-Type parsing")
-
 resp <- source("example.com/html.R")$value
 
 test_that("get_content_type handles valid Content-Types, including omitted", {

diff --git a/tests/testthat/test-expect-header.R b/tests/testthat/test-expect-header.R
@@ -1,20 +1,24 @@
-context("expect_header")
-
 public({
     with_fake_http({
         test_that("expect_header with fake HTTP", {
             expect_GET(expect_success(expect_header(GET("http://httpbin.org/",
                 config=add_headers(Accept="image/jpeg")),
                 "Accept: image/jpeg")))
-            expect_GET(expect_failure(expect_header(GET("http://httpbin.org/",
-                config=add_headers(Accept="image/png")),
-                "Accept: image/jpeg")))
+            expect_GET(expect_failure(expect_warning(
+                expect_header(GET("http://httpbin.org/",
+                    config=add_headers(Accept="image/png")),
+                    "Accept: image/jpeg"),
+                "Accept: image/png"
+            )))
             expect_POST(expect_success(expect_header(POST("http://httpbin.org/",
                 config=add_headers(Accept="image/jpeg")),
                 "Accept: image/jpeg")))
-            expect_POST(expect_failure(expect_header(POST("http://httpbin.org/",
-                config=add_headers(Accept="image/png")),
-                "Accept: image/jpeg")))
+            expect_POST(expect_failure(expect_warning(
+                expect_header(POST("http://httpbin.org/",
+                    config=add_headers(Accept="image/png")),
+                    "Accept: image/jpeg"),
+                "Content-Type: Accept: image/png"
+            )))
         })
     })
 
@@ -23,24 +27,32 @@ public({
             expect_success(expect_header(GET("api/object1/",
                 config=add_headers(Accept="image/jpeg")),
                 "Accept: image/jpeg"))
-            expect_failure(expect_header(GET("api/object1/",
-                config=add_headers(Accept="image/png")),
-                "Accept: image/jpeg"))
-            expect_POST(expect_success(expect_header(POST("http://httpbin.org/",
-                config=add_headers(Accept="image/jpeg")),
-                "Accept: image/jpeg")))
-            expect_failure(expect_header(expect_POST(POST("http://httpbin.org/",
-                config=add_headers(Accept="image/png")), silent=TRUE),
+            suppressWarnings(
+                expect_failure(expect_header(GET("api/object1/",
+                    config=add_headers(Accept="image/png")),
+                    "Accept: image/jpeg"))
+            )
+            suppressWarnings(
+                expect_POST(expect_success(expect_header(POST("http://httpbin.org/",
+                    config=add_headers(Accept="image/jpeg")),
+                    "Accept: image/jpeg")))
+            )
+            skip_if(third_edition)
+            expect_failure(expect_header(
+                expect_POST(POST("http://httpbin.org/",
+                    config=add_headers(Accept="image/png")), silent=TRUE),
                 "Accept: image/jpeg"))
         })
         test_that("expect_header ignore.case", {
             expect_success(expect_header(GET("api/object1/",
                 config=add_headers(Accept="image/jpeg")),
                 "accept: image/jpeg"))
-            expect_failure(expect_header(GET("api/object1/",
-                config=add_headers(Accept="image/jpeg")),
-                "accept: image/jpeg",
-                ignore.case=FALSE))
+            suppressWarnings(
+                expect_failure(expect_header(GET("api/object1/",
+                    config=add_headers(Accept="image/jpeg")),
+                    "accept: image/jpeg",
+                    ignore.case=FALSE))
+            )
         })
     })
 
@@ -49,9 +61,12 @@ public({
             expect_GET(expect_success(expect_header(GET("http://httpbin.org/",
                 config=add_headers(Accept="image/jpeg")),
                 "Accept: image/jpeg")))
-            expect_GET(expect_failure(expect_header(GET("http://httpbin.org/",
-                config=add_headers(Accept="image/png")),
-                "Accept: image/jpeg")))
+            expect_GET(expect_warning(
+                expect_failure(expect_header(GET("http://httpbin.org/",
+                    config=add_headers(Accept="image/png")),
+                    "Accept: image/jpeg")),
+                "Accept: image/png"
+            ))
         })
     })
 
@@ -60,8 +75,11 @@ public({
         expect_success(expect_header(GET("http://httpbin.org/get",
             config=add_headers(Accept="image/jpeg")),
             "Accept: image/jpeg"))
-        expect_failure(expect_header(GET("http://httpbin.org/get",
-            config=add_headers(Accept="image/png")),
-            "Accept: image/jpeg"))
+        expect_failure(expect_warning(
+            expect_header(GET("http://httpbin.org/get",
+                config=add_headers(Accept="image/png")),
+                "Accept: image/jpeg"),
+            "Accept: image/png"
+        ))
     })
 })
diff --git a/tests/testthat/test-fake-http.R b/tests/testthat/test-fake-http.R
@@ -1,5 +1,3 @@
-context("Fake HTTP")
-
 public({
     with_fake_http({
         test_that("fakeGET", {
@@ -85,17 +83,25 @@ public({
 })
 
 test_that("fake_response returns a valid enough response even if you give it just a URL", {
-    expect_is(fake_response("http://httpbin.org/get"), "response")
+    expect_s3_class(fake_response("http://httpbin.org/get"), "response")
 })
 
 test_that("fake_request gets covered directly (not just in tracer)", {
-    expect_is(fake_request(list(method="GET", url="http://httpbin.org/get")),
-        "response")
-    expect_is(fake_request(
-        list(
-            method="POST",
-            url="http://httpbin.org/get",
-            options=list(postfields=charToRaw("body"))
-        )),
+    expect_s3_class(
+        expect_message(
+            fake_request(list(method="GET", url="http://httpbin.org/get")),
+            "GET http://httpbin.org/get"
+        ),
+    "response")
+    expect_s3_class(
+        expect_message(
+            fake_request(
+                list(
+                    method="POST",
+                    url="http://httpbin.org/get",
+                    options=list(postfields=charToRaw("body"))
+                )),
+            "POST http://httpbin.org/get body"
+        ),
         "response")
 })
diff --git a/tests/testthat/test-find-redactor.R b/tests/testthat/test-find-redactor.R
@@ -1,5 +1,3 @@
-context("Find package redactors")
-
 expect_redactor <- function (expr) {
     expect_identical(names(formals(expr)), "response")
 }
@@ -34,16 +32,34 @@ test_that("get_current_redactor edge cases", {
 with_mock_api({
     test_that("Reading redactors from within a package (and install that package)", {
         newmocks <- tempfile()
-        expect_message(
-            capture_while_mocking(path=newmocks, {
-                ## Install the "testpkg" to a temp lib.loc _after_ we've already started recording
-                lib <- install_testpkg("testpkg")
-                library(testpkg, lib.loc=lib)
-                expect_true("testpkg" %in% names(sessionInfo()$otherPkgs))
+        testthat_transition(
+            expect_message(
+                capture_while_mocking(path=newmocks, {
+                    ## Install the "testpkg" to a temp lib.loc _after_ we've
+                    ## already started recording
+                    lib <- install_testpkg("testpkg")
+                    library(testpkg, lib.loc=lib)
+                    expect_true("testpkg" %in% names(sessionInfo()$otherPkgs))
 
-                r <- GET("http://example.com/get")
-            }),
-            paste0("Using redact.R from ", dQuote("testpkg"))
+                    r <- GET("http://example.com/get")
+                }),
+                paste0("Using redact.R from ", dQuote("testpkg"))
+            ),
+            expect_message(
+                expect_message(
+                    capture_while_mocking(path=newmocks, {
+                        ## Install the "testpkg" to a temp lib.loc _after_ we've
+                        ## already started recording
+                        lib <- install_testpkg("testpkg")
+                        library(testpkg, lib.loc=lib)
+                        expect_true("testpkg" %in% names(sessionInfo()$otherPkgs))
+
+                        r <- GET("http://example.com/get")
+                    }),
+                    paste0("Using redact.R from ", dQuote("testpkg"))
+                ),
+                paste0("Using request.R from ", dQuote("testpkg"))
+            )
         )
         with_mock_path(newmocks, {
             r2 <- GET("http://example.com/get")
@@ -84,7 +100,7 @@ with_mock_api({
         })
         expect_message(
             capture_while_mocking(path=newmocks3, {
-                pkgload::load_all("testpkg")
+                pkgload::load_all("testpkg", quiet = TRUE)
                 expect_true("testpkg" %in% names(sessionInfo()$otherPkgs))
                 r <- GET("http://example.com/get")
             }),

diff --git a/tests/testthat/test-json-equal.R b/tests/testthat/test-json-equal.R
@@ -1,5 +1,3 @@
-context("JSON equivalence")
-
 test_that("json_compare", {
     obj <- list(c=1, b=list(list(2, 3), list(d=9, f=5)), a=5)
     expect_false(json_compare(list(1, 2), list(2, 1))$equal)

diff --git a/tests/testthat/test-mock-api.R b/tests/testthat/test-mock-api.R
@@ -1,5 +1,3 @@
-context("Mock API")
-
 public({
     with_mock_api({
         test_that("Can load an object and file extension is added", {
@@ -201,9 +199,9 @@ test_that("load_response invalid extension handling", {
 })
 
 test_that("mock_request code paths are covered (outside of trace)", {
-    expect_is(mock_request(list(method="GET", url="api/")),
+    expect_s3_class(mock_request(list(method="GET", url="api/")),
         "response")
-    expect_is(mock_request(list(method="GET", url="http://example.com/html")),
+    expect_s3_class(mock_request(list(method="GET", url="http://example.com/html")),
         "response")
     expect_error(mock_request(list(method="PUT", url="api/")))
 })
diff --git a/tests/testthat/test-mock-paths.R b/tests/testthat/test-mock-paths.R
@@ -1,5 +1,3 @@
-context("Setting different/multiple mock directories")
-
 public({
     test_that(".mockPaths works more or less like .libPaths", {
         expect_identical(.mockPaths(), ".")