Repository navigation
Fix snapshot cleanup deleting .new files for multi-dot filenames - #2355
Open
jadhavgaurav wants to merge 1 commit into
Open
jadhavgaurav wants to merge 1 commit into
jadhavgaurav wants to merge 1 commit into
Conversation
snapshot_expected() split snapshot filenames on the last dot via tools::file_path_sans_ext()/tools::file_ext() to predict the .new variant, while new_name() (used when the .new file is actually created) splits on the first dot via split_path(). For a filename like tc12.2_image_annotate.html this mismatch makes cleanup expect tc12.2_image_annotate.new.html while the real file is named tc12.new.2_image_annotate.html, so the real .new file is flagged as unused and deleted. snapshot_expected() now calls new_name() directly instead of duplicating its splitting logic, so both sides always agree. Fixes r-lib#2326
This branch has not been deployed
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.
Problem
expect_snapshot_file()with a snapshot name containing more than one dot (for exampletc12.2_image_annotate.html) has its.newcomparison file deleted by the next snapshot cleanup, sosnapshot_review()has nothing to show.Root cause
new_name()inR/snapshot-file.R(used when the.newfile is created) splits a filename viasplit_path(), which finds the first dot withregexpr(".", name, fixed = TRUE). Fortc12.2_image_annotate.htmlthis producestc12.new.2_image_annotate.html.snapshot_expected()inR/snapshot-cleanup.R(used during cleanup to decide which files are still expected) instead built the.newname withtools::file_path_sans_ext()/tools::file_ext(), which split on the last dot. For the same filename that predictstc12.2_image_annotate.new.html.Since the predicted name never matches the file that was actually created, cleanup treats the real
.newfile as unused and deletes it.Fix
snapshot_expected()now callsnew_name()directly instead of re-deriving the.newname with a different, inconsistent splitting convention. Both sides of the comparison are now guaranteed to agree because they're the same code.Testing
Added a regression test in
tests/testthat/test-snapshot-cleanup.Rusing a multi-dot snapshot name. Verified it fails on the unmodified source (flags the real.newfile as outdated) and passes with the fix. Ran the full existing test suite (pkgload::load_all()+testthat::test_file()per file, since the parallelcallr-based runner doesn't start in this sandbox); 1041 pre-existing tests pass unmodified, 3 pre-existing failures reproduce identically with and without this change (they depend on running inside the package's own R CMD check/devtools::test()process context, e.g.testing_package()andtopenv()checks, not on anything touched here).Fixes #2326