[dgkf]

For now just drafting changes, will log findings here

[dgkf]

I wasn't able to track down why this was failing, but by iteratively working through various tests I was able to at least narrow it down to which code was causing the failure.

Turns out it was just the use of devtools::install in our testing suite. Simple enough, I just swapped it out for the base-R equivalent.

Looking more holistically over the package, our use of devtools is pretty narrow, and it's quite a heavy dependency for such a small footprint. I'm a bit reluctant to swap out the machinery for R CMD check while this is maintenance mode, but if we continue to see issues with devtools, the easiest path forward might be to just strip out devtools for more specialized packages.

That said, I'd really appreciate if some other folks could test this against the development build of devtools because I'm still unclear on why we're seeing this in the first place.

[llrs-roche]

devtools' NEWS on devel reports this change:

• install() now installs dependencies with pak::local_install_deps() instead of remotes::install_deps()
  Haven't tested the changes though.

[dgkf]

devtools' NEWS on devel reports this change:

• install() now installs dependencies with pak::local_install_deps() instead of remotes::install_deps()
  Haven't tested the changes though.

I hadn't seen this, but I'm not sure why it would have an effect. The package fixture's only dependency is R (>= 3.1.2).

Not seeing any changes in the diff since last release that I would expect to crash subprocesses.

[llrs-roche]

Related to devtools I only see one relevant change from devtools::install() to install.packages(). I think this will be enough to avoid issues and locally I couldn't detect any issue with devtools 2.5.0.

I left some comments about minor simplifications of the code too.

---

While testing I realized that webmockr is not used conditionally as the check failed with an error:

Error in `source_dir()`:
! Failed to evaluate './setup_mock_web_requests.R'.
Caused by error in `library()`:
! there is no package called 'webmockr'

I would check other packages on Suggests that they are properly used only when the package is available.

---

I also got a note (with R 4.5.3) about the documentation:

❯ checking Rd files ... NOTE
  checkRd: (-1) pkg_ref.Rd:74-75: Lost braces in \itemize; meant \describe ?
  checkRd: (-1) pkg_ref.Rd:77: Lost braces in \itemize; meant \describe ?
  checkRd: (-1) pkg_ref.Rd:78-80: Lost braces in \itemize; meant \describe ?
  checkRd: (-1) pkg_ref.Rd:81-82: Lost braces in \itemize; meant \describe ?
  checkRd: (-1) pkg_ref.Rd:84-85: Lost braces in \itemize; meant \describe ?
  checkRd: (-1) pkg_ref.Rd:86-87: Lost braces in \itemize; meant \describe ?
  checkRd: (-1) pkg_ref.Rd:88-89: Lost braces in \itemize; meant \describe ?

[llrs-roche]

I'm not sure how does on.exit() play with withr

Suggested change

	on.exit(file.remove(pkg_archive))
	install.packages(pkg_archive, type = "source", repos = NULL, quiet = TRUE)
	on.exit(file.remove(pkg_archive), add = TRUE)
	install.packages(pkg_archive, type = "source", repos = NULL, quiet = TRUE)

If the installation of the package fails, is there a way to capture and exit early the test? Just to cover all bases. I don't think it fail, but installing packages while testing might be a bit problematic

[llrs-roche]

Now that the path is out of the built procedure it can be reused:

Suggested change

	pkg_ref_source_good <- pkg_source(file.path(
	test_path(),
	"test_packages",
	"pkgsourcegood"
	))
	pkg_ref_source_good <- pkg_source(pkg_path)

[dgkf]

I would check other packages on Suggests that they are properly used only when the package is available.

This is a bit tricky in this situation because it's used in setup code. The tests would still run even if we skipped the setup code when webmockr is unavailable, but we'd proceed to run our tests against actual http request responses. We'd have to make sure we're skip_on'ing all the tests that use these requests, even though it might not be obvious.

I agree that it would be better to add a skip_on for any test that uses a webmockr request, but I don't want to rock the boat just before an urgent CRAN fix.

I also got a note (with R 4.5.3) about the documentation

Good call, I'll fix these before the release.

---

Otherwise, both of the suggestions look good to me. I think on.exit applies only when the current scope exits, but add = TRUE is a good safety measure regardless.