fix(dock_from_desc): remove dead if (missing(out)) branch (#98) - #99
Merged
Conversation
VincentGuyader
added a commit
that referenced
this pull request
May 5, 2026
Address self-review nits on PR #99: - nit 1: drop the inert `file.create(fake_tar)` + `on.exit` setup. With `update_tar_gz = FALSE`, the `if (update_tar_gz)` block is skipped and `list.files` never reads the fake file - the setup did nothing. - nit 2: add a second test for the `build_from_source = FALSE, update_tar_gz = TRUE` path. Mocks `pkgbuild::build` (returns a fake tar.gz path) and `usethis::use_build_ignore` (no-op), then asserts both bindings were actually called - the survivor lines of the dead-code removal (`use_build_ignore(files = out)` and the `cat_green_tick(...)` success message) are now exercised. This brings the previously-claimed-but-untested 0% region to covered.
1 task
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
3 tasks
`out <- pkgbuild::build(...)` is a local assignment, not a function
parameter, so `missing(out)` is unconditionally FALSE. The
`cat_red_bullet("Error during tar.gz building")` branch was therefore
unreachable. `pkgbuild::build()` errors via `stop()` on failure, not
by returning a missing sentinel, so the intent (catch failure of
`build()`) was at the wrong layer anyway.
Drop the dead `if/else` wrapper; let R's normal error propagation
surface any `build()` failure to the caller.
Add a smoke test for `build_from_source = FALSE` (a code path that
had 0% coverage). The test creates a fake tar.gz fixture, mocks
`get_sysreqs()` to skip the network call, and asserts the generated
Dockerfile contains the expected COPY of the tar.gz, the
`remotes::install_local()` RUN, and the cleanup `rm /app.tar.gz`.
Address self-review nits on PR #99: - nit 1: drop the inert `file.create(fake_tar)` + `on.exit` setup. With `update_tar_gz = FALSE`, the `if (update_tar_gz)` block is skipped and `list.files` never reads the fake file - the setup did nothing. - nit 2: add a second test for the `build_from_source = FALSE, update_tar_gz = TRUE` path. Mocks `pkgbuild::build` (returns a fake tar.gz path) and `usethis::use_build_ignore` (no-op), then asserts both bindings were actually called - the survivor lines of the dead-code removal (`use_build_ignore(files = out)` and the `cat_green_tick(...)` success message) are now exercised. This brings the previously-claimed-but-untested 0% region to covered.
VincentGuyader
force-pushed
the
fix/dock-from-desc-dead-missing-98
branch
from
May 5, 2026 20:59
c66877a to
7171d64
Compare
VincentGuyader
added a commit
that referenced
this pull request
May 5, 2026
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 %
VincentGuyader
added a commit
that referenced
this pull request
May 5, 2026
* tests: bump coverage from 78.36% to 99.67% 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 % * tests: address self-review nits on coverage PR - .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). * tests: skip on macOS the 3 tests that depend on pak's cross-platform 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. * tests: clean up dockerbuild tempdir leak; rephrase mock-driver comment 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.
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.
Closes #98.
out <- pkgbuild::build(...)is a local assignment, not a function parameter, somissing(out)always returns FALSE. The error-handling branchcat_red_bullet("Error during tar.gz building")was unreachable.pkgbuild::build()errors viastop()on failure, so the intent (catch failure) was at the wrong layer anyway.Changes
if/elsewrapper, keep the success-path body inline.build_from_source = FALSE(path that had 0% coverage previously). The test mocksget_sysreqs()to avoid the network call and asserts the COPY of the tar.gz + theinstall_local()RUN + the cleanup are present.Test plan
devtools::test(filter = "dock_from_desc")-> 29 PASS / 0 FAIL.Related
Discovered by full-codebase audit (#94 sibling, finding F2). The companion finding F8 (
try(initialize(), silent=TRUE)on the vendored renv) is subsumed by #97 (drop the vendored renv entirely).