diff --git a/DESCRIPTION b/DESCRIPTION index eda686353..9e0e29d5d 100644 --- a/DESCRIPTION +++ b/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' diff --git a/NAMESPACE b/NAMESPACE index 0a5501456..057bf62d7 100644 --- a/NAMESPACE +++ b/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) diff --git a/NEWS.md b/NEWS.md index 898d05ffc..ae8694fa0 100644 --- a/NEWS.md +++ b/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) diff --git a/R/expect_identical_linter.R b/R/expect_identical_linter.R new file mode 100644 index 000000000..5eabbd8af --- /dev/null +++ b/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" + )) + }) +} diff --git a/inst/lintr/linters.csv b/inst/lintr/linters.csv index f4ced54cb..6bc68ebff 100644 --- a/inst/lintr/linters.csv +++ b/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 diff --git a/man/expect_identical_linter.Rd b/man/expect_identical_linter.Rd new file mode 100644 index 000000000..898a70bad --- /dev/null +++ b/man/expect_identical_linter.Rd @@ -0,0 +1,39 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/expect_identical_linter.R +\name{expect_identical_linter} +\alias{expect_identical_linter} +\title{Require usage of expect_identical(x, y) where appropriate} +\usage{ +expect_identical_linter() +} +\description{ +At Google, \code{\link[testthat:equality-expectations]{testthat::expect_identical()}} should be the default/go-to function for +comparing an output to an expected value. \code{expect_true(identical(x, y))} +is an equivalent but unadvised method of the same test. Further, +\code{\link[testthat:equality-expectations]{testthat::expect_equal()}} should only be used when \code{expect_identical()} +is inappropriate, i.e., when \code{x} and \code{y} need only be \emph{numerically +equivalent} instead of fully identical (in which case, provide the +\verb{tolerance=} argument to \code{expect_equal()} explicitly). This also applies +when it's inconvenient to check full equality (e.g., names can be ignored, +in which case \code{ignore_attr = "names"} should be supplied to +\code{expect_equal()} (or, for 2nd edition, \code{check.attributes = FALSE}). +} +\section{Exceptions}{ + + +The linter allows \code{expect_equal()} in three circumstances: +\enumerate{ +\item A named argument is set (e.g. \code{ignore_attr} or \code{tolerance}) +\item Comparison is made to an explicit decimal, e.g. +\code{expect_equal(x, 1.0)} (implicitly setting \code{tolerance}) +\item \code{...} is passed (wrapper functions whcih might set +arguments such as \code{ignore_attr} or \code{tolerance}) +} +} + +\seealso{ +\link{linters} for a complete list of linters available in lintr. +} +\section{Tags}{ +\link[=package_development_linters]{package_development} +} diff --git a/man/linters.Rd b/man/linters.Rd index 8b8dffe89..801a480e6 100644 --- a/man/linters.Rd +++ b/man/linters.Rd @@ -24,7 +24,7 @@ The following tags exist: \item{\link[=correctness_linters]{correctness} (7 linters)} \item{\link[=default_linters]{default} (25 linters)} \item{\link[=efficiency_linters]{efficiency} (4 linters)} -\item{\link[=package_development_linters]{package_development} (11 linters)} +\item{\link[=package_development_linters]{package_development} (12 linters)} \item{\link[=readability_linters]{readability} (24 linters)} \item{\link[=robustness_linters]{robustness} (10 linters)} \item{\link[=style_linters]{style} (31 linters)} @@ -43,6 +43,7 @@ The following linters exist: \item{\code{\link{duplicate_argument_linter}} (tags: common_mistakes, configurable, correctness)} \item{\code{\link{equals_na_linter}} (tags: common_mistakes, correctness, default, robustness)} \item{\code{\link{expect_comparison_linter}} (tags: best_practices, package_development)} +\item{\code{\link{expect_identical_linter}} (tags: package_development)} \item{\code{\link{expect_length_linter}} (tags: best_practices, package_development, readability)} \item{\code{\link{expect_named_linter}} (tags: best_practices, package_development, readability)} \item{\code{\link{expect_not_linter}} (tags: best_practices, package_development, readability)} diff --git a/man/package_development_linters.Rd b/man/package_development_linters.Rd index eee6b33c7..bbdb17f3f 100644 --- a/man/package_development_linters.Rd +++ b/man/package_development_linters.Rd @@ -14,6 +14,7 @@ The following linters are tagged with 'package_development': \itemize{ \item{\code{\link{backport_linter}}} \item{\code{\link{expect_comparison_linter}}} +\item{\code{\link{expect_identical_linter}}} \item{\code{\link{expect_length_linter}}} \item{\code{\link{expect_named_linter}}} \item{\code{\link{expect_not_linter}}} diff --git a/tests/testthat/test-expect_identical_linter.R b/tests/testthat/test-expect_identical_linter.R new file mode 100644 index 000000000..e157820e3 --- /dev/null +++ b/tests/testthat/test-expect_identical_linter.R @@ -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()) +})