diff --git a/NEWS.md b/NEWS.md index 3cb8bd7a4..aa844a3c6 100644 --- a/NEWS.md +++ b/NEWS.md @@ -110,6 +110,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 diff --git a/R/yoda_test_linter.R b/R/yoda_test_linter.R index 69a3028b4..1c0b7da1c 100644 --- a/R/yoda_test_linter.R +++ b/R/yoda_test_linter.R @@ -21,12 +21,19 @@ 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) @@ -34,10 +41,18 @@ yoda_test_linter <- function() { 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" )) }) diff --git a/tests/testthat/test-yoda_test_linter.R b/tests/testthat/test-yoda_test_linter.R index 4ec1bedcf..abfbc3e6a 100644 --- a/tests/testthat/test-yoda_test_linter.R +++ b/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