r-lib / devtools

Tools to make an R developer's life easier
https://devtools.r-lib.org
Other
2.37k stars 755 forks source link

Improve test coverage functionality #2537

Closed hadley closed 9 months ago

hadley commented 9 months ago

This was never used correctly because we're passing the path to the package root, instead of the test directory. Additionally, based on https://github.com/hadley/testcoverage, they don't actually appear to be needed.

Fixes r-lib/testthat#1858


@jennybc my main scepticism here for the case of test_coverage_active_file(), I can't see how this can possibly work (even though it does). As far as I can tell, nothing in test_coverage_active_file() causes either TESTTHAT_PKG or the edition to be set correctly, but the tests still pass.

...

Ooooh maybe the problem is that test_coverage_active_file() never actually reveals when the test fails.

...

Yes, that was the problem, and once I fixed that, it revealed that the snapshot paths weren't set up correctly, and it helped me understand why local_test_directory() is needed here, and why it works. I've added comments so hopefully this is less of a journey in the future.

hadley commented 9 months ago

@jennybc could you please interactively kick the tires of this PR with https://github.com/hadley/testcoverage and convince yourself that it all works as expected?

jennybc commented 9 months ago

Yes, I feel like my tire-kicking of https://github.com/hadley/testcoverage has been successful.

But as I've used test_coverage_active_file(), which apparently I don't do much of in real life, I see this warning, which is coming from covr, I think. I suppose this is a side observation, as I don't think it has anything to do with recent changes in testthat or in this PR.

> test_coverage_active_file()
Warning message:
In mapply(FUN = f, ..., SIMPLIFY = FALSE) :
  longer argument not a multiple of length of shorter
jennybc commented 9 months ago

I made some other updates on main to eliminate some nuisance noise in R CMD check, so I merged that in here.

hadley commented 9 months ago

🥳

Do you have a reprex for the recycling warning? I know I saw it multiple times in the workshop, but now I don't.

jennybc commented 9 months ago

I am also not seeing the mapply() warning any more 😐