Unnecessary skip_if_not_installed() calls?

[etiennebacher]

Sometimes, test_that() calls contain skip_if_not_installed(), which is good practice. However, some of those calls are redundant because testthat already imports those packages, so if testthat runs then we know that this package is necessarily installed.

One example I've seen (and done myself) is skip_if_not_installed("withr"), which is redundant since testthat imports withr. There could be other cases, such as brio and jsonlite. Would it make sense to have a linter to detect those unnecessary skip_if_not_installed()?

[MichaelChirico]

I think this can go either way though -- {testthat} currently imports {with}, but what if it eventually changes to Suggests, or even refactors to use {withr2}, or the functionality is absorbed to {base}, etc?

So while I understand not using skip_if_not_installed("withr") for the reason stated, I also understand doing so as a future-proofing device.

This gets all the more complicated when considering recursive inclusions of packages -- should skip_if_not_installed("diffobj") pass (because it's required by {waldo})? These links are even more fragile & subject to unexpected breakage.

[etiennebacher]

Right, I guess there's not much harm in ignoring the existing skip_if_not_installed("withr") and it makes things more future proof indeed. Not sure if this should be closed then.