[Bug]: sorting seems broken in the latest release

[prymitive]

Description

I’ve noticed that updating snapshots in the latest release moves them around in the snapshot file randomly.
Seems like sorting is no longer done when sort is enabled.

Steps to Reproduce

Enable sorting
Update a list of snapshots in one file.

Expected Behavior

Snapshots are not reordered randomly

[gkampitakis]

Hey 👋 Thanks for opening this issue. I believe this is a behaviour change since 037cb97. @G-Rath Now the sorting only happens when UPDATE_SNAPS=true and in the past it has happening on addition of a new snapshot 🤔 . Is this expected?

[G-Rath]

It would be good if you could provide more of a reproduction of what you're seeing, as it sounds like you're saying the snapshots are completely misordered which would be a bug, but I've not come across this over in osv-scanner and co where we use this library quite heavily

[prymitive]

Sorry, created this ticket quickly while trying to do some refactor and forgot to update with details. Will try to add some repro

[prymitive]

I think this is happening when:

• you have Clean() with Sort: true
• you have a snapshot file on disk
• you add more unit tests that would add some new entries to the snapshot
• you run tests without UPDATE_SNAPS env variable

That's because missing entries are added even without UPDATE_SNAPS but when that happens they are not sorted.

Example, if I create snaps/clean_sort_test.go with this content:

package snaps_test

import (
	"fmt"
	"os"
	"testing"

	"github.com/gkampitakis/go-snaps/internal/test"
	"github.com/gkampitakis/go-snaps/snaps"
)

func TestMain(m *testing.M) {
	v := m.Run()

	dirty, err := snaps.Clean(m, snaps.CleanOpts{Sort: true})
	if err != nil {
		fmt.Printf("Clean returned an error: %s\n", err)
		os.Exit(1)
	}
	if dirty {
		fmt.Println("Some snapshots were outdated.")
	}

	expected := `

[TestClean/Test_1 - 1]
int(1)
---

[TestClean/Test_2 - 1]
int(2)
---

[TestClean/Test_3 - 1]
int(3)
---

`
	got, err := os.ReadFile("mock.snap")
	if err != nil {
		fmt.Printf("Can't read snap file: %s\n", err)
		os.Exit(1)
	}

	fmt.Printf("Expected:\n %s\n\nGot: %s\n", expected, string(got))
	if expected != string(got) {
		os.Exit(1)
	}

	os.Exit(v)
}

func TestClean(t *testing.T) {
	snapshot := `

[TestClean/Test_3 - 1]
int(3)
---

`

	err := os.WriteFile("mock.snap", []byte(snapshot), 0o644)
	test.NoError(t, err)

	s := snaps.WithConfig(snaps.Dir("."), snaps.Filename("mock"), snaps.Ext(""))

	t.Run("Test_3", func(t *testing.T) {
		s.MatchSnapshot(t, 3)
	})
	t.Run("Test_1", func(t *testing.T) {
		s.MatchSnapshot(t, 1)
	})
	t.Run("Test_2", func(t *testing.T) {
		s.MatchSnapshot(t, 2)
	})
}

and run that test I get this:

$ go test -count=1 -run=TestClean ./snaps/
PASS

Snapshot Summary

✓ 1 snapshot passed
✎ 2 snapshots added

Some snapshots were outdated.
Expected:
 

[TestClean/Test_1 - 1]
int(1)
---

[TestClean/Test_2 - 1]
int(2)
---

[TestClean/Test_3 - 1]
int(3)
---



Got: 

[TestClean/Test_3 - 1]
int(3)
---


[TestClean/Test_1 - 1]
int(1)
---

[TestClean/Test_2 - 1]
int(2)
---

FAIL	github.com/gkampitakis/go-snaps/snaps	0.003s
FAIL

[prymitive]

Running the same test with UPDATE_SNAPS:

$ UPDATE_SNAPS=true go test -count=1 -run=TestClean ./snaps/
PASS

Snapshot Summary

✓ 1 snapshot passed
✎ 2 snapshots added

Expected:
 

[TestClean/Test_1 - 1]
int(1)
---

[TestClean/Test_2 - 1]
int(2)
---

[TestClean/Test_3 - 1]
int(3)
---



Got: 
[TestClean/Test_1 - 1]
int(1)
---

[TestClean/Test_2 - 1]
int(2)
---

[TestClean/Test_3 - 1]
int(3)
---

[G-Rath]

Yes, so right now that is expected because existing snapshots within a file can only be changed when UPDATE_SNAPS is true, otherwise you could sort unrelated snapshots which while that might be desired by some, would be a violation of the idea that we don't change stuff without explicitly told to.

The ideal which I think you're requesting here is that when adding new snapshots with sorting enabled, the existing snapshot file is read-in and the new snapshots are sorted "around" those, and then written to disk.

[prymitive]

If I have Sort:true set then no other test should be affected, only the newly added entry.
Without sorting you get a bit more churn, as first new entry is added at the end, then you need to rerun tests with update_snaps to sort it