diff --git a/DESCRIPTION b/DESCRIPTION index 9e0e29d5d..d1e211bca 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -56,6 +56,7 @@ Collate: 'commas_linter.R' 'comment_linters.R' 'comments.R' + 'conjunct_expectation_linter.R' 'cyclocomp_linter.R' 'declared_functions.R' 'deprecated.R' diff --git a/NAMESPACE b/NAMESPACE index 057bf62d7..2192f7c64 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -21,6 +21,7 @@ export(clear_cache) export(closed_curly_linter) export(commas_linter) export(commented_code_linter) +export(conjunct_expectation_linter) export(cyclocomp_linter) export(default_linters) export(default_settings) diff --git a/NEWS.md b/NEWS.md index ae8694fa0..04d680fcb 100644 --- a/NEWS.md +++ b/NEWS.md @@ -90,6 +90,7 @@ function calls. (#850, #851, @renkun-ken) + `expect_type_linter()` Require usage of `expect_type(x, t)` over `expect_equal(typeof(x), t)` and similar + `expect_s3_class_linter()` Require usage of `expect_s3_class(x, k)` over `expect_equal(class(x), k)` and similar + `expect_s4_class_linter()` Require usage of `expect_s4_class(x, k)` over `expect_true(methods::is(x, k))` + + `conjunct_expectation_linter()` Require usage of `expect_true(x); expect_true(y)` over `expect_true(x && y)` and similar + `expect_not_linter()` Require usage of `expect_false(x)` over `expect_true(!x)`, and _vice versa_. + `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 diff --git a/R/conjunct_expectation_linter.R b/R/conjunct_expectation_linter.R new file mode 100644 index 000000000..222ea8dcd --- /dev/null +++ b/R/conjunct_expectation_linter.R @@ -0,0 +1,47 @@ +#' Force && conditions in expect_true(), expect_false() to be written separately +#' +#' For readability of test outputs, testing only one thing per call to +#' [testthat::expect_true()] is preferable, i.e., +#' `expect_true(A); expect_true(B)` is better than `expect_true(A && B)`, and +#' `expect_false(A); expect_false(B)` is better than `expect_false(A || B)`. +#' +#' @evalRd rd_tags("expect_true_false_and_condition_linter") +#' @seealso [linters] for a complete list of linters available in lintr. +#' @export +conjunct_expectation_linter <- function() { + Linter(function(source_file) { + # need the full file to also catch usages at the top level + if (length(source_file$full_parsed_content) == 0L) { + return(list()) + } + + xml <- source_file$full_xml_parsed_content + + xpath <- "//expr[ + ( + expr[SYMBOL_FUNCTION_CALL[text() = 'expect_true']] + and expr[2][AND2] + ) or ( + expr[SYMBOL_FUNCTION_CALL[text() = 'expect_false']] + and expr[2][OR2] + ) + ]" + + bad_expr <- xml2::xml_find_all(xml, xpath) + + return(lapply( + bad_expr, + xml_nodes_to_lint, + source_file, + lint_message = function(expr) { + matched_fun <- xml2::xml_text(xml2::xml_find_first(expr, "expr/SYMBOL_FUNCTION_CALL")) + op <- if (matched_fun == "expect_true") "&&" else "||" + message <- + sprintf("Instead of %1$s(A %2$s B), write multiple expectations like %1$s(A) and %1$s(B)", matched_fun, op) + paste(message, "The latter will produce better error messages in the case of failure.") + }, + type = "warning", + global = TRUE + )) + }) +} diff --git a/inst/lintr/linters.csv b/inst/lintr/linters.csv index 6bc68ebff..0600e6795 100644 --- a/inst/lintr/linters.csv +++ b/inst/lintr/linters.csv @@ -5,6 +5,7 @@ backport_linter,robustness configurable package_development closed_curly_linter,style readability default configurable commas_linter,style readability default commented_code_linter,style readability best_practices default +conjunct_expectation_linter,package_development best_practices readability cyclocomp_linter,style readability best_practices default configurable duplicate_argument_linter,correctness common_mistakes configurable equals_na_linter,robustness correctness common_mistakes default diff --git a/man/best_practices_linters.Rd b/man/best_practices_linters.Rd index cd9fd1e32..91b21d619 100644 --- a/man/best_practices_linters.Rd +++ b/man/best_practices_linters.Rd @@ -14,6 +14,7 @@ The following linters are tagged with 'best_practices': \itemize{ \item{\code{\link{absolute_path_linter}}} \item{\code{\link{commented_code_linter}}} +\item{\code{\link{conjunct_expectation_linter}}} \item{\code{\link{cyclocomp_linter}}} \item{\code{\link{expect_comparison_linter}}} \item{\code{\link{expect_length_linter}}} diff --git a/man/conjunct_expectation_linter.Rd b/man/conjunct_expectation_linter.Rd new file mode 100644 index 000000000..1125e999e --- /dev/null +++ b/man/conjunct_expectation_linter.Rd @@ -0,0 +1,20 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/conjunct_expectation_linter.R +\name{conjunct_expectation_linter} +\alias{conjunct_expectation_linter} +\title{Force && conditions in expect_true(), expect_false() to be written separately} +\usage{ +conjunct_expectation_linter() +} +\description{ +For readability of test outputs, testing only one thing per call to +\code{\link[testthat:logical-expectations]{testthat::expect_true()}} is preferable, i.e., +\verb{expect_true(A); expect_true(B)} is better than \code{expect_true(A && B)}, and +\verb{expect_false(A); expect_false(B)} is better than \code{expect_false(A || B)}. +} +\seealso{ +\link{linters} for a complete list of linters available in lintr. +} +\section{Tags}{ +No tags are given. +} diff --git a/man/linters.Rd b/man/linters.Rd index 801a480e6..843f5b04f 100644 --- a/man/linters.Rd +++ b/man/linters.Rd @@ -17,15 +17,15 @@ Documentation for linters is structured into tags to allow for easier discovery. \section{Tags}{ The following tags exist: \itemize{ -\item{\link[=best_practices_linters]{best_practices} (19 linters)} +\item{\link[=best_practices_linters]{best_practices} (20 linters)} \item{\link[=common_mistakes_linters]{common_mistakes} (5 linters)} \item{\link[=configurable_linters]{configurable} (16 linters)} \item{\link[=consistency_linters]{consistency} (7 linters)} \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} (12 linters)} -\item{\link[=readability_linters]{readability} (24 linters)} +\item{\link[=package_development_linters]{package_development} (13 linters)} +\item{\link[=readability_linters]{readability} (25 linters)} \item{\link[=robustness_linters]{robustness} (10 linters)} \item{\link[=style_linters]{style} (31 linters)} } @@ -39,6 +39,7 @@ The following linters exist: \item{\code{\link{closed_curly_linter}} (tags: configurable, default, readability, style)} \item{\code{\link{commas_linter}} (tags: default, readability, style)} \item{\code{\link{commented_code_linter}} (tags: best_practices, default, readability, style)} +\item{\code{\link{conjunct_expectation_linter}} (tags: best_practices, package_development, readability)} \item{\code{\link{cyclocomp_linter}} (tags: best_practices, configurable, default, readability, style)} \item{\code{\link{duplicate_argument_linter}} (tags: common_mistakes, configurable, correctness)} \item{\code{\link{equals_na_linter}} (tags: common_mistakes, correctness, default, robustness)} diff --git a/man/package_development_linters.Rd b/man/package_development_linters.Rd index bbdb17f3f..9a9ff41e2 100644 --- a/man/package_development_linters.Rd +++ b/man/package_development_linters.Rd @@ -13,6 +13,7 @@ Linters useful to package developers, for example for writing consistent tests. The following linters are tagged with 'package_development': \itemize{ \item{\code{\link{backport_linter}}} +\item{\code{\link{conjunct_expectation_linter}}} \item{\code{\link{expect_comparison_linter}}} \item{\code{\link{expect_identical_linter}}} \item{\code{\link{expect_length_linter}}} diff --git a/man/readability_linters.Rd b/man/readability_linters.Rd index 2236178af..a8beec5f2 100644 --- a/man/readability_linters.Rd +++ b/man/readability_linters.Rd @@ -15,6 +15,7 @@ The following linters are tagged with 'readability': \item{\code{\link{closed_curly_linter}}} \item{\code{\link{commas_linter}}} \item{\code{\link{commented_code_linter}}} +\item{\code{\link{conjunct_expectation_linter}}} \item{\code{\link{cyclocomp_linter}}} \item{\code{\link{expect_length_linter}}} \item{\code{\link{expect_named_linter}}} diff --git a/tests/testthat/test-conjunct_expectation_linter.R b/tests/testthat/test-conjunct_expectation_linter.R new file mode 100644 index 000000000..e14cc4bec --- /dev/null +++ b/tests/testthat/test-conjunct_expectation_linter.R @@ -0,0 +1,59 @@ +test_that("conjunct_expectation_linter skips allowed usages of expect_true", { + expect_lint("expect_true(x)", NULL, conjunct_expectation_linter()) + expect_lint("testthat::expect_true(x, y, z)", NULL, conjunct_expectation_linter()) + + # more complicated expression + expect_lint("expect_true(x || (y && z))", NULL, conjunct_expectation_linter()) + # the same by operator precedence, though not obvious a priori + expect_lint("expect_true(x || y && z)", NULL, conjunct_expectation_linter()) + expect_lint("expect_true(x && y || z)", NULL, conjunct_expectation_linter()) +}) + +test_that("conjunct_expectation_linter skips allowed usages of expect_true", { + expect_lint("expect_false(x)", NULL, conjunct_expectation_linter()) + expect_lint("testthat::expect_false(x, y, z)", NULL, conjunct_expectation_linter()) + + # more complicated expression + # (NB: xx && yy || zz and xx || yy && zz both parse with || first) + expect_lint("expect_false(x && (y || z))", NULL, conjunct_expectation_linter()) +}) + +test_that("conjunct_expectation_linter blocks && conditions with expect_true()", { + expect_lint( + "expect_true(x && y)", + rex::rex("Instead of expect_true(A && B), write multiple expectations"), + conjunct_expectation_linter() + ) + + expect_lint( + "expect_true(x && y && z)", + rex::rex("Instead of expect_true(A && B), write multiple expectations"), + conjunct_expectation_linter() + ) +}) + +test_that("conjunct_expectation_linter blocks || conditions with expect_false()", { + expect_lint( + "expect_false(x || y)", + rex::rex("Instead of expect_false(A || B), write multiple expectations"), + conjunct_expectation_linter() + ) + + expect_lint( + "expect_false(x || y || z)", + rex::rex("Instead of expect_false(A || B), write multiple expectations"), + conjunct_expectation_linter() + ) + + # these lint because `||` is always outer by operator precedence + expect_lint( + "expect_false(x || y && z)", + rex::rex("Instead of expect_false(A || B), write multiple expectations"), + conjunct_expectation_linter() + ) + expect_lint( + "expect_false(x && y || z)", + rex::rex("Instead of expect_false(A || B), write multiple expectations"), + conjunct_expectation_linter() + ) +})