Extend return_linter to allow prefix exclusions #2335

Activity

1. MichaelChirico commented on Nov 22, 2023

   

   MichaelChirico

   on Nov 22, 2023

   Last edited by MichaelChirico

   Collaborator

   More actions

   It would be nice for RUnit exclusions to supply except = c("Test*", "check*")), instead of having to remember/list out all the RUnit::check* functions. As well as supporting custom checkers, e.g. I've seen a checkDataFrame() used as a helper in an RUnit suite.

   But check* is probably too broad (likely to include non-RUnit functions). The more general glob check[A-Z]* probably works, but leaves me wondering if there's any disadvantage to supporting regex more generally.
2. MichaelChirico commented on Nov 23, 2023

   

   MichaelChirico

   on Nov 23, 2023

   Collaborator

   More actions

   Here are the tests we dropped from the initial PR for return_linter() #2271:

   test_that("return_linter passes on .setUp/.tearDown calls", {
     linter <- return_linter(use_implicit_returns = FALSE, use_runit = TRUE)
   
     setup_lines <- c(
       ".setUp <- function() {",
       "  options(foo = TRUE)",
       "}"
     )
     expect_lint(setup_lines, NULL, linter)
   
     teardown_lines <- c(
       ".tearDown <- function() {",
       "  options(foo = TRUE)",
       "}"
     )
     expect_lint(teardown_lines, NULL, linter)
   })
   
   test_that("return_linter allows RUnit tests to pass", {
     linter <- return_linter(use_implicit_returns = FALSE, use_runit = TRUE)
   
     lines <- c(
       "TestKpSxsSummary <- function() {",
       "  context <- foo(72643424)",
       "  expected <- data.frame(a = 2)",
       "  checkEquals(expected, bar(context))",
       "}"
     )
     expect_lint(lines, NULL, linter)
   
     custom_lines <- c(
       "TestMyPackage <- function() {",
       "  checkMyCustomComparator(x, y)",
       "}"
     )
     expect_lint(custom_lines, NULL, linter)
   })
   
   test_that("return_linter skips RUnit functions in argumented tests", {
     lines <- c(
       "TestSummary <- function(an_argument) {",
       "  context <- foo(an_argument)",
       "  expected <- data.frame(a = 2)",
       "  checkEquals(expected, bar(context))",
       "}"
     )
     expect_lint(lines, NULL, return_linter(use_implicit_returns = FALSE, use_runit = TRUE))
   })
   

   And here's the implementation that is dropped for now:

   if (use_runit) {
   
     side_effect_functions <- union(side_effect_functions, c(".setUp", ".tearDown"))
   
     # tests in the RUnit framework are functions ending with a call to one
     #   of the below. would rather users just use a different framework
     #   (e.g. testthat or tinytest), but already 250+ BUILD files depend
     #   on RUnit, so just cater to that. confirmed the efficiency impact
     #   of including these is minimal.
     # RUnit tests look like 'TestInCamelCase <- function()'
     #   NB: check for starts-with(text(), 'Test') below is not sufficient, e.g.
     #   in cases of a "driver" test function taking arguments and the main unit
     #   test iterating over those.
     allowed_functions <- union(
       allowed_functions,
       c(
         "checkEquals", "checkEqualsNumeric", "checkException", "checkIdentical",
         "checkStop", "checkTrue", "checkWarnings"
       )
     )
   
     ignore_start <- "
     or (
       preceding-sibling::expr/SYMBOL[starts-with(text(), 'Test')]
       and not(SYMBOL_FORMALS)
     )
     "
   } else {
     ignore_start <- ""
   }
3. MichaelChirico

   added this to the 3.1.2 milestone on Nov 23, 2023
4. MichaelChirico

   added

   featurea feature request or enhancementa feature request or enhancement

   on Nov 28, 2023
5. MichaelChirico commented on Dec 6, 2023

   

   MichaelChirico

   on Dec 6, 2023

   Collaborator

   More actions

   WDYT about return_linter(except = "regex:Test.*")? Just looking for make.names(x) != x seems too limiting for except=, but a "real" function matching regex:.*[*] seems highly unlikely.

   Alternatively, we could support a list input: return_linter(except = list(<exact matches>, regex = <regex matches>, glob = <glob matches>)), where the name exact = is optional for the first input.

   The more general glob check[A-Z]* probably works, but leaves me wondering if there's any disadvantage to supporting regex more generally.

   OTOH, the downside is it's hard to pass such a check through to XPath since XPath1.0 doesn't support any regex engine. So maybe it's best to leave this down to glob matching, and convert the glob to either starts-with or an ends-with equivalent (since neither is ends-with() supported in XPath1.0).
6. AshesITR commented on Dec 6, 2023

   

   AshesITR

   on Dec 6, 2023

   CollaboratorAuthor

   More actions

   Regarding pushdown to XPath I wouldn't worry too much, it might be worthwhile to capture and cache assignment exprs, too.
7. AshesITR

   closed this as completedin #2433on Dec 15, 2023