New yoda_test_linter

.gitignore

@@ -6,6 +6,7 @@
 bad.R
 script.R
 *~
+\#*\#
 
 *.Rcheck
 lintr_*.tar.gz

DESCRIPTION

@@ -112,5 +112,6 @@ Collate:
     'unneeded_concatenation_linter.R'
     'with_id.R'
     'xp_utils.R'
+    'yoda_test_linter.R'
     'zzz.R'
 Roxygen: list(markdown = TRUE)

NAMESPACE

@@ -76,6 +76,7 @@ export(undesirable_operator_linter)
 export(unneeded_concatenation_linter)
 export(with_defaults)
 export(with_id)
+export(yoda_test_linter)
 import(rex)
 importFrom(cyclocomp,cyclocomp)
 importFrom(stats,na.omit)

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
+   * `yoda_test_linter()` Require usage of `expect_identical(x, 1L)` over `expect_equal(1L, x)` 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)
 * `infix_spaces_linter()` now throws a lint on `a~b` and `function(a=1) {}` (#930, @michaelchirico)

R/yoda_test_linter.R

@@ -0,0 +1,45 @@
+#' Block obvious "yoda tests"
+#'
+#' Yoda tests test `(expected, actual)` instead of the more common `(actual, expected)`.
+#' This is not always possible to detect statically; this linter focuses on
+#'   the simple case of testing an expression against a literal value, e.g.
+#'   `(1L, foo(x))` should be `(foo(x), 1L)`.
+#'
+#' @evalRd rd_tags("yoda_test_linter")
+#' @seealso
+#'   [linters] for a complete list of linters available in lintr.
+#'   <https://en.wikipedia.org/wiki/Yoda_conditions>
+#' @export
+yoda_test_linter <- function() {
+  Linter(function(source_file) {
+    if (length(source_file$parsed_content) == 0L) {
+      return(list())
+    }
+
+    xml <- source_file$xml_parsed_content
+
+    # catch the following types of literal in the first argument:
+    #   (1) numeric literal (e.g. TRUE, 1L, 1.0, NA) [NUM_CONST]
+    #   (2) string literal (e.g. 'str' or "str")     [STR_CONST]
+    #   (3) arithmetic literal (e.g. 1+1 or 0+1i)    [OP-PLUS or OP-MINUS...]
+    xpath <- "//expr[
+      expr[SYMBOL_FUNCTION_CALL[text() = 'expect_equal' or text() = 'expect_identical' or text() = 'expect_setequal']]
+      and expr[2][NUM_CONST or STR_CONST or ((OP-PLUS or OP-MINUS) and count(expr[NUM_CONST]) = 2)]
+    ]"
+
+    bad_expr <- xml2::xml_find_all(xml, xpath)
+
+    return(lapply(
+      bad_expr,
+      xml_nodes_to_lint,
+      source_file = source_file,
+      message = paste(
+        "Tests should compare objects in the order 'actual', 'expected', ",
+        "not the reverse. For example, do ",
+        "expect_identical(CountObservations(x), 2L) instead of ",
+        "expect_identical(2L, CountObservations(x))."
+      ),
+      type = "warning"
+    ))
+  })
+}

inst/lintr/linters.csv

@@ -48,3 +48,4 @@ trailing_whitespace_linter,style default
 undesirable_function_linter,style efficiency configurable robustness best_practices
 undesirable_operator_linter,style efficiency configurable robustness best_practices
 unneeded_concatenation_linter,style readability efficiency
+yoda_test_linter,package_development best_practices readability

tests/testthat/test-yoda_test_linter.R

@@ -0,0 +1,44 @@
+test_that("yoda_test_linter skips allowed usages", {
+  expect_lint("expect_equal(x, 2)", NULL, yoda_test_linter())
+  # namespace qualification doesn't matter
+  expect_lint("testthat::expect_identical(x, 'a')", NULL, yoda_test_linter())
+  # two variables can't be distinguished which is expected/actual (without
+  #   playing quixotic games trying to parse that out from variable names)
+  expect_lint("expect_equal(x, y)", NULL, yoda_test_linter())
+})
+
+test_that("yoda_test_linter blocks simple disallowed usages", {
+  expect_lint(
+    "expect_equal(2, x)",
+    rex::rex("Tests should compare objects in the order 'actual', 'expected'"),
+    yoda_test_linter()
+  )
+  expect_lint(
+    "testthat::expect_identical('a', x)",
+    rex::rex("Tests should compare objects in the order 'actual', 'expected'"),
+    yoda_test_linter()
+  )
+  expect_lint(
+    "expect_setequal(2, x)",
+    rex::rex("Tests should compare objects in the order 'actual', 'expected'"),
+    yoda_test_linter()
+  )
+  # complex literals are slightly odd
+  expect_lint(
+    "expect_equal(2 + 1i, x)",
+    rex::rex("Tests should compare objects in the order 'actual', 'expected'"),
+    yoda_test_linter()
+  )
+})
+
+# TODO(michaelchirico): Should this be extended to RUnit tests? It seems yes,
+#   but the argument names in RUnit (inherited from base all.equal()) are a bit
+#   confusing, e.g. `checkEqual(target=, current=)`. From the name, one might
+#   reasonably conclude 'expected' comes first, and 'actual' comes second.
+# TODO(michaelchirico): What sorts of combinations of literals can be included?
+#   e.g. expect_equal(c(1, 2), x) is a yoda test; is expect_equal(c(x, 1), y)?
+#   clearly it's not true for general f() besides c(). What about other
+#   constructors of literals? data.frame(), data.table(), tibble(), ...?
+# TODO(michaelchirico): The logic could also be extended to "tests" inside regular
+#   code, not just test suites, e.g. `if (2 == x)`, `while(3 <= x)`,
+#   `stopifnot('a' == foo(y))`.