New expect_identical_linter (#958)

* New expect_identical_linter

* comment about double-lint test

* simply test for any named argument

* shorten message; put details in man page

* @section

Co-authored-by: AshesITR <alexander.rosenstock@web.de>

Author: MichaelChirico

DESCRIPTION

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

NAMESPACE

@@ -29,6 +29,7 @@ export(default_undesirable_operators)
 export(duplicate_argument_linter)
 export(equals_na_linter)
 export(expect_comparison_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
    * `expect_comparison_linter()` Require usage of `expect_gt(x, y)` over `expect_true(x > y)` and similar
 * `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)

R/expect_identical_linter.R

@@ -0,0 +1,76 @@
+#' 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`).
+#'
+#' @section Exceptions:
+#'
+#' The linter allows `expect_equal()` in three circumstances:
+#'   1. A named argument is set (e.g. `ignore_attr` or `tolerance`)
+#'   2. Comparison is made to an explicit decimal, e.g.
+#'      `expect_equal(x, 1.0)` (implicitly setting `tolerance`)
+#'   3. `...` is passed (wrapper functions whcih might set
+#'      arguments such as `ignore_attr` or `tolerance`)
+#'
+#' @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 any named argument is set. most commonly this
+    #       is check.attributes (for 2e tests) or one of the ignore_*
+    #       arguments (for 3e tests). This will generate some false
+    #       negatives, but will be much easier to maintain.
+    #     - 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
+          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,
+      lint_message = paste(
+        "Use expect_identical(x, y) by default; resort to expect_equal() only when needed,",
+        "e.g. when setting ignore_attr= or tolerance=."
+      ),
+      type = "warning"
+    ))
+  })
+}

inst/lintr/linters.csv

@@ -9,6 +9,7 @@ cyclocomp_linter,style readability best_practices default configurable
 duplicate_argument_linter,correctness common_mistakes configurable
 equals_na_linter,robustness correctness common_mistakes default
 expect_comparison_linter,package_development best_practices
+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

man/expect_identical_linter.Rd

@@ -0,0 +1,62 @@
+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("Use expect_identical(x, y) by default; resort to expect_equal() only when needed"),
+    expect_identical_linter()
+  )
+
+  # different usage to redirect to expect_identical
+  expect_lint(
+    "expect_true(identical(x, y))",
+    rex::rex("Use expect_identical(x, y) by default; resort to expect_equal() only when needed"),
+    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("Use expect_identical(x, y) by default; resort to expect_equal() only when needed"),
+    expect_identical_linter()
+  )
+  expect_lint(
+    "expect_equal(x, 1)",
+    rex::rex("Use expect_identical(x, y) by default; resort to expect_equal() only when needed"),
+    expect_identical_linter()
+  )
+  # NB: TRUE is a NUM_CONST so we want test matching it, even though this test is
+  #   also a violation of expect_true_false_linter()
+  expect_lint(
+    "expect_equal(x, TRUE)",
+    rex::rex("Use expect_identical(x, y) by default; resort to expect_equal() only when needed"),
+    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())
+})