New expect_identical_linter

DESCRIPTION

@@ -62,6 +62,7 @@ Collate:
     'duplicate_argument_linter.R'
     'equals_na_linter.R'
     'exclude.R'
+    'expect_identical_linter.R'
     'expect_length_linter.R'
     'expect_lint.R'
     'expect_named_linter.R'

NAMESPACE

@@ -28,6 +28,7 @@ export(default_undesirable_functions)
 export(default_undesirable_operators)
 export(duplicate_argument_linter)
 export(equals_na_linter)
+export(expect_identical_linter)
 export(expect_length_linter)
 export(expect_lint)
 export(expect_lint_free)

NEWS.md

@@ -94,6 +94,7 @@ function calls. (#850, #851, @renkun-ken)
    + `expect_true_false_linter()` Require usage of `expect_true(x)` over `expect_equal(x, TRUE)` and similar
    + `expect_named_linter()` Require usage of `expect_named(x, n)` over `expect_equal(names(x), n)` and similar
    * `expect_length_linter()` Require usage of `expect_length(x, n)` over `expect_equal(length(x), n)` and similar
+   * `expect_identical_linter()` Require usage of `expect_identical()` by default, and `expect_equal()` only by exception
 * `assignment_linter()` now lints right assignment (`->` and `->>`) and gains two arguments. `allow_cascading_assign` (`TRUE` by default) toggles whether to lint `<<-` and `->>`; `allow_right_assign` toggles whether to lint `->` and `->>` (#915, @michaelchirico)
 * `infix_spaces_linter()` gains argument `exclude_operators` to disable lints on selected infix operators. By default, all "low-precedence" operators throw lints; see `?infix_spaces_linter` for an enumeration of these. (#914 @michaelchirico)
 * `infix_spaces_linter()` now throws a lint on `a~b` and `function(a=1) {}` (#930, @michaelchirico)

R/expect_identical_linter.R

@@ -0,0 +1,80 @@
+#' Require usage of expect_identical(x, y) where appropriate
+#'
+#' At Google, [testthat::expect_identical()] should be the default/go-to function for
+#'   comparing an output to an expected value. `expect_true(identical(x, y))`
+#'   is an equivalent but unadvised method of the same test. Further,
+#'   [testthat::expect_equal()] should only be used when `expect_identical()`
+#'   is inappropriate, i.e., when `x` and `y` need only be *numerically
+#'   equivalent* instead of fully identical (in which case, provide the
+#'   `tolerance=` argument to `expect_equal()` explicitly). This also applies
+#'   when it's inconvenient to check full equality (e.g., names can be ignored,
+#'   in which case `ignore_attr = "names"` should be supplied to
+#'   `expect_equal()` (or, for 2nd edition, `check.attributes = FALSE`).
+#'
+#' @evalRd rd_tags("expect_identical_linter")
+#' @seealso [linters] for a complete list of linters available in lintr.
+#' @export
+expect_identical_linter <- function() {
+  Linter(function(source_file) {
+    if (length(source_file$parsed_content) == 0L) {
+      return(list())
+    }
+
+    xml <- source_file$xml_parsed_content
+
+    # outline:
+    #   1. conditions for expect_equal()
+    #     - skip when tolerance= is set
+    #     - skip when check.attributes = FALSE [for 2nd edition tests]
+    #     - skip when any ignore_* is set [for 3rd edition tests]
+    #       specifically ignore_{srcref,attr,encoding,function_env,formula_env}
+    #     - skip cases like expect_equal(x, 1.02) or the constant vector version
+    #       where a numeric constant indicates inexact testing is preferable
+    #     - skip calls using dots (`...`); see tests
+    #   2. conditions for expect_true()
+    xpath <- glue::glue("//expr[
+      (
+        SYMBOL_FUNCTION_CALL[text() = 'expect_equal']
+        and not(
+          following-sibling::SYMBOL_SUB[text() = 'tolerance']
+          or following-sibling::SYMBOL_SUB[
+            (
+              text() = 'check.attributes'
+              and following-sibling::expr[1][NUM_CONST[text() = 'FALSE']]
+            )
+            or starts-with(text(), 'ignore_')
+          ]
+          or following-sibling::expr[
+            expr[SYMBOL_FUNCTION_CALL[text() = 'c']]
+            and expr[NUM_CONST[contains(text(), '.')]]
+          ]
+          or following-sibling::expr[NUM_CONST[contains(text(), '.')]]
+          or following-sibling::expr[SYMBOL[text() = '...']]
+        )
+      ) or (
+        SYMBOL_FUNCTION_CALL[text() = 'expect_true']
+        and following-sibling::expr[1][
+          expr[SYMBOL_FUNCTION_CALL[text() = 'identical']]
+        ]
+      )
+    ]")
+
+    bad_expr <- xml2::xml_find_all(xml, xpath)
+    return(lapply(
+      bad_expr,
+      xml_nodes_to_lint,
+      source_file = source_file,
+      message = paste(
+        "expect_identical(x, y) is the default way of comparing two objects in",
+        "testthat unit tests. Only use expect_equal() if testing numeric",
+        "equality (and specifying tolerance= explicitly) or there's a need to",
+        "ignore some attributes, e.g. with ignore_attr = 'names' for the 3rd",
+        "edition or check.attributes = FALSE for the second. For testing",
+        "approximate equality to a whole number, you can also avoid setting",
+        "tolerance= by including an explicit decimal point, e.g.,",
+        "expect_equal(x, 1.0), not expect_equal(x, 1)."
+      ),
+      type = "warning"
+    ))
+  })
+}

inst/lintr/linters.csv

@@ -8,6 +8,7 @@ commented_code_linter,style readability best_practices default
 cyclocomp_linter,style readability best_practices default configurable
 duplicate_argument_linter,correctness common_mistakes configurable
 equals_na_linter,robustness correctness common_mistakes default
+expect_identical_linter,package_development
 expect_length_linter,package_development best_practices readability
 expect_named_linter,package_development best_practices readability
 expect_not_linter,package_development best_practices readability

tests/testthat/test-expect_identical_linter.R

@@ -0,0 +1,67 @@
+test_that("expect_identical_linter skips allowed usages", {
+  # expect_type doesn't have an inverted version
+  expect_lint("expect_true(identical(x, y) || identical(y, z))", NULL, expect_identical_linter())
+  # NB: also applies to tinytest, but it's sufficient to test testthat
+  expect_lint("testthat::expect_true(identical(x, y) || identical(y, z))", NULL, expect_identical_linter())
+
+  # expect_equal calls with explicit tolerance= are OK
+  expect_lint("expect_equal(x, y, tolerance = 1e-6)", NULL, expect_identical_linter())
+
+  # ditto for check.attributes = FALSE
+  expect_lint("expect_equal(x, y, check.attributes = FALSE)", NULL, expect_identical_linter())
+})
+
+test_that("expect_identical_linter blocks simple disallowed usages", {
+  expect_lint(
+    "expect_equal(x, y)",
+    rex::rex("expect_identical(x, y) is the default way of comparing two objects"),
+    expect_identical_linter()
+  )
+
+  # check.attributes = TRUE (set explicitly) gets dinged
+  expect_lint(
+    "expect_equal(x, y, check.attributes = TRUE)",
+    rex::rex("expect_identical(x, y) is the default way of comparing two objects"),
+    expect_identical_linter()
+  )
+
+  # different usage to redirect to expect_identical
+  expect_lint(
+    "expect_true(identical(x, y))",
+    rex::rex("expect_identical(x, y) is the default way of comparing two objects"),
+    expect_identical_linter()
+  )
+})
+
+test_that("expect_identical_linter skips cases likely testing numeric equality", {
+  expect_lint("expect_equal(x, 1.034)", NULL, expect_identical_linter())
+  expect_lint("expect_equal(x, c(1.01, 1.02))", NULL, expect_identical_linter())
+  # whole numbers with explicit decimals are OK, even in mixed scenarios
+  expect_lint("expect_equal(x, c(1.0, 2))", NULL, expect_identical_linter())
+  # plain numbers are still caught, however
+  expect_lint(
+    "expect_equal(x, 1L)",
+    rex::rex("expect_identical(x, y) is the default way of comparing two objects"),
+    expect_identical_linter()
+  )
+  expect_lint(
+    "expect_equal(x, 1)",
+    rex::rex("expect_identical(x, y) is the default way of comparing two objects"),
+    expect_identical_linter()
+  )
+  expect_lint(
+    "expect_equal(x, TRUE)",
+    rex::rex("expect_identical(x, y) is the default way of comparing two objects"),
+    expect_identical_linter()
+  )
+})
+
+test_that("expect_identical_linter skips 3e cases needing expect_equal", {
+  expect_lint("expect_equal(x, y, ignore_attr = 'names')", NULL, expect_identical_linter())
+})
+
+# this arose where a helper function was wrapping expect_equal() and
+#   some of the "allowed" arguments were being passed --> false positive
+test_that("expect_identical_linter skips calls using ...", {
+  expect_lint("expect_equal(x, y, ...)", NULL, expect_identical_linter())
+})