Refine the structure of describe and it

Also evaluate `describe()` with `test_code()`. Then refactor `test_code()` to run  `local_test_context()` and find a default reporter, which simplifies calls and makes test code more reliably the same

Fixes #2007

Author: hadley

R/describe.R

@@ -62,30 +62,16 @@ describe <- function(description, code) {
   describe_description <- description
 
   # prepares a new environment for each it-block
-  describe_environment <- new.env(parent = parent.frame())
-  describe_environment$it <- function(description, code = NULL) {
+  describe_env <- new.env(parent = parent.frame())
+  describe_env$it <- function(description, code = NULL) {
     check_string(description, allow_empty = FALSE)
-    code <- substitute(code)
-
     description <- paste0(describe_description, ": ", description)
-    describe_it(description, code, describe_environment)
-  }
 
-  eval(substitute(code), describe_environment)
-  invisible()
-}
-
-describe_it <- function(description, code, env = parent.frame()) {
-  reporter <- get_reporter() %||% local_interactive_reporter()
-  local_test_context()
+    code <- substitute(code)
+    test_code(description, code, env = describe_env, skip_on_empty = FALSE)
+  }
 
-  test_code(
-    description,
-    code,
-    env = env,
-    reporter = reporter,
-    skip_on_empty = FALSE
-  )
+  test_code(description, code, describe_env, skip_on_empty = FALSE)
 }
 
 #' @export
@@ -94,5 +80,5 @@ it <- function(description, code = NULL) {
   check_string(description, allow_empty = FALSE)
 
   code <- substitute(code)
-  describe_it(description, code, env = parent.frame())
+  test_code(description, code, env = parent.frame(), skip_on_empty = FALSE)
 }

R/test-that.R

@@ -46,21 +46,16 @@ test_that <- function(desc, code) {
     }
   }
 
-  # Must initialise interactive reporter before local_test_context()
-  reporter <- get_reporter() %||% local_interactive_reporter()
-  local_test_context()
-
-  test_code(
-    desc,
-    code,
-    env = parent.frame(),
-    reporter = reporter
-  )
+  test_code(desc, code, env = parent.frame())
 }
 
 # Access error fields with `[[` rather than `$` because the
 # `$.Throwable` from the rJava package throws with unknown fields
-test_code <- function(test, code, env, reporter, skip_on_empty = TRUE) {
+test_code <- function(test, code, env, reporter = NULL, skip_on_empty = TRUE) {
+  # Must initialise interactive reporter before local_test_context()
+  reporter <- get_reporter() %||% local_interactive_reporter()
+  local_test_context()
+
   frame <- caller_env()
 
   if (!is.null(test)) {

tests/testthat/_snaps/describe.md

@@ -1,4 +1,4 @@
-# describe: has to have a valid description for the block
+# has to have a valid description for the block
 
     Code
       describe()

tests/testthat/test-describe.R

@@ -1,4 +1,3 @@
-someExternalVariable <- 1
 describe("describe", {
   it("can contain nested describe blocks", {
     describe("addition", {
@@ -18,36 +17,39 @@ describe("describe", {
       it("can be shown that P != NP")
     })
   })
+})
 
-  it("has to have a valid description for the block", {
-    expect_snapshot(error = TRUE, {
-      describe()
-      describe("")
-      describe(c("a", "b"))
-
-      it()
-      it("")
-      it(c("a", "b"))
-    })
-  })
-
+someExternalVariable <- 1
+describe("variable scoping", {
   someInternalVariable <- 1
   it("should be possible to use variables from outer environments", {
     expect_equal(1, someExternalVariable)
     expect_equal(1, someInternalVariable)
+
+    someInternalVariable <- 2
+    someExternalVariable <- 3
   })
 
   # prefix is needed to test this use case
-  testthat::it("should be possible to use variables from outer environments with package prefix", {
+  testthat::it("even when using it() directly", {
     expect_equal(1, someExternalVariable)
     expect_equal(1, someInternalVariable)
   })
 
-  it("should not be possible to access variables from other specs (1)", {
-    some_test_var <- 5
+  it("shouldn't affect other tests", {
+    expect_equal(1, someExternalVariable)
+    expect_equal(1, someInternalVariable)
   })
+})
+
+test_that("has to have a valid description for the block", {
+  expect_snapshot(error = TRUE, {
+    describe()
+    describe("")
+    describe(c("a", "b"))
 
-  it("should not be possible to access variables from other specs (2)", {
-    expect_false(exists("some_test_var"))
+    it()
+    it("")
+    it(c("a", "b"))
   })
 })