Skip to content

Conversation

@hadley
Copy link
Member

@hadley hadley commented Aug 28, 2025

This shouldn't have any effect on user code, but we need some way to ensure that CRAN skips aren't propagated across multiple test_code() boundaries so that we can properly test.

This shouldn't have any effect on user code, but we need some way to ensure that CRAN skips aren't propagated across multiple `test_code()` boundaries so that we can properly test.
@claude
Copy link

claude bot commented Aug 28, 2025

Claude finished @hadley's task —— View job


Code Review Complete

  • Read and analyze changed files
  • Check implementation for bugs/issues
  • Provide review feedback

Review Findings

LGTM - The implementation correctly addresses the stated goal of preventing CRAN skips from propagating across test_code() boundaries.

Key Changes:

  1. Centralized signal handling: Created signal_snapshot_on_cran() function that wraps the signal with a muffle_cran_snapshot restart
  2. Added restart invocation: The snapshot_on_cran handler in test_code() now calls invokeRestart("muffle_cran_snapshot")
  3. Consistent usage: Updated expect_snapshot_file() and expect_snapshot_helper() to use the new centralized function

Technical soundness:

  • The restart mechanism properly contains the skip signal within test boundaries
  • Function placement in R/snapshot.R is appropriate as it serves both snapshot functions
  • Test change from expect_snapshot_skip() to expect_skip() correctly reflects that CRAN skips are now muffled during testing

No issues identified - The changes are minimal, focused, and implement the intended behavior without affecting user-facing functionality.

@hadley hadley merged commit 6883737 into main Aug 28, 2025
13 checks passed
@hadley hadley deleted the cran-skips branch August 28, 2025 21:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants