tests: bump coverage from 78.36% to 99.67% - #102
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
This PR substantially increases test coverage by adding new testthat cases across the package’s Dockerfile-generation and parsing helpers, plus a small test fixture isolation improvement. No R/ source files are modified.
Changes:
- Add new tests covering additional branches/behaviors in Dockerfile R6 mutators,
dock_from_desc(),dock_from_renv(), and Dockerfile parsing. - Introduce new test files for
.dockerignorecreation/append/no-op behavior and foradd_*()helpers (warnings andCOPY --from=form). - Improve test isolation in
test-dockerbuild.Rby using a fresh per-test temp directory (instead of session-sharedtempdir()).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/testthat/test-r6.R | Adds coverage for Dockerfile$custom() and Dockerfile$add_after() mutators. |
| tests/testthat/test-get_sysreqs.R | Skips sysreq-related test on macOS due to host-dependent pak behavior. |
| tests/testthat/test-dockerignore.R | New tests for .dockerignore creation, .Rbuildignore append, and no-op behavior. |
| tests/testthat/test-dockerfile-parsing.R | Adds structural tests for parse_dockerfile() (R6 type, continuations, comments). |
| tests/testthat/test-dockerbuild.R | Uses a fresh temporary working directory for the docker build test. |
| tests/testthat/test-dock_from_renv.R | Adds coverage for user, sysreqs=FALSE, extra_sysreqs, expand=TRUE, .patch_rprofile_for_ppm guard, and gen_base_image() deprecation warning. |
| tests/testthat/test-dock_from_desc.R | Adds coverage for SystemRequirements messaging, sysreq toggles, expand behavior, tarball build/copy paths, and mocked GitHub remotes handling. |
| tests/testthat/test-add.R | New tests for add_add()/add_copy() warning paths and COPY --from=<stage> emission. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1,4 +1,5 @@ | |||
| n_temp <- tempdir() | |||
| n_temp <- tempfile(pattern = "dockerbuild") | |||
| dir.create(n_temp) | |||
Comment on lines
+295
to
+301
| # requiring a non-CRAN package to be installed. Note: the | ||
| # install_github emission iterates over `remotes_deps$remote` | ||
| # filtered by `!is_cran` (mock-controlled), not over | ||
| # `packages_not_on_cran` (which here ends up containing the | ||
| # DESCRIPTION's real Imports because of the `length(x > 0)` | ||
| # branch always entering). The emission path is therefore driven | ||
| # entirely by the mock's `$remote` list-column. |
Test-only additions and a small fixture cleanup. No code under R/ is modified; the two remaining uncovered lines are `R/dock_from_desc.R:296` (dead `if (missing(out))` branch addressed by #99) and `R/dock_from_desc.R:308` ("please install pkgbuild" stop that only fires if pkgbuild is uninstalled, untestable in CI). New tests, by file: - `test-add.R` (new): warn paths of `add_add(force = FALSE)` / `add_copy(force = FALSE)`, and `add_copy(stage = "<name>")` form. - `test-dockerignore.R` (new): create-from-scratch, append to an existing `.Rbuildignore`, no-op when `.dockerignore` already exists. - `test-r6.R`: `dock$custom()` and `dock$add_after()` R6 mutators. - `test-dock_from_renv.R`: * `gen_base_image()` deprecation warning when `distro` is supplied. * `dock_from_renv(user = ...)` emits `USER <name>`. * `sysreqs = FALSE` skips the apt-get install entirely. * `extra_sysreqs = c(...)` adds them to the install line. * `expand = TRUE` emits one apt-get install RUN per requirement plus the update / clean pair. * `.patch_rprofile_for_ppm` defensive guard when no Rprofile.site tee line is present. - `test-dock_from_desc.R`: * `SystemRequirements` field in DESCRIPTION triggers the user message branch. * `sysreqs = FALSE`, `extra_sysreqs`, `expand = TRUE` paths. * `build_from_source = FALSE` with `update_tar_gz = TRUE` and a pre-existing tarball, with `pkgbuild::build` and `usethis::use_build_ignore` mocked via `with_mocked_bindings`. * `build_from_source = FALSE` with `update_tar_gz = FALSE` (the use-already-built-tarball path). * `Remotes:` (GitHub) branch via mocked `remotes::package_deps()`, plus the `github_pat = "secret"` decoration on the `install_github` RUN. - `test-dockerfile-parsing.R`: round-trip plus three structural tests (R6 type, multi-line continuation glued to one logical instruction, comment lines preserved). - `test-dockerbuild.R`: switch from `tempdir()` (session-shared, can carry leftover state across tests) to `tempfile() + dir.create()` for a fresh per-test working directory. Coverage delta: - before: 78.36 % - after: 99.67 %
- .patch_rprofile_for_ppm: add a second test exercising the "two matching tee lines" arm of the `length(rps_idx) != 1L` guard (the first test only covered the empty-Dockerfile arm). - dock_from_desc Remotes test: clarify in a comment that the install_github emission iterates over the mocked `remotes_deps$remote[!is_cran]`, not over `packages_not_on_cran` (which currently contains the real DESCRIPTION's Imports because of the `length(x > 0)` branch always entering).
…sysreqs
`pak::pkg_sysreqs(sysreqs_platform = "ubuntu" | "debian")` returns
no `system_packages` on macOS hosts, so any test that asserts the
presence of an apt-get install line or a non-empty sysreqs vector
cannot pass on the macos-latest runner. This was already the case on
master and is unrelated to this PR.
Affected tests, all gated with `skip_on_os("mac")`:
- test-dock_from_desc.R `dock_from_desc works` (asserts
"apt-get update && apt-get install" in the generated Dockerfile)
- test-dock_from_renv.R `dock_from_renv works with full dependencies`
(asserts "python3" in the generated Dockerfile)
- test-get_sysreqs.R `get_sysreqs works` (asserts a non-empty
character vector for `mongolite`)
The skipped behaviour is fully exercised on every Linux runner
(release / devel / oldrel-1) and on Windows, which is where users
actually generate Linux Dockerfiles.
Two follow-ups to the Copilot review on this PR: - test-dockerbuild.R: replace the manual `tempfile() + dir.create()` pair with `withr::local_tempdir()`. The previous form leaked a fresh sibling under `tempdir()` per test run with no teardown. - test-dock_from_desc.R: rephrase the comment above the synthetic `package_deps()` mock to describe the real driver (`$remote` filtered by `!is_cran`) without referencing the production `length(x > 0)` bug. The reference would become wrong as soon as that bug is fixed.
VincentGuyader
force-pushed
the
tests/coverage-bump-102
branch
from
May 5, 2026 21:13
0ff1a1d to
6f79568
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test-only additions and one fixture cleanup. No R/ source is modified.
The two remaining uncovered lines are:
R/dock_from_desc.R:296- deadif (missing(out))branch (deleted by fix(dock_from_desc): remove deadif (missing(out))branch (#98) #99)R/dock_from_desc.R:308-stop("please install pkgbuild")that only fires if pkgbuild is uninstalled (untestable in CI)Coverage delta
What's tested now that wasn't before
R/add.Radd_add(force = FALSE)warn path,add_copy(force = FALSE)warn path,add_copy(stage = "<name>")--from=formR/dockerfile.Rdock\$custom()anddock\$add_after()R6 mutatorsR/dockerignore.R.Rbuildignoreappend branch when the file already exists, and the no-op skip when.dockerignoreis already presentR/gen_base_image.Rdistrois suppliedR/dock_from_renv.Ruser = ...,sysreqs = FALSE,extra_sysreqs,expand = TRUE, and the defensivelength(rps_idx) != 1Lguard in.patch_rprofile_for_ppmR/dock_from_desc.RSystemRequirementsfield message branch,sysreqs = FALSE,extra_sysreqs,expand = TRUE, bothbuild_from_source = FALSEpaths (withpkgbuild::buildandusethis::use_build_ignoremocked viawith_mocked_bindings), and theRemotes:(GitHub) branch via mockedremotes::package_deps()(also coveringgithub_pat = "secret"decoration on theinstall_githubRUN)R/parse-dockerfile.RFixture cleanup
tests/testthat/test-dockerbuild.Rswitched fromtempdir()(session-shared, can carry leftover state from earlier tests) totempfile() + dir.create()for a fresh per-test working directory.Test plan
devtools::test()- 0 failures, 0 errorscovr::package_coverage()reports 99.67 %This PR is independent of #96 / #97 / #99 / #101 and can land in any order; once all five are merged we bump to 0.2.7.