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 non-draft PRs touching each test script as of now:

• inst/tests/tests.Rraw: fread: consider quoted na.strings in text columns #7068, Clarify x/i error prefixes in merge() for incompatible factor joins #7048, Fix: Ensure column widths are data-driven when col.names = "none" in print.data.table  #6916, groupingsets(): introduce and make use of the enclos argument #6899, Fixed nested ':=' reference assignment fails #6789, add support for a list of selection expressions in .SDcols #6624, Add warning for joins on mixed time classes #6610, Return File Path after Writing in fwrite #6181, all.equal.data.table gets ignore.indices argument #6136, Forder segfault #6111, implement frev as fast base::rev alternative #5907, rolling functions: adaptive left, frollmax, frollapply adaptive, partial #5441, Fix incorrect keying after merge of keyed, non-alphabetic factor and character columns #5362, set automatically allocates new column slots if needed #5269, last/first get argument na.rm #5168, Topn doing order |> head/tail #5167, allow.assign.inplace attribute #4978, Add trunc.IDate #4868, export uniq function (uniqlist) #4372, Both columns for rolling and non-equi joins #3093
• inst/tests/froll.Rraw: frollmax10: deduplicate, support for adaptive&&partial #5898, frollmax9: give.names argument #5897, frollmax8: deduplicate args handling #5896, frollmax7: arg rename, use macros, Inf support #5895, frollmax5: NA support in frollmax #5893, frollmax4: frollapply adaptive #5892, frollmax3: partial arg #5891, frollmax2: code reorg, docs, tests #5890, rolling median #5692, more rolling functions #5682, adapt uneven time series rolling window #5576, frollapply rewritten, supports by.column=F #5575, rolling functions: adaptive left, frollmax, frollapply adaptive, partial #5441
• inst/tests/benchmark.Rraw: rolling functions: adaptive left, frollmax, frollapply adaptive, partial #5441
• inst/tests/other.Rraw: rolling functions: adaptive left, frollmax, frollapply adaptive, partial #5441, last/first get argument na.rm #5168, cbindlist, mergelist #4370
• inst/tests/mergelist.Rraw cbindlist, mergelist #4370

[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