use xml-based check for yoda_test_linter (#1065)

* use xml-based check for yoda_test_linter

Branch comparison crashing a bunch because of failing on unparseable files -- this is why

* yoda test linter doesnt lint on expect_equal(x$"key", 2) (#1068)

* yoda test linter doesnt lint on expect_equal(x$"key", 2)

* handle pipe-terminal tests in yoda linter (#1069)

* handle pipe-terminal tests in yoda linter

* customize yoda_test_linter message for placeholder tests (#1070)

* customize yoda_test_linter message for placeholder tests

* fix indentation

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

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

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

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

NEWS.md

@@ -112,6 +112,9 @@ function calls. (#850, #851, @renkun-ken)
    + `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
+     + Extended for #979 to improve the lint message displayed for placeholder tests like `expect_equal(1, 1)`
+     + Extended for #1066 to exclude tests at the ends of pipelines like `foo() %>% expect_equal(2)`
+     + Extended for #1067 to exclude `$` extractions like `expect_equal(x$"key", 2)`
    * `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
    * `if_else_match_braces_linter()` Require balanced usage of `{}` in `if`/`else` conditions

R/yoda_test_linter.R

@@ -12,7 +12,7 @@
 #' @export
 yoda_test_linter <- function() {
   Linter(function(source_file) {
-    if (length(source_file$parsed_content) == 0L) {
+    if (length(source_file$xml_parsed_content) == 0L) {
       return(list())
     }
 
@@ -21,23 +21,38 @@ yoda_test_linter <- function() {
     # 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]
+    #       (but _not_ x$"key", #1067)
     #   (3) arithmetic literal (e.g. 1+1 or 0+1i)    [OP-PLUS or OP-MINUS...]
     # TODO(#963): fully generalize this & re-use elsewhere
-    xpath <- "//expr[
+    const_condition <- "
+      NUM_CONST
+      or (STR_CONST and not(OP-DOLLAR))
+      or ((OP-PLUS or OP-MINUS) and count(expr[NUM_CONST]) = 2)
+    "
+    xpath <- glue::glue("//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)]
-    ]"
+      and expr[2][ {const_condition} ]
+      and not(preceding-sibling::*[self::PIPE or self::SPECIAL[text() = '%>%']])
+    ]")
 
     bad_expr <- xml2::xml_find_all(xml, xpath)
 
     return(lapply(
       bad_expr,
       xml_nodes_to_lint,
       source_file = source_file,
-      lint_message = paste(
-        "Tests should compare objects in the order 'actual', 'expected', not the reverse.",
-        "For example, do expect_identical(foo(x), 2L) instead of expect_identical(2L, foo(x))."
-      ),
+      lint_message = function(expr) {
+        matched_call <- xml2::xml_text(xml2::xml_find_first(expr, "expr/SYMBOL_FUNCTION_CALL"))
+        second_const <- xml2::xml_find_first(expr, glue::glue("expr[position() = 3 and ({const_condition})]"))
+        if (is.na(second_const)) {
+          paste(
+            "Tests should compare objects in the order 'actual', 'expected', not the reverse.",
+            sprintf("For example, do %1$s(foo(x), 2L) instead of %1$s(2L, foo(x)).", matched_call)
+          )
+        } else {
+          sprintf("Avoid storing placeholder tests like %s(1, 1)", matched_call)
+        }
+      },
       type = "warning"
     ))
   })

tests/testthat/test-yoda_test_linter.R

@@ -31,6 +31,26 @@ test_that("yoda_test_linter blocks simple disallowed usages", {
   )
 })
 
+test_that("yoda_test_linter ignores strings in $ expressions", {
+  # the "key" here shows up at the same level of the parse tree as plain "key" normally would
+  expect_lint('expect_equal(x$"key", 2)', NULL, yoda_test_linter())
+})
+
+# if we only inspect the first argument & ignore context, get false positives
+test_that("yoda_test_linter ignores usage in pipelines", {
+  expect_lint("foo() %>% expect_identical(2)", NULL, yoda_test_linter())
+  skip_if_not_installed("base", "4.1.0")
+  expect_lint("bar() |> expect_equal('a')", NULL, yoda_test_linter())
+})
+
+test_that("yoda_test_linter throws a special message for placeholder tests", {
+  expect_lint(
+    "expect_equal(1, 1)",
+    rex::rex("Avoid storing placeholder tests like expect_equal(1, 1)"),
+    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