Skip to content

Preserve snapshots for tests that were not run #2215

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Aug 15, 2025
Merged

Preserve snapshots for tests that were not run #2215

merged 3 commits into from
Aug 15, 2025

Conversation

hadley
Copy link
Member

@hadley hadley commented Aug 13, 2025

Fixes #2066

@jennybc please kick the tires on this.

@hadley hadley requested a review from jennybc August 13, 2025 22:07
Copy link

claude bot commented Aug 13, 2025

Claude finished @hadley's task —— View job


Code Review Complete

  • Read and analyze changed files
  • Check for potential bugs or issues
  • Provide review feedback

Review Findings

Issue: The fix preserves snapshots for tests that were not run when filtering with desc. The core logic looks sound, but found a few areas for improvement:

  1. Missing parameter documentation in R/snapshot-reporter.R:229: The new desc parameter in local_snapshotter() lacks roxygen documentation. Add @param desc documentation.

  2. Potential edge case in R/snapshot-reporter.R:49: The startsWith() logic assumes test names don't contain / characters, but there's no validation. Consider adding a check or clearer documentation about this limitation.

  3. Function signature consistency: test_description() now takes an optional parameter but existing calls assume it's parameterless. Good defensive programming to maintain backward compatibility.

Code quality: Clean implementation with appropriate test coverage. The filtering logic correctly preserves order by setting matched elements to NULL rather than removing them.

Test coverage: Good - the new test "can filter with desc" validates the core functionality properly.

LGTM overall. The minor documentation issue is worth addressing.

Copy link
Member

@jennybc jennybc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I checked with my original reprex and also checked that this new test fails when applied to main. I pushed one small commit with another skip, which seems consistent with what I see going on elsewhere (unrelated but came across my radar while reviewing).

@hadley
Copy link
Member Author

hadley commented Aug 15, 2025

@jennybc FWIW testthat should now automatically skip suggested packages that are not installed.

@hadley hadley closed this Aug 15, 2025
@hadley hadley reopened this Aug 15, 2025
@hadley hadley merged commit b179aa3 into main Aug 15, 2025
25 checks passed
@hadley hadley deleted the snap-filtering branch August 15, 2025 12:39
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.

Running a single snapshot test deletes snapshots from all other snapshot tests in that file
2 participants