From 05cd63f1b9f183e541e6829ea853b340b1a7c8f6 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Thu, 7 Dec 2023 10:59:30 +0800 Subject: [PATCH] first pass done --- tests/testthat/test-lengths_linter.R | 2 +- .../test-routine_registration_linter.R | 6 ++- tests/testthat/test-scalar_in_linter.R | 13 +---- tests/testthat/test-sort_linter.R | 8 +-- .../test-spaces_left_parentheses_linter.R | 6 ++- tests/testthat/test-sprintf_linter.R | 10 ++-- .../testthat/test-strings_as_factors_linter.R | 6 ++- tests/testthat/test-system_file_linter.R | 26 +++++----- .../testthat/test-unnecessary_lambda_linter.R | 10 ++-- .../test-unnecessary_nesting_linter.R | 51 ++++++++++++------- .../test-unnecessary_placeholder_linter.R | 6 ++- tests/testthat/test-unused_import_linter.R | 4 +- tests/testthat/test-vector_logic_linter.R | 8 +-- tests/testthat/test-yoda_test_linter.R | 8 +-- 14 files changed, 101 insertions(+), 63 deletions(-) diff --git a/tests/testthat/test-lengths_linter.R b/tests/testthat/test-lengths_linter.R index ce1a40645..fbca36bd8 100644 --- a/tests/testthat/test-lengths_linter.R +++ b/tests/testthat/test-lengths_linter.R @@ -50,7 +50,7 @@ test_that("lints vectorize", { expect_lint( trim_some("{ sapply(x, length) - ma_int(x, length) + map_int(x, length) }"), list( list(lint_msg, line_number = 2L), diff --git a/tests/testthat/test-routine_registration_linter.R b/tests/testthat/test-routine_registration_linter.R index e9c402f03..e0bdafefa 100644 --- a/tests/testthat/test-routine_registration_linter.R +++ b/tests/testthat/test-routine_registration_linter.R @@ -14,13 +14,17 @@ patrick::with_parameters_test_that( ) test_that("lints vectorize", { + lint_msg <- "Register your native code routines with useDynLib" + expect_lint( trim_some("{ + .C('ROUTINE', PACKAGE = 'foo') + .External('POUTINE', PACKAGE = 'bar') }"), list( list(lint_msg, line_number = 2L), list(lint_msg, line_number = 3L) ), - linter + routine_registration_linter() ) }) diff --git a/tests/testthat/test-scalar_in_linter.R b/tests/testthat/test-scalar_in_linter.R index d428b20c7..2bfd66f83 100644 --- a/tests/testthat/test-scalar_in_linter.R +++ b/tests/testthat/test-scalar_in_linter.R @@ -31,18 +31,9 @@ test_that("multiple lints are generated correctly", { x %in% 1 y %chin% "a" }'), - list("%in%", "%chin%"), - linter - ) -}) - -test_that("lints vectorize", { - expect_lint( - trim_some("{ - }"), list( - list(lint_msg, line_number = 2L), - list(lint_msg, line_number = 3L) + list("%in%", line_number = 2L), + list("%chin%", line_number = 3L) ), linter ) diff --git a/tests/testthat/test-sort_linter.R b/tests/testthat/test-sort_linter.R index 3f25bbeb2..765743106 100644 --- a/tests/testthat/test-sort_linter.R +++ b/tests/testthat/test-sort_linter.R @@ -122,11 +122,13 @@ test_that("sort_linter blocks simple disallowed usages", { test_that("lints vectorize", { expect_lint( trim_some("{ + x == sort(x) + x != sort(x) }"), list( - list(lint_msg, line_number = 2L), - list(lint_msg, line_number = 3L) + list(rex::rex("is.unsorted(x)"), line_number = 2L), + list(rex::rex("!is.unsorted(x)"), line_number = 3L) ), - linter + sort_linter() ) }) diff --git a/tests/testthat/test-spaces_left_parentheses_linter.R b/tests/testthat/test-spaces_left_parentheses_linter.R index 65597eac7..ce854828c 100644 --- a/tests/testthat/test-spaces_left_parentheses_linter.R +++ b/tests/testthat/test-spaces_left_parentheses_linter.R @@ -97,13 +97,17 @@ test_that("doesn't produce a warning", { }) test_that("lints vectorize", { + lint_msg <- rex::rex("Place a space before left parenthesis, except in a function call.") + expect_lint( trim_some("{ + y1<-(abs(yn)>90)*1 + for(i in j) { } }"), list( list(lint_msg, line_number = 2L), list(lint_msg, line_number = 3L) ), - linter + spaces_left_parentheses_linter() ) }) diff --git a/tests/testthat/test-sprintf_linter.R b/tests/testthat/test-sprintf_linter.R index ce2a0c86d..1896db824 100644 --- a/tests/testthat/test-sprintf_linter.R +++ b/tests/testthat/test-sprintf_linter.R @@ -133,13 +133,17 @@ local({ }) test_that("lints vectorize", { + skip_if_not_r_version("4.1.0") + expect_lint( trim_some("{ + sprintf('%s', a, b) + sprintf('%s%s', a) }"), list( - list(lint_msg, line_number = 2L), - list(lint_msg, line_number = 3L) + list("one argument not used by format", line_number = 2L), + list("too few arguments", line_number = 3L) ), - linter + linter <- sprintf_linter() ) }) diff --git a/tests/testthat/test-strings_as_factors_linter.R b/tests/testthat/test-strings_as_factors_linter.R index 5b2cc6453..a45624b80 100644 --- a/tests/testthat/test-strings_as_factors_linter.R +++ b/tests/testthat/test-strings_as_factors_linter.R @@ -78,13 +78,17 @@ test_that("strings_as_factors_linter catches more functions with string output", }) test_that("lints vectorize", { + lint_msg <- "Supply an explicit value for stringsAsFactors for this code" + expect_lint( trim_some("{ + data.frame('a') + data.frame('b') }"), list( list(lint_msg, line_number = 2L), list(lint_msg, line_number = 3L) ), - linter + strings_as_factors_linter() ) }) diff --git a/tests/testthat/test-system_file_linter.R b/tests/testthat/test-system_file_linter.R index ef773f3bc..431a454f4 100644 --- a/tests/testthat/test-system_file_linter.R +++ b/tests/testthat/test-system_file_linter.R @@ -1,30 +1,30 @@ test_that("system_file_linter skips allowed usages", { - expect_lint("system.file('a', 'b', 'c')", NULL, system_file_linter()) - expect_lint("file.path('a', 'b', 'c')", NULL, system_file_linter()) + linter <- system_file_linter() + + expect_lint("system.file('a', 'b', 'c')", NULL, linter) + expect_lint("file.path('a', 'b', 'c')", NULL, linter) }) test_that("system_file_linter blocks simple disallowed usages", { - expect_lint( - "system.file(file.path('path', 'to', 'data'), package = 'foo')", - rex::rex("Use the `...` argument of system.file() to expand paths"), - system_file_linter() - ) + linter <- system_file_linter() + lint_msg <- rex::rex("Use the `...` argument of system.file() to expand paths") - expect_lint( - "file.path(system.file(package = 'foo'), 'path', 'to', 'data')", - rex::rex("Use the `...` argument of system.file() to expand paths"), - system_file_linter() - ) + expect_lint("system.file(file.path('path', 'to', 'data'), package = 'foo')", lint_msg, linter) + expect_lint("file.path(system.file(package = 'foo'), 'path', 'to', 'data')", lint_msg, linter) }) test_that("lints vectorize", { + lint_msg <- rex::rex("Use the `...` argument of system.file() to expand paths") + expect_lint( trim_some("{ + file.path(system.file(package = 'foo'), 'bar') + system.file(file.path('bar', data'), package = 'foo') }"), list( list(lint_msg, line_number = 2L), list(lint_msg, line_number = 3L) ), - linter + system_file_linter() ) }) diff --git a/tests/testthat/test-unnecessary_lambda_linter.R b/tests/testthat/test-unnecessary_lambda_linter.R index 3714943d9..2d87e67e1 100644 --- a/tests/testthat/test-unnecessary_lambda_linter.R +++ b/tests/testthat/test-unnecessary_lambda_linter.R @@ -265,11 +265,15 @@ test_that("function shorthand is handled", { test_that("lints vectorize", { expect_lint( trim_some("{ + sapply(x, function(xi) sd(xi)) + lapply(y, function(yi) { + sum(yi) + }) }"), list( - list(lint_msg, line_number = 2L), - list(lint_msg, line_number = 3L) + list("sd", line_number = 2L), + list("sum", line_number = 3L) ), - linter + unnecessary_lambda_linter() ) }) diff --git a/tests/testthat/test-unnecessary_nesting_linter.R b/tests/testthat/test-unnecessary_nesting_linter.R index 1b78a0836..076ba2d05 100644 --- a/tests/testthat/test-unnecessary_nesting_linter.R +++ b/tests/testthat/test-unnecessary_nesting_linter.R @@ -2,23 +2,29 @@ test_that("unnecessary_nesting_linter skips allowed usages", { linter <- unnecessary_nesting_linter() # parallel stops() and return()s are OK - double_stop_lines <- c( - "if (A) {", - " stop()", - "} else {", - " stop()", - "}" + expect_lint( + trim_some(" + if (A) { + stop() + } else { + stop() + } + "), + NULL, + linter ) - expect_lint(double_stop_lines, NULL, linter) - double_return_lines <- c( - "if (A) {", - " return()", - "} else {", - " return()", - "}" + expect_lint( + trim_some(" + if (A) { + return() + } else { + return() + } + "), + NULL, + linter ) - expect_lint(double_return_lines, NULL, linter) }) # TODO(michaelchirico): consider if there's a nice easy pattern to enforce for @@ -317,13 +323,24 @@ test_that("unnecessary_nesting_linter allow_assignment= argument works", { }) test_that("lints vectorize", { + lint_msg <- rex::rex("Reduce the nesting of this if/else") + expect_lint( - trim_some("{ - }"), + trim_some(" + if (A) { + stop('no') + } else { + if (B) { + stop('really no') + } else { + 1 + } + } + "), list( list(lint_msg, line_number = 2L), list(lint_msg, line_number = 3L) ), - linter + unnecessary_nesting_linter() ) }) diff --git a/tests/testthat/test-unnecessary_placeholder_linter.R b/tests/testthat/test-unnecessary_placeholder_linter.R index 63285ce00..d8a1e677a 100644 --- a/tests/testthat/test-unnecessary_placeholder_linter.R +++ b/tests/testthat/test-unnecessary_placeholder_linter.R @@ -39,13 +39,17 @@ patrick::with_parameters_test_that( ) test_that("lints vectorize", { + lint_msg <- rex::rex("Don't use the placeholder (`.`) when it's not needed") + expect_lint( trim_some("{ + x %>% foo(.) + y %T>% bar(.) }"), list( list(lint_msg, line_number = 2L), list(lint_msg, line_number = 3L) ), - linter + unnecessary_placeholder_linter() ) }) diff --git a/tests/testthat/test-unused_import_linter.R b/tests/testthat/test-unused_import_linter.R index 1c66ae99c..92aa277dd 100644 --- a/tests/testthat/test-unused_import_linter.R +++ b/tests/testthat/test-unused_import_linter.R @@ -42,8 +42,8 @@ test_that("unused_import_linter handles message vectorization", { xmlparsedata::xml_parse_data(parse(text = 'a')) "), list( - rex::rex("Package 'crayon' is attached but never used."), - rex::rex("Don't attach package 'xmlparsedata', which is only used by namespace") + list(rex::rex("Package 'crayon' is attached but never used."), line_number = 1L), + list(rex::rex("Don't attach package 'xmlparsedata', which is only used by namespace"), line_number = 2L) ), unused_import_linter() ) diff --git a/tests/testthat/test-vector_logic_linter.R b/tests/testthat/test-vector_logic_linter.R index ff237c003..e7d24db86 100644 --- a/tests/testthat/test-vector_logic_linter.R +++ b/tests/testthat/test-vector_logic_linter.R @@ -103,11 +103,13 @@ test_that("vector_logic_linter recognizes some false positves around bitwise &/| test_that("lints vectorize", { expect_lint( trim_some("{ + if (AA & BB) {} + if (CC | DD) {} }"), list( - list(lint_msg, line_number = 2L), - list(lint_msg, line_number = 3L) + list(rex::rex("`&&`"), line_number = 2L), + list(rex::rex("`||`"), line_number = 3L) ), - linter + vector_logic_linter() ) }) diff --git a/tests/testthat/test-yoda_test_linter.R b/tests/testthat/test-yoda_test_linter.R index 1b442903d..88ce1909e 100644 --- a/tests/testthat/test-yoda_test_linter.R +++ b/tests/testthat/test-yoda_test_linter.R @@ -51,12 +51,14 @@ test_that("yoda_test_linter throws a special message for placeholder tests", { test_that("lints vectorize", { expect_lint( trim_some("{ + expect_equal(1, 1) + expect_equal(2, foo(x)) }"), list( - list(lint_msg, line_number = 2L), - list(lint_msg, line_number = 3L) + list("Avoid storing placeholder tests", line_number = 2L), + list("Compare objects in tests in the order 'actual', 'expected'", line_number = 3L) ), - linter + yoda_test_linter() ) })