Skip to content

fix: only update snapshots when actually allowed #135

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.

Sign up for GitHub

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
gkampitakis merged 5 commits into from
Sep 17, 2025
Merged

fix: only update snapshots when actually allowed #135

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
907c290
fix: regression on reporting isDirty and sorting snaps
gkampitakis Sep 10, 2025
ac2c6e5
add the writing enabled guard
gkampitakis Sep 10, 2025
b441d56
fix: only update snapshots when actually allowed, but always report i…
G-Rath Sep 16, 2025
ea3d141
refactor: rename variables
G-Rath Sep 16, 2025
74b5803
Merge branch 'main' into gkampitakis/dirty2
gkampitakis Sep 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 11 additions & 9 deletions snaps/clean.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ func examineSnaps(
used []string,
runOnly string,
count int,
update,
shouldUpdate,
sort bool,
) ([]string, bool, error) {
obsoleteTests := []string{}
Expand All @@ -251,7 +251,7 @@ func examineSnaps(
return nil, isDirty, err
}

var hasDiffs bool
var needsUpdating bool

registeredTests := occurrences(registry[snapPath], count, snapshotOccurrenceFMT)
s := snapshotScanner(f)
Expand All @@ -267,7 +267,7 @@ func examineSnaps(

if !registeredTests.Has(testID) && !testSkipped(testID, runOnly) {
obsoleteTests = append(obsoleteTests, testID)
hasDiffs = true
needsUpdating = true

removeSnapshot(s)
continue
Expand All @@ -292,13 +292,15 @@ func examineSnaps(
return nil, isDirty, err
}

shouldSort := sort && !slices.IsSortedFunc(testIDs, naturalSort) && update
shouldDelete := hasDiffs && update
needsSorting := sort && !slices.IsSortedFunc(testIDs, naturalSort)

isDirty = isDirty || (sort && !slices.IsSortedFunc(testIDs, naturalSort)) || hasDiffs
// if we're not allowed to update anything, just capture if the snapshot
// needs cleaning, and then continue to the next snapshot
if !shouldUpdate {
if needsUpdating || needsSorting {
isDirty = true
}

// if we don't have to "write" anything on the snap we skip
if !shouldDelete && !shouldSort {
f.Close()

clear(tests)
Expand All @@ -308,7 +310,7 @@ func examineSnaps(
continue
}

if shouldSort {
if needsSorting {
// sort testIDs
slices.SortFunc(testIDs, naturalSort)
}
Expand Down
18 changes: 14 additions & 4 deletions snaps/clean_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,8 @@ func TestExamineSnaps(t *testing.T) {
// Content of snaps is not changed
test.Equal(t, mockSnap1, []byte(content1))
test.Equal(t, mockSnap2, []byte(content2))

// And thus we are dirty since the contents do need changing
test.True(t, isDirty)
})

Expand Down Expand Up @@ -276,13 +278,15 @@ string hello world 2 2 1
)
test.NoError(t, err)

// Content of snaps is not changed
// Content of snaps have been updated
test.Equal(t, expected1, content1)
test.Equal(t, expected2, content2)
test.True(t, isDirty)

// And thus we are not dirty
test.False(t, isDirty)
})

t.Run("should sort all tests", func(t *testing.T) {
t.Run("should sort all tests when allowed to update", func(t *testing.T) {
shouldUpdate, sort := true, true
mockSnap1 := loadMockSnap(t, "mock-snap-sort-1")
mockSnap2 := loadMockSnap(t, "mock-snap-sort-2")
Expand All @@ -306,9 +310,12 @@ string hello world 2 2 1
content1 := test.GetFileContent(t, filepath.FromSlash(dir1+"/test1.snap"))
content2 := test.GetFileContent(t, filepath.FromSlash(dir2+"/test2.snap"))

// Content of snaps are now sorted
test.Equal(t, string(expectedMockSnap1), content1)
test.Equal(t, string(expectedMockSnap2), content2)
test.True(t, isDirty)

// And thus we are not dirty
test.False(t, isDirty)
})

t.Run(
Expand Down Expand Up @@ -347,8 +354,11 @@ string hello world 2 2 1
content1 := test.GetFileContent(t, filepath.FromSlash(dir1+"/test1.snap"))
content2 := test.GetFileContent(t, filepath.FromSlash(dir2+"/test2.snap"))

// Content of snaps is not changed
test.Equal(t, string(mockSnap1), content1)
test.Equal(t, string(mockSnap2), content2)

// And thus we are dirty, since there are obsolete snapshots that should be removed
test.True(t, isDirty)
},
)
Expand Down
Loading