Extend return_linter to allow prefix exclusions

[AshesITR]

What about adding rudimentary globbing support to except=? It could detect * at the end of exceptions and convert those entries to prefix-exclusions.

Originally posted by @AshesITR in #2271 (comment)

[MichaelChirico]

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.

[MichaelChirico]

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 <- ""
}

[MichaelChirico]

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).

[AshesITR]

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