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

find_edition() / find_description() may have too strong a preference for an installed package #1858

Closed
jennybc opened this issue Sep 17, 2023 · 4 comments · Fixed by r-lib/devtools#2537
Labels
3e 📚 bug an unexpected problem or unintended behavior
Milestone

Comments

@jennybc
Copy link
Member

jennybc commented Sep 17, 2023

This is a discovery made during the package development masterclass as posit conf.

We asked participants to check the test coverage for hadley/stringb and there would be no reason for anyone to have installed that package. And devtools::test_coverage() fails in this case.

> devtools:::test_coverage()
ℹ Computing test coverage for stringb
Error in idesc_create_package(self, private, package) : 
  Cannot find DESCRIPTION for installed package stringb

It goes like this: devtools::test_coverage() ➡️ testthat::local_test_directory(pkg$path, pkg$package) ➡️ find_edition(path, package) ➡️ find_description(path, package):

https://github.com/r-lib/testthat/blob/29018e067f87b07805e55178f387d2a04ff8311f/R/edition.R#L14C1-L23

I think find_description() is too eager to consult an installed package. It seems to assume that's safe and preferred if you happen to know the package's name.

I was able to "fix" the devtools problem by not passing the package name, but I don't think that's a great solution (r-lib/devtools#2536). That has other downsides, e.g., the package name won't be available for setting the TESTTHAT_PKG environment variable.

TESTTHAT_PKG = package,

@hadley hadley added this to the v3.2.0 milestone Sep 22, 2023
@hadley hadley added bug an unexpected problem or unintended behavior 3e 📚 labels Sep 22, 2023
@hadley
Copy link
Member

hadley commented Sep 26, 2023

Consulting that path isn't going to work either (and I can't see that it ever did) because the path is to the test directory, not the package directory.

@hadley
Copy link
Member

hadley commented Sep 26, 2023

Maybe test_coverage() just shouldn't call local_test_directory() at all? package_coverage() installs the package to a temporary location and then calls testInstalledPackage() which presumably calls tests/testthat and there is already existing infrastructure to correctly detect the edition?

@hadley
Copy link
Member

hadley commented Sep 26, 2023

Also need to consider test_coverage_active_file().

@hadley
Copy link
Member

hadley commented Sep 27, 2023

I've made https://github.com/hadley/testcoverage as a test — it contains two tests that will fail if either the edition or the TESTTHAT_PKG are set incorrectly.

hadley added a commit to r-lib/devtools that referenced this issue 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
hadley added a commit to r-lib/devtools that referenced this issue Sep 29, 2023
Remove unneeded (and harmful) call to `local_test_directory()` from `test_coverage()`.  Fixes r-lib/testthat#1858.

Rewrite `test_coverage_active_file()` to ensure that test failures are forwarded to the user, and fix the problems (mostly with snapshotting thus revealed).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3e 📚 bug an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants