checks for tests?

[maelle]

when reading hadley/r-pkgs#777 I was wondering whether there could be ckecks of

• naked code in test files
• library calls in test files
• source() calls in test files

[mpadge]

Some notes from my side:

• library calls are sometimes legitmately required, as are directly modifications of tests/testthat.R - use of httptest2 being a prime example. Given that, i don't think that pattern could be used in any general checking framework here.
• I personally have "naked code" in a lot of my test files, including occassionally in the middle, most of which implement my own testthat::skip...<whatever> logic to give me finer control than would be possible without this naked code. It is also common to have variable assignment statements for test-file-level "global" variables, and i imagine there are cases where any other approach would be less clear. Another example are the CRAN restrictions on parallel threads. I insert explicit "naked code" in packages to ensure those don't exceed the maximum of 2, and do not agree in contexts like these with the general recommendations in that chapter that alternative approaches which put these kinds of statements in on.exit or whatever are clearer. Having them as naked code, even in the middle of test files, is in my opinion the clearest way to indicate exactly at which point in the test sequence the calls are otherwise likely to exceed those CRAN limits.
• I also use source(), which i should probably not do, but note that advice agains this is only relevant in testthat environments, so a general check fail when source() is used is also an absolute requirement that testthat is used, and none of the other excellent alternatives such as tinytest. I would personally view that as overly prescriptive in any general context, but do agree that use of source() in packages which do use testthat could maybe be a useful check addition.

[mpadge]

Plus important to realise that many packages only partly use testthat, and mix those tests with other forms of tests, so actually even that last idea may not be generally feasible?

[maelle]

for httptest2 one actually modifies setup.R, not the testthat file?

[maelle]

I agree these checks are only for packages using testthat.

For naked code it could be a note. It's useful as it could be code that could be moved to a helper file, and even if not it'd be something interesting for editors/reviewers to notice as it's a tricky thing. Maybe same with library?

[maelle]

if a package uses a mix is it possible to isolate which test folder uses testthat?

[mpadge]

for httptest2 one actually modifies setup.R, not the testthat file?

No, the package requires an additional library call in tests/testthat.R.

For naked code it could be a note. It's useful as it could be code that could be moved to a helper file, and even if not it'd be something interesting for editors/reviewers to notice as it's a tricky thing. Maybe same with library?

I'm not sure though, because that really would then be complying with the subjective views of effectively one person only (Hadley). There are other very prominent developers and R-core members who do not view that as "best practice". So I'm not even sure we should be systematically/automatically pointing out any divergence from r-pkgs recommendations? That book codifies one approach, but requiring that to be the only approach may be unnecessarily restrictive, I think.

if a package uses a mix is it possible to isolate which test folder uses testthat?

In principle, yes. Even directly possibly to just use parse() on each test file and tease apart sections within single files. But I'd first have to be convinced on the above point.

[maelle]

but for testthat it might be fine to try and apply the recs of testthat authors?

If pkgcheck notified about these uses, it'd be interesting as their deviating from the "norm" means they are something relevant to look at while reviewing (compared to boring expectations in the tests).

We could revert if we realize all submissions have a legitimate "different" testthat use. 😅