gkampitakis / go-snaps

Jest-like snapshot testing in Go 📸
https://pkg.go.dev/github.com/gkampitakis/go-snaps
MIT License
164 stars 6 forks source link

Snapshots combined with using `-count` #87

Closed G-Rath closed 8 months ago

G-Rath commented 9 months ago

Issue

This could be a bug or a feature - I'm still actually digging into this but wanted to open an issue because you might be able to dig into it faster (if you've not already come across this!)

It looks like when running with -count (i.e. go test -count 5 ./...) in some (if not all?) cases snapshots get counted multiple times for the same test rather than recompared - i.e. the snapshot gets updated with [TestRun/#00 - n] where n is the number of snapshots called so far across all runs.

I think this is because the test ids are using a global registry + mutex and that -count is working within the same process, it just calls the test functions multiple times.

I think this could be worked around by exposing a way of manually clearing/resetting the registry, which could be called before every test in TestMain, but I've not yet tested that to confirm.

I'm also not sure if something like https://github.com/golang/go/issues/64883 would make it possible for the library to handle this automatically.

gkampitakis commented 9 months ago

Hey 👋 That is correct. I can't say it's a bug but rather a feature of supporting multiple calls of matchSnapshot in a single test.

in some (if not all?)

It's all cases.

I have spent some time looking into this, but couldn't find a good solution unfortunately. I would be willing to do a breaking change (some people are using snaps with a count) as the correct way would be do run a test mulitple times and verify with the same snapshot each time but I don't want to remove the ability to be able to call matchSnapshot multiple times in a single test.

The reason why this is difficult to solve is because there is no way to differentiate a snaps.MatchSnapshot is called due to the -count>1 or the snaps.MatchSnapshot is called multiple times inside the test.

As for the TestMain I am pretty it's called only once when you invoke go test and not count times.

To sum up, from the snaps perspective

This

func TestMyTest(t *testing.T) {
  t.Run("my test", func (t *testing.T) {
    snaps.MatchSnapshot(t, "hello world")
    snaps.MatchSnapshot(t, "hello world")
  }
}

and this can't be differentiated.

// go test -count=2 ./...
func TestMyTest(t *testing.T) {
  t.Run("my test", func (t *testing.T) {
    snaps.MatchSnapshot(t, "hello world")
  }
}

I can check again my notes and what I tried in the past as it has been some time. As for the go issue it could potentially help in oder to identify if calling with -count>1.

G-Rath commented 9 months ago

What you've said is pretty much word-for-word what I was thinking too, and why I went looking for https://github.com/golang/go/issues/64883 but then I managed to get myself confused in trying to reproduce it 😅

I've put a comment on that testing.Count issue.

gkampitakis commented 8 months ago

Okay found my old notes and spent some time brainstorming with a new solution. I think have something working, but need to spend some time refining it before creating a pr. My solution interfers a bit with clean up functionality and I wouldn't like to introduce a bug there.

G-Rath commented 8 months ago

Cool - fwiw I was thinking you could also support this by introducing a new method for resetting/clearing the global registry for a test which could be called whenever the test started:

func TestMyTest(t *testing.T) {
    t.Run("my test", func(t *testing.T) {
        snaps.Start(t)
        snaps.MatchSnapshot(t, "hello world")
        snaps.MatchSnapshot(t, "hello world")
    })
}
gkampitakis commented 8 months ago

I tried to do something similar indeed without affecting the api but this is a breaking change as some people are already running it with -count>1 but as I said

the correct way would be do run a test mulitple times and verify with the same snapshot each time

Need to run more manual tests to verify I don't introduce any regressions and add unit tests.

Happy to hear your thoughts on this solution.