Add options= to test(), convert most Rraw scripts

[MichaelChirico]

Closes #5842.

Didn't touch tests.Rraw yet as there are 200+ existing calls to options(). Better to save that for a future clean-up. Converted the other scripts with a more manageable diff to demonstrate the new usage.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.53%. Comparing base (566bff0) to head (50a6d17).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #5845   +/-   ##
=======================================
  Coverage   97.53%   97.53%           
=======================================
  Files          80       80           
  Lines       14915    14915           
=======================================
  Hits        14547    14547           
  Misses        368      368           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ben-schwen]

Looking great!

The only thing we might have to think about is where to put the options argument in a test(...) call. Current practice is AFAIC to have test(num, call, expected) so test(num, options=options, call, expected) differs from that.

[MichaelChirico]

Looking great!

The only thing we might have to think about is where to put the options argument in a test(...) call. Current practice is AFAIC to have test(num, call, expected) so test(num, options=options, call, expected) differs from that.

yes, this is a new style. basically this is a "set up" step as it might be called in other unit testing frameworks. so I think it's important to have it up front for clarity. that doesn't have a parallel among current arguments. I might even put it before the test number, but definitely not after the test outcome parts.

[ben-schwen]

yes, this is a new style. basically this is a "set up" step as it might be called in other unit testing frameworks. so I think it's important to have it up front for clarity. that doesn't have a parallel among current arguments. I might even put it before the test number, but definitely not after the test outcome parts.

I also thought about the setup step, so 2nd place makes definitely sense.

Pls, do not put it before the test number because in searching for tests I often use test(testnum if the test number occurs multiple times in tests.Rraw

[tdhock]

I would like to see a similar test(testnum, my_computed_value, my_expected_value, env=list(SOME_ENV_VAR="VALUE")) -- I was recently working on #5807 which involves environment variables for getting default number of threads, and the tests would be much easier to understand/modify with such an enhancement to test.

[jangorecki]

@tdhock I understand the use case, but if it is not as common as options, then I think it can be left to R's functions to set variables. Contrary R options are very common in tests, as Michael said, more than 200 times.

[MichaelChirico]

I would like to see a similar test(testnum, my_computed_value, my_expected_value, env=list(SOME_ENV_VAR="VALUE")) -- I was recently working on #5807 which involves environment variables for getting default number of threads, and the tests would be much easier to understand/modify with such an enhancement to test.

I think it's perfectly reasonable, but let's save it for a follow-up issue to keep PRs sharp

[MichaelChirico]

Here is a breakdown of the branches touching each test script as of now:

• inst/tests/benchmark.Rraw: benchmark is.valid
• inst/tests/froll.Rraw: adapt coerceAs frollapply frollmax frollmax2 frollmax3 frollmax4 frollmax5 frollmax7 frollmax8 frollmax9 fwrite_zlib_present join-retain-more-keys rollfuns rollmedian
• inst/tests/nafill.Rraw: assign_todos coerceAs coerceAs_bit64 colselect cutils2 fwrite_zlib_present is.valid nafill5
• inst/tests/other.Rraw: fwrite_zlib_present gforce_cumsum is.valid join-retain-more-keys last_narm rbind_class_att
• inst/tests/programming.Rraw: allow-env-dot-alias
• inst/tests/types.Rraw:

branch	benchmark.Rraw	froll.Rraw	nafill.Rraw	other.Rraw	programming.Rraw	types.Rraw
adapt		✅
allow-env-dot-alias					✅
assign_todos			✅
benchmark	✅
coerceAs		✅	✅
coerceAs_bit64			✅
colselect			✅
cutils2			✅
frollapply		✅
frollmax		✅
frollmax2		✅
frollmax3		✅
frollmax4		✅
frollmax5		✅
frollmax7		✅
frollmax8		✅
frollmax9		✅
fwrite_zlib_present		✅	✅	✅
gforce_cumsum				✅
is.valid	✅		✅	✅
join-retain-more-keys		✅		✅
last_narm				✅
nafill5			✅
rbind_class_att				✅
rollfuns		✅
rollmedian		✅

library(data.table)

tests = list.files(pattern = ".Rraw$", recursive = TRUE)
tests = setdiff(tests, "inst/tests/tests.Rraw")
branches_by_test = lapply(tests, function(test) {
  system2("git", c("log", "--all", "--format=%S", "--", test), stdout=TRUE) |>
    unique() |>
    sort() |>
    grep(pattern = "^refs/remotes/origin/", value=TRUE) |>
    gsub(pattern = "^refs/remotes/origin/", replacement = "") |>
    setdiff("test-options")
})

branch_link <- \(branch) paste0("[`", branch, "`](https://github.com/Rdatatable/data.table/tree/", branch, ")")

for (ii in seq_along(tests)) {
  branches = branches_by_test[[ii]]
  cat(sprintf(" - `%s`: %s\n", tests[ii], paste(branch_link(branches), collapse = " ")))
}

branch_data <- data.table(file = rep(tests, lengths(branches_by_test)), branch = unlist(branches_by_test)) |>
  dcast(branch ~ basename(file), fun.aggregate = \(x) if (length(x)) "✅" else "", value.var = "branch")
branch_data[, branch := branch_link(branch)]
knitr::kable(branch_data)

[MichaelChirico]

• Add options= to test(), convert most Rraw scripts #5845 👈
• Use options= in test() for rarely-touched scripts #6015
• master

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @MichaelChirico and the rest of your teammates on Graphite

[MichaelChirico]

This PR is waiting for other PRs to clear before we can merge. In particular here are the PRs to close/merge before we can merge each test file included here:

• froll.Rraw - adapt uneven time series rolling window #5576 frollapply rewritten, supports by.column=F #5575 rolling functions: adaptive left, frollmax, frollapply adaptive, partial #5441 frollmax2: code reorg, docs, tests #5890 frollmax3: partial arg #5891 frollmax4: frollapply adaptive #5892 frollmax5: NA support in frollmax #5893 frollmax7: arg rename, use macros, Inf support #5895 frollmax8: deduplicate args handling #5896 frollmax9: give.names argument #5897 more rolling functions #5682 rolling median #5692
• nafill.Rraw - is.valid #4480 nafill character, logical, etc. #4980
• other.Rraw - is.valid #4480 last/first get argument na.rm #5168 Rbind allow binding of different class attributes #5446