Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve test coverage functionality #2537

Merged
merged 3 commits into from
Sep 29, 2023
Merged

Improve test coverage functionality #2537

merged 3 commits into from
Sep 29, 2023

Conversation

hadley
Copy link
Member

@hadley hadley commented Sep 27, 2023

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.

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
@hadley hadley requested a review from jennybc September 27, 2023 13:54
@hadley hadley changed the title Don't call local_test_directory() in test coverage Improve test coverage functionality Sep 27, 2023
@hadley
Copy link
Member Author

hadley commented Sep 27, 2023

@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
Copy link
Member

jennybc commented Sep 29, 2023

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

Copy link
Member

@jennybc jennybc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I think all is well here and in testthat, (but suspect there may be some buglet in covr).

@jennybc
Copy link
Member

jennybc commented Sep 29, 2023

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

@hadley hadley merged commit aebb4e3 into main Sep 29, 2023
13 checks passed
@hadley hadley deleted the test-coverage-settings branch September 29, 2023 12:06
@hadley
Copy link
Member Author

hadley commented Sep 29, 2023

🥳

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
Copy link
Member

jennybc commented Oct 7, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

find_edition() / find_description() may have too strong a preference for an installed package
2 participants