test(fix_globals): cover multi-run scenarios A-F#128
Merged
Conversation
Lock in the contract of `fix_globals(write = TRUE)` under repeated invocations with varying shapes. Six new scenarios, all green on the post-#125 / #126 codebase: - A. 2 runs accumulating new vars on the 2nd run without losing 1st-run names (per-`# <fun>:` group emitted for each new fun). - B. Overlap between runs deduplicated at runtime via the `unique(c(...))` wrapper. - C. Consecutive no-op runs yield byte-identical `globals.R` (idempotence after the #125 early-return). - D. User-curated name added manually between runs survives the next `fix_globals(write = TRUE)` pass. Comment loss is documented as tolerated for now via an explicit `expect_false()` so a future change that preserves comments has to revisit the assertion. - E. Real round-trip with `create_example_pkg()`: the 2nd pass is a filesystem no-op (`skip_on_cran()` because of the real `rcmdcheck()` call). - F. Function-only notes on N consecutive runs never create `globals.R`; `liste_funs` surfaces every detected function on every run (both `where` and `summarise` asserted independently). No source change; pure regression coverage. Closes #127.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds regression coverage for repeated fix_globals(write = TRUE) runs, focusing on accumulation, deduplication, idempotence, manual edits, real package round-trips, and function-only note handling.
Changes:
- Adds multi-run
fix_globals()test scenarios A-F. - Introduces local helpers for temporary packages, mocked check results, and declared global extraction.
- Includes one real
create_example_pkg()round-trip gated withskip_on_cran().
Comments suppressed due to low confidence (1)
tests/testthat/test-fix-globals-multi-run.R:126
- This assertion cannot catch duplicate declarations because
aftercomes fromdeclared_names(), which delegates toextract_existing_globals()and is already deduplicated withunique(). If the goal is to lock in thatsharedis not emitted twice, parse/readglobals.Rdirectly without the deduping helper before counting occurrences.
after <- declared_names(globals_path)
expect_setequal(after, c("shared", "only_in_run1", "only_in_run2"))
expect_equal(sum(after == "shared"), 1L,
info = "duplicate names must be collapsed by the runtime"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+84
to
+86
| expect_equal(length(after), length(unique(after)), | ||
| info = "no name should be declared twice" | ||
| ) |
Comment on lines
+124
to
+127
| expect_setequal(after, c("shared", "only_in_run1", "only_in_run2")) | ||
| expect_equal(sum(after == "shared"), 1L, | ||
| info = "duplicate names must be collapsed by the runtime" | ||
| ) |
Both scenarios had a duplicate-suppression assertion that routed through `declared_names()` -> `extract_existing_globals()`, which internally calls `unique()` (R/audit_globals.R:249). The assertions were therefore tautological - they would have passed even if the on-disk file carried a literal duplicate. Investigation also revealed that the merge path of `fix_globals(write = TRUE)` does emit a literal duplicate on disk in scenario B's shape (1 occurrence in the fresh `# fn_b:` block, 1 in the `# previously declared:` chunk); the safety net is the `unique(c(...))` wrapper in `utils::globalVariables(unique(c(...)))` that collapses the duplicate at runtime, which is what R CMD check actually consumes. Fixes: - Scenario A: drop the tautology. `expect_setequal()` already locks in the runtime set. - Scenario B: replace the tautology with a parse-and-eval assertion that bypasses `declared_names()` and exercises the runtime `unique()` directly. Docstring updated to spell out that the contract is the runtime set, not the on-disk text.
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
tests/testthat/test-fix-globals-multi-run.Rwith 6test_thatblocks A-F locking in the multi-run contract offix_globals(write = TRUE).Closes #127.
Scenarios
# <fun>:group emitted per new fun; 1st-run names surviveunique(c(...))wrapperglobals.R(idempotence after #125 early-return)create_example_pkg(): 2nd pass is a filesystem no-op (skip_on_cran())globals.Rover N runs;liste_funssurfaceswhereandsummariseindependently every runTest plan
devtools::test()green (604 PASS / 0 FAIL with the new 19 expectations).skip_on_cran()and useslocal_tempdir_clean()+on.exit(unlink).Follow-ups (out of scope)
local_pkg_with_globals()is now defined inline in bothtest-fix-globals-merge.Randtest-fix-globals-multi-run.R. Promote totests/testthat/helpers.Rin a follow-up.fake_check_result()(new, flexible multi-fun) is a strict superset offake_globals()(in merge test). Unify under a single name in helpers.R when the duplication is addressed.