Skip snapshot on CRAN at end of test

[hadley]

This ensures that the snapshots are not deleted. This also made me realise I missed the analgous update in expect_snapshot_file() and it's also clear that it should announce the file so it doesn't get deleted.

With these changes in place I can now run all the tests and ensure they pass on CRAN. This fixes #2214

@lionel- this feels like the right logic, even though the implementation feels a bit clunky. Now you still get a skip, but it only happens at the end of the test, after expectations have run. You can experiment yourself with local_on_cran(TRUE) and then running the tests.

[claude]

Claude finished @hadley's task —— View job

---

PR Review: Skip snapshot on CRAN at end of test

Todo List:

• Read and analyze the 7 modified files
• Review core logic changes in snapshot.R, snapshot-file.R, and test-that.R
• Check test coverage and consistency
• Provide technical feedback on potential bugs/issues

Analysis

The changes implement a deferred CRAN skip mechanism for snapshot tests. Key modifications:

Logic Changes:

• R/snapshot.R:316: Added signal(class = "snapshot_on_cran") instead of immediate skip
• R/snapshot-file.R:104-107: Added announce_snapshot_file() call and replaced skip("On CRAN") with signal
• R/test-that.R:68,181-182: Added snapshot_skipped flag and handler to trigger skip at end of test

Potential Issues:

1. Race condition in snapshot_skipped flag: The flag is set via <<- operator in line 181, but there's no reset mechanism if the test runs multiple snapshots. Could persist across different snapshot calls within same test.

2. Missing announce_snapshot_file() consistency: The change adds this to expect_snapshot_file() but not to other snapshot functions that might need similar protection.

3. Test isolation: The snapshot_skipped flag is scoped to the test execution environment but may not reset properly between tests if an error occurs.

Minor:

• Test file changes are appropriate defensive additions of local_on_cran(FALSE)
• The logic flow seems correct for the intended behavior

LGTM overall - the approach solves the stated problem and the implementation looks sound, though the race condition with snapshot_skipped flag could theoretically cause issues in edge cases.

---

[lionel-]

Nice!