Refactor for clarity and modernize the test suite#32
Merged
Conversation
- Drop unused internal helpers verify_status(), %||%, version_above(), and block_show() (zero callers; rlang already provides %||%). - Remove return()/FALSE statements that are unreachable because the preceding cli_abort() always throws (binary_download, dmg_package_install, pkg_install). - Regenerate docs to drop the orphaned man pages.
- gfortran: heredoc target '~./Renviron' -> '~/.Renviron' (4 examples). - toolchain: 'macros_rtools_*'/'mac_rtools_*' -> real 'macos_rtools_*' names. - xcode: removal step showed 'touch' instead of 'rm'.
- shell_execute() declared a timeout= parameter that the body never used
and no caller passes; drop it and its @param.
- .onLoad() called rlang::check_installed("rlang"), which is circular: the
rlang:: prefix already loads rlang, a hard Imports dependency.
Use base::identical(status, 0L) like the sibling xcode_cli_* helpers instead of the lone '== 0L', which is more robust to length-0/NA status.
…lpers install_gfortran_12_arm/11_arm were byte-identical except for the URL, as were install_gfortran_14_2_universal/12_2_universal. Extract install_gfortran_arm_tar() and install_gfortran_universal_pkg() and have the named entry points delegate with their URL. Behavior and the version-dispatch entry points are unchanged.
The pattern tryCatch(sys::as_text(sys::exec_internal(cmd, args)$stdout), error = function(e) fallback) was copy-pasted across toolchain.R (xcode-select, xcodebuild, gfortran), gfortran.R (gfortran) and openmp.R (otool, clang). Replace with a single exec_text(command, args, fallback = "Unknown") helper. The codesign (stderr) and structured gfortran()/xcode wrappers are left as-is since they read a different stream / return structured output.
- timestamp_now(): centralize the '%Y-%m-%d %H:%M:%S' format used twice in toolchain.R. - remove_file_if_exists(): replace the duplicated temp-file cleanup blocks in xcode_cli_install(). - ls_ld_fields(path, fields): collapse the two 'ls -ld' field-parsing tryCatch blocks in create_install_location(). - Hoist the 'hdiutil detach' command, built twice in dmg_package_install(), into a single detach_cmd. The block-delimiter defaults in blocks.R are left as-is to preserve parity with the upstream usethis/rlang code it is vendored from.
Introduce openmp_library_path(), openmp_header_path() and openmp_makevars_lines() as the single source of truth for the OpenMP install paths and compiler flags, replacing the scattered string literals across is_openmp_installed/openmp_version/openmp_install/openmp_uninstall/openmp_test and configure_openmp_makevars. Display sites wrap the flags with the existing two-space indent via paste0(); the written-config and bare message sites use them directly, so rendered output is unchanged. Also dedupe the default OpenMP filename by reusing openmp_mapping$filename[1] for the fallback.
The 9 is_macos_<release>() detectors and is_macos_r_supported() repeated the body 'mac_version <- shell_mac_version(); version_between(mac_version, lo, hi)'; introduce macos_version_in_range(lower, upper) and make each a one-liner. Update the is_macos_r_supported test to mock shell_mac_version at the namespace level (so the stub reaches through the new helper) and exercise the real version_between logic, and add a focused test for macos_version_in_range.
macos_rtools_install() and macos_rtools_uninstall() each re-implemented the 'prompt for a password when NULL' logic with their own wording. Delegate both to the existing force_password() helper so the prompt text and NULL-handling live in one place. This unifies (and slightly changes) the two prompt messages. Update the uninstall tests to stub force_password() instead of the now-deeper askpass::askpass() call. The sudo-guarded prompt in recipes.R is left as-is since that file is vendored near-verbatim from R Core's installer.
Every assert_* helper passed advice='...' to cli::cli_abort(), but cli_abort has no advice parameter, so the guidance was captured into ... as a condition field and never shown. Fold each advice string into the message vector as an 'i' bullet so it renders. Add a test confirming the guidance now appears in the error message.
cli_alert_info/cli_alert_warning only render their first element, so passing a multi-element vector silently dropped the rest. Split each into a single-line headline alert followed by cli_bullets() for the detail lines (the pattern already used elsewhere in the package): - force_password() (utils.R): 3 dropped explanation lines now shown. - shell_sudo_command() failure branch (shell.R): status/cause lines now shown. - dmg_package_install() unmount-failure warning (installers.R): the 'unmount manually' hint now shown (this third site was not in the original survey).
use_r_environ() is parameterized on path but hardcoded '~/.Renviron' in its 'Creating a new Renviron file' message, so a non-default path made the two messages disagree. Interpolate path instead. The base::message calls are kept (rather than migrated to cli) to stay consistent with the vendored block_append helper this function calls.
Two stubs targeted calls that moved into extracted helpers, making the stubs dead no-ops so the tests silently shelled out to the real system: - test-gfortran.R: stub exec_text (not base::tryCatch) after the gfortran --version lookup moved into exec_text(). - test-toolchain.R: stub timestamp_now (not base::format) after the timestamp moved into timestamp_now(). Surfaced by an adversarial review of the refactoring diff.
Removing the circular rlang::check_installed("rlang") from .onLoad left rlang
declared in Imports but never used directly (R CMD check NOTE: 'All declared
Imports should be used'). rlang remains available transitively via cli.
- Replace the simplified_is_r_version reimplementation (which tested a copy of the logic, not the function) with a real test of is_r_version() that mocks the version helpers via local_mocked_bindings(). - Migrate the is_macos/is_aarch64/is_x86_64 internal-function stubs from mockery::stub() to local_mocked_bindings() so they survive helper extraction, and add the complementary FALSE assertions. base::Sys.info / sys:: stubs are left as mockery (local_mocked_bindings cannot mock base/other-package calls).
All stubs here target internal predicates, so migrate them from mockery::stub() to local_mocked_bindings() (robust to helper extraction). Drop the two cli::cli_abort interception stubs and the base::paste hack: the real cli_abort already throws a matchable message, and mocking r_version_full() directly is clearer than rebinding base::paste to fake the version.
Convert the ~143 mockery::stub() calls that target macrtools' own internal functions (predicate gates like is_gfortran_installed/is_xcode_cli_installed, and extracted helpers like force_password/exec_text/shell_execute) to testthat 3's local_mocked_bindings() across the six remaining test files. Namespace-level mocking survives helper extraction, which is the exact failure mode that orphaned several stubs during the refactor. base::/cli::/sys::/askpass:: stubs are intentionally left as mockery::stub() (local_mocked_bindings cannot mock base or other-package functions), so some tests now use both mechanisms. No assertions, inputs, or test descriptions were changed; full suite and R CMD check (with tests) stay green.
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
1 participant
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.
A light refactoring pass over the package, continuing the cleanup from #31.
Refactoring
return()s aftercli_abort().exec_text()for the repeated version-lookup idiom,macos_version_in_range()for the macOS predicates, URL-parameterized gfortran installers, centralized OpenMP path/Makevars literals, and small shared helpers (timestamp_now(),remove_file_if_exists(),ls_ld_fields()).force_password().timeoutargument, the circularrlang::check_installed()call, and the now-unusedrlangimport.Bug fixes
advice=guidance thatcli_abort()was silently swallowing, asibullets.cli_alert_*()messages that previously dropped all but the first line.pathin theuse_r_environ()message instead of hardcoding~/.Renviron.Tests
is_r_version().mockery::stub()tolocal_mocked_bindings()so they survive future helper extraction (base::/cli::stubs are kept as mockery).