tidyverse / magrittr

Improve the readability of R code with the pipe
https://magrittr.tidyverse.org
Other
957 stars 157 forks source link

Make testthat tests conditional on its presence. #245

Closed dmurdoch closed 2 years ago

dmurdoch commented 3 years ago

magrittr fails tests on CRAN in winUCRT because testthat is missing. testthat is a suggested package.

This creates a huge cascade of failures of packages that depend on magrittr.

magrittr should be able to pass tests on CRAN with no ERRORs even if suggested packages are missing.

This PR reduces the magrittr problems to a NOTE about testthat being missing.

lionel- commented 3 years ago

It seems to me that if testthat is not installed, running the tests should be an error. From that point of view, merging your PR would be a regression (I like that it at least emits a warning though). At the same time we don't want to add testthat to Imports since it's not a user dependency, that's only a dependency for the scripts in the test folder.

My conclusion is that we need a way to specify the deps of these test scripts in the DESCRIPTION file, which is currently not expressive enough.

dmurdoch commented 3 years ago

I think you'd get an issue from CRAN if you made testthat a hard dependency since it's not used in user code. So with current policy, it needs to be a soft dependency, and packages should test for it before using it.

I think declaring testthat as a hard dependency for testing wouldn't solve the problem, unless magrittr was allowed to be installed without running tests, since testthat depends on magrittr, and magrittr would then depend on testthat.

There may be a change to CRAN practices or DESCRIPTION fields that would solve this circular dependency problem, but I don't know it. My small PR creates a warning (not a note as in the original change), which will allow magrittr to be available to the thousands of packages that depend on it, including testthat.

lionel- commented 2 years ago

Closing this until we get a more general solution for test-time dependencies.