Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle comparison to negative non-integer in expect_identical_linter #2411

Merged
merged 14 commits into from
Jun 8, 2024
1 change: 1 addition & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

## Bug fixes

* `expect_identical_linter()` also skips `expect_equal()` comparison to _negative_ non-integers like `-1.034` (#2411, @Bisaloo). This is a parity fix since _positive_ reals have always been skipped because "high-precision" comparisons are typically done to get tests within `tolerance`, so `expect_identical()` is not a great substitution.
* `object_name_linter()` no longer errors when user-supplied `regexes=` have capture groups (#2188, @MichaelChirico).
* `.lintr` config validation correctly accepts regular expressions which only compile under `perl = TRUE` (#2375, @MichaelChirico). These have always been valid (since `rex::re_matches()`, which powers the lint exclusion logic, also uses this setting), but the new up-front validation in v3.1.1 incorrectly used `perl = FALSE`.
* `.lintr` configs set by option `lintr.linter_file` or environment variable `R_LINTR_LINTER_FILE` can point to subdirectories (#2512, @MichaelChirico).
Expand Down
32 changes: 26 additions & 6 deletions R/expect_identical_linter.R
Original file line number Diff line number Diff line change
Expand Up @@ -60,18 +60,38 @@ expect_identical_linter <- function() {
# - 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
expect_equal_xpath <- "
non_integer <- glue::glue("
NUM_CONST[contains(text(), '.')]
or (
OP-MINUS
and count(expr) = 1
and expr[NUM_CONST[contains(text(), '.')]]
)
")

expect_equal_xpath <- glue::glue("
parent::expr[not(
following-sibling::EQ_SUB
or following-sibling::expr[
expr[1][SYMBOL_FUNCTION_CALL[text() = 'c']]
and expr[NUM_CONST[contains(text(), '.')]]
(
expr[1][SYMBOL_FUNCTION_CALL[text() = 'c']]
and expr[{non_integer}]
) or (
{non_integer}
) or (
OP-MINUS
and count(expr) = 1
and expr[
expr[1][SYMBOL_FUNCTION_CALL[text() = 'c']]
and expr[{non_integer}]
AshesITR marked this conversation as resolved.
Show resolved Hide resolved
]
) or (
SYMBOL[text() = '...']
)
]
or following-sibling::expr[NUM_CONST[contains(text(), '.')]]
or following-sibling::expr[SYMBOL[text() = '...']]
)]
/parent::expr
"
")
expect_true_xpath <- "
parent::expr
/following-sibling::expr[1][expr[1]/SYMBOL_FUNCTION_CALL[text() = 'identical']]
Expand Down
7 changes: 7 additions & 0 deletions tests/testthat/test-expect_identical_linter.R
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@ test_that("expect_identical_linter skips cases likely testing numeric equality",
lint_msg <- rex::rex("Use expect_identical(x, y) by default; resort to expect_equal() only when needed")

expect_lint("expect_equal(x, 1.034)", NULL, linter)
expect_lint("expect_equal(x, -1.034)", NULL, linter)
expect_lint("expect_equal(x, c(-1.034))", NULL, linter)
expect_lint("expect_equal(x, -c(1.034))", NULL, linter)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this case relevant in practice?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm on the fence.

On the one hand, it should be caught be unnecessary_concatenation_linter() and converted to the standard case. On the other hand, it's not just a false negative; not treating this case would lead us to produce incorrect advice.


expect_lint("expect_equal(x, c(1.01, 1.02))", NULL, linter)
# whole numbers with explicit decimals are OK, even in mixed scenarios
expect_lint("expect_equal(x, c(1.0, 2))", NULL, linter)
Expand All @@ -39,6 +43,9 @@ test_that("expect_identical_linter skips cases likely testing numeric equality",
# 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)", lint_msg, linter)

expect_lint("expect_equal(x, 1.01 - y)", lint_msg, linter)
expect_lint("expect_equal(x, foo() - 0.01)", lint_msg, linter)
})

test_that("expect_identical_linter skips 3e cases needing expect_equal", {
Expand Down
Loading