gkampitakis / go-snaps

Jest-like snapshot testing in Go ðŸ“ļ
https://pkg.go.dev/github.com/gkampitakis/go-snaps
MIT License
174 stars 6 forks source link

Questions about effectively using go-snaps #100

Closed stdedos closed 3 months ago

stdedos commented 4 months ago

Issue

Hello there!

I have recently taken to use https://github.com/gkampitakis/go-snaps, and I am also not-a-pro in Golang!

I'd be hoping you could borrow me a bit of your experience, for using the library more effectively (and/or Go experience).

Here is my testing attempt: https://github.com/stdedos/junit2html/tree/test/add-snapshot-based-testing/tests More or less, my question revolves around "How can each folder be an individual test?".

That solves me a couple of questions:

  1. How could each test run individually?

I haven't gotten around refactoring everything to be a T.., err signature, so inevitably I'm panicing. ... Which, inevitably can crash n > 1 snapshot testing.

  1. snaps.Clean(m, snaps.CleanOpts{Sort: true}) does not do its job, if it's only called from this test.

Suppose I renamed the directory. Then I'd have two snapshots, instead of one.

  1. Since I'm calling the snapshot once in my test, could I "somehow" store the file, "instead like the snapshot format", as e.g. a result.html?

(Okay, okay - I'm calling it twice ðŸĪŠ) Then that gives the possibility to "statically render" the file - hence, providing I/O examples to people coming and visiting.

gkampitakis commented 4 months ago

Hey 👋

How can each folder be an individual test?

I am not 100% following this. If you could elaborate a bit more.

  1. How could each test run individually?

go test supports either running a specific bath go test /my-package or go test -run <regex> ( you can see more information with go help testflag )

snaps.Clean(m, snaps.CleanOpts{Sort: true}) does not do its job, if it's only called from this test.

snaps.Clean is supposed to be called from the TestMain of the package as it needs to be called once after all tests have been executed. if your "one test for each folder" is a separate package your can have multiple TestMain else you can have a small helper function that on every run will delete all snapshots and recreated them.

Since I'm calling the snapshot once in my test, could I "somehow" store the file, "instead like the snapshot format", as e.g. a result.html?

I am not sure what would be the purpose of it, but unfortunately go-snaps isn't built around giving you this possibility.

I am not 100% sure I have exactly understood your usecase, or what your "end goal" is. Happy to help in any case.

p.s. For the 3rd point a similar project for snapshot testing Cupaloy uses one file per snapshot and could potentially allow you to have the setup you need.

stdedos commented 4 months ago
Irrelephant Your MD format is a little broken 😅 Did you mean Hey 👋 > How can each folder be an individual test? I am not 100% following this. If you could elaborate a bit more. > 1. How could each test run individually? `go test` supports either running a specific bath `go test /my-package` or `go test -run ` ( you can see more information with `go help testflag` ) > snaps.Clean(m, snaps.CleanOpts{Sort: true}) does not do its job, if it's only called from this test. `snaps.Clean` is supposed to be called from the `TestMain` of the package as it needs to be called once after all tests have been executed. if your "one test for each folder" is a separate package your can have multiple `TestMain` else you can have a small helper function that on every run will delete all snapshots and recreated them. > Since I'm calling the snapshot once in my test, could I "somehow" store the file, "instead like the snapshot format", as e.g. a result.html? I am not sure what would be the purpose of it, but unfortunately go-snaps isn't built around giving you this possibility. I am not 100% sure I have exactly understood your usecase, or what your "end goal" is. Happy to help in any case. p.s. For the 3rd point a similar project for snapshot testing [Cupaloy](https://github.com/bradleyjkemp/cupaloy) uses one file per snapshot and could potentially allow you to have the setup you need. ?

I am not 100% following this. If you could elaborate a bit more.

tl;dr instead of this:

$  tree tests
tests
├── file_based_test.go
├── my_first_test
│   └── junit.xml -> ../../examples/junit.xml
├── __snapshots__
│   └── file_based_test.snap
└── testcase_only_tag
    ├── TEST-test.checks.abc.xml
    └── TEST-test.def.xml

Each folder instead have a snapshot. I have even tried doing

            err := os.Chdir(wd.Name())
            assert.Nil(t, err)

on the tt-scope, but that doesn't help. I guess the point is where

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

    // After all tests have run, `go-snaps` will sort snapshots.
    snaps.Clean(m, snaps.CleanOpts{Sort: true})

    os.Exit(v)
}

is running.

p.s. For the 3rd point a similar project for snapshot testing Cupaloy uses one file per snapshot and could potentially allow you to have the setup you need.

Yea, I guess https://github.com/bradleyjkemp/cupaloy looks like it: https://github.com/bradleyjkemp/cupaloy/blob/bae07880a7b7e65701a45ecbb32914e6552626a5/examples/.snapshots/chosen-by-user

I guess I could even rename it to .html and be done with it!

... but the library had its last commit 2 years ago 😕

I am always skeptical (and upset on a tangent) that there are so many snapshot-testing libraries, but "enough" don't look recently update. Maybe they are just feature-complete ofc, but idk 😕

I am not 100% sure I have exactly understood your usecase, or what your "end goal" is. Happy to help in any case.

You can render https://github.com/stdedos/junit2html/blob/4b0b264bb69e9fb01f93c77e16f1351f2ac595d1/examples/test-report.html via https://htmlpreview.github.io/?https://github.com/stdedos/junit2html/blob/4b0b264bb69e9fb01f93c77e16f1351f2ac595d1/examples/test-report.html (or faster if someone fixes https://github.com/Mottie/GitHub-userscripts/wiki/GitHub-html-preview 😅)

I was hopping that every test in the tests/ folder would be also renderable.

It's the fastest way to show to the users "what does the library do".

Is ... some part of that API https://github.com/bradleyjkemp/cupaloy/blob/bae07880a7b7e65701a45ecbb32914e6552626a5/cupaloy.go#L64-L66 "convenient" to be implemented in your library?

gkampitakis commented 4 months ago

Yes sorry for the broken md (corrected). Will sumarrise and tell me if I got it right

  1. You would like to have the snapshot files in the same "level" folder as your _test.go file?
  2. You would like to have one file per snapshot without the special delimiters go-snaps is using and probably your own file extension?

For the first one you can do

snaps := snaps.WithConfig(
    // if you want your file to have the same name as your test
    snaps.Filename(t.Name()),
    snaps.Dir("."),
)

snaps.MatchSnapshot

Of course you need to keep in mind that if you change folder go-snaps "will" only search for obsolete tests in the folder it's running.

As for the second point. As said above on file per snapshot is possible, but having the whole snapshot as is (no delimiters) and allowing your own extension would be a bit more complicated. The code does a lot of assumptions that all files .snap are "owned" by go-snaps and can "find" them and delete them and the same thing for the delimiters.

I am inclined to say no, but I would ask first how is this handled on jest?

Are good issues with good insights, but I would need some time to think about.

stdedos commented 4 months ago

Without adding much, tl;dr what https://github.com/jestjs/jest/issues/6383#issuecomment-414072837 is saying is exactly what I'm trying to achieve.

The code does a lot of assumptions that all files .snap are "owned" by go-snaps and can "find" them and delete them and the same thing for the delimiters.

I would not object to tl;dr:

$  tree tests
tests
├── file_based_test.go
├── my_first_test
│   ├ __snapshots__
│   │   └── result.snap.html
│   └── junit.xml -> ../../examples/junit.xml
└── testcase_only_tag
    ├ __snapshots__
    │   └── result.snap.html
    ├── TEST-test.checks.abc.xml
    └── TEST-test.def.xml

Or any other layout/condition you like that would help you continue making assumptions about "files that are yours". I mean, AFAIC, __snapshots__ are yours. Name them __go-snaps__ FWIW 😅

... ofc this extends (significantly?) your API. And I respect if you are not interested to handle it.

... I'm fine "finally" rolling my own solution. I just despise the "nih syndrome".

gkampitakis commented 4 months ago

Reading from the jest issues this sounds a good idea, and something that I would like to support just need to think what would be the best way for being useful, not break the other methods and have similar behaviour.

If keeping the current api it could look like

snaps.MatchFile(t, "<mockdata>")

and by default save the file on __snapshots__ with testname and .snap suffix. The problem with this is if you call it twice inside the same test the last will override the first.

and then you can also do this and save the file wherever you want.

snaps.WithConfig(snaps.Filename(t.Name()), snaps.Dir("."),).MatchFile(t, "<mockdata>")

That could work but

  1. can cause unexpected results ( calling twice the MatchFile inside a test ) - This is a problem because all other methods allow you to call them multiple times.
  2. doesn't allow you to change the file's extension which is something you want - inconvenience or bad devx ? (what else)
  3. not very intuitive

The second api that could be exposed and it's more intuitive is

snaps.MatchFile(t, "<mockdata>", "myfile.txt") 

so it's explicit where this snapshot will be stored. but then makes this useless snaps.FileName.

Or any other layout/condition you like that would help you continue making assumptions about "files that are yours". I mean, AFAIC, snapshots are yours. Name them go-snaps FWIW 😅

For this it's not 100% true, if you can put the snapshots "wherever" you want with snaps.Dir and also have "whatever" extension then you could have something like

- my_code.go
- my_code_test.go
- my_snapshot.html 

is my_snapshot.html a file created from go-snaps or not? there is no "way" you can tell either from name or from directory (if you call a test with it you can know but I am referring to the case of being outdated and orphan).

That's why I will need to do some thinking around the design of it.

How important is the file's extension? (even though your suggestion adding a .snap.html could work)

stdedos commented 4 months ago
  1. can cause unexpected results ( calling twice the MatchFile inside a test ) - This is a problem because all other methods allow you to call them multiple times.

Ofc I cannot talk "general consumption" (where I think you are right), but I am more than happy to comply. "I am hacking the system", I'm ready to pay the price.

2. doesn't allow you to change the file's extension which is something you want - inconvenience or bad devx ? (what else)

Idk if https://htmlpreview.github.io/ will render it? Maybe ofc it does not act "pedantically" (which could stand to reason).

I'm sure that https://github.com/Mottie/GitHub-userscripts/wiki/GitHub-html-preview won't pick it up.

Going by https://github.com/jestjs/jest/issues/6383#issuecomment-414072837 "it won't be picked up by a syntactic parser by itself".

(tl;dr: hopefully not, but not a disaster)

The second api that could be exposed and it's more intuitive is ...

I do support the snaps.WithConfig "callback". Maybe extend it with an .Extension? If not ".snap", then ".snap" + config.Extension?

For this it's not 100% true, if you can put the snapshots "wherever" you want with snaps.Dir and also have "whatever" extension then you could have something like

For me __snapshot__ etc as a dir is acceptable. Again I cannot talk "general consumption", but I'd be happy to have an API that "you can only change the .Directory or .Extension, but not both".


Finally, you can expose your new API under some /exp/ namespace, if it turns out to be a drastic Public API change. See how people work with it. You don't have to violate any backwards compatibility nor make commitments before it's "tried and tested".

stdedos commented 4 months ago

PS: Generate quotes by doing that

image

> Ofc I cannot talk "general consumption" (where I think you are right), but I am more than happy to comply. "I am hacking the system", I'm ready to pay the price.
> 
> > 2. doesn't allow you to change the file's extension which is something you want - inconvenience or bad devx ? (what else)
> 
> Idk if [htmlpreview.github.io](https://htmlpreview.github.io/) will render it? Maybe ofc it does not act "pedantically" (which could stand to reason).
> 
> I'm sure that [Wiki: GitHub html preview (Mottie/GitHub-userscripts)](https://github.com/Mottie/GitHub-userscripts/wiki/GitHub-html-preview) won't pick it up.
> 
> Going by [jestjs/jest#6383 (comment)](https://github.com/jestjs/jest/issues/6383#issuecomment-414072837) "it won't be picked up by a syntactic parser by itself".
> 
> (tl;dr: hopefully not, but not a disaster)
> 
> > The second api that could be exposed and it's more intuitive is ...
> 
> I do support the `snaps.WithConfig` "callback". Maybe extend it with an `.Extension`? If not `".snap"`, then `".snap" + config.Extension`?
> 
> > For this it's not 100% true, if you can put the snapshots "whereve

You don't have to do any of the boring "quoting" work (loving that GH-feature)

gkampitakis commented 4 months ago

Thanks a lot for the very useful feedback 🙇 All are great ideas. I think it's possible and I will play around with this and share some results here.

gkampitakis commented 4 months ago
  1. Supporting a new method MatchStandaloneSnapshot(t, any) 1a. By default saving at __snapshots__/testfileaname_testName_#.snap 1b. Can be called multiple times and can be cleaned up 1c. Sequence number is not related to other snapshot methods
  2. Support overriding the directory as well as the filename 2a. Still will have a sequence number allowing calling multiple times <my-directory>/<my-filename>_#.snap
    snaps.WithConfig(snaps.Dir(<my-directory>),snaps.Filename(<my-filename>)).MatchStandaloneSnapshot(t, any)
  3. Support overriding the extension 3a. Will still have the .snap extension but added the new extension at the end <my-directory>/<my-filename>_#.snap.<my-extension>
    snaps.WithConfig(
        snaps.Dir(<my-directory>),
        snaps.Filename(<my-filename>),
        snaps.Ext(<my-extension>),
    ).MatchStandaloneSnapshot(t, any)

This is what I am having in mind, taking inspiration from https://github.com/jestjs/jest/issues/6383#issuecomment-1652106204, @stdedos wdyt?

Only two small concerns

  1. the default naming can end up be long e.g. running a test from my_long_name_test.go and t.Run("shoud do something") will end as my_long_name_test_should_do_something_1.snap
  2. with the sequencing # adding a number at the end we allow running multiple snapshots from the same test by default but if you remove one the reordering will shuffle the snapshots until you update them (e.g. _1.snap,_2.snap,_3.snap,_4.snap and remove the 3 it will try to match snapshot 4 with the file 3 until you update snapshots) something that already happens now, so might not be a big deal.
stdedos commented 4 months ago

All sounds nice - if you expose the API somehow, I'll try to use it on my branch sometime soon.


  1. the default naming can end up be long e.g. running a test from my_long_name_test.go and t.Run("shoud do something") will end as my_long_name_test_should_do_something_1.snap

That is sad. Only one, small, maybe-theoretical issue, if you are fan of those: https://unix.stackexchange.com/a/32834/ Although how could you overshoot that is beyond me (and probably maybe perhaps user's problem 😅)

2. with the sequencing # adding a number at the end we allow running multiple snapshots from the same test by default but if you remove one the reordering will shuffle the snapshots until you update them (e.g. _1.snap,_2.snap,_3.snap,_4.snap and remove the 3 it will try to match snapshot 4 with the file 3 until you update snapshots) ...

Sounds bad

... something that already happens now, so might not be a big deal.

Or maybe not? ðŸĪĢ

stdedos commented 4 months ago

Idk if htmlpreview.github.io will render it? Maybe ofc it does not act "pedantically" (which could stand to reason).

Yeah, it doesn't care: https://htmlpreview.github.io/?https://github.com/stdedos/junit2html/blob/99504d01ff525decb676484d2c5ba6470f52b522/tests/my_first_test/my_first_test.snap

stdedos commented 4 months ago

idk if this is strictly related to this issue, but for now I'll post it here:

After experimenting a little bit with the code, I arrived here (test/go-snaps/plain-vs-update-clean): https://github.com/stdedos/junit2html/blob/c53a4ec8479d4a2a47702f6eb1a17b3f73fb27ec/tests/file_based_test.go#L79-L102

So I did

go test $(go list ./... | grep -v /junit2html/example)

to get my snapshots up-to-date, and then I was told

$ go test $(go list ./... | grep -v /junit2html/example)
ok      github.com/stdedos/junit2html   (cached)
ok      github.com/stdedos/junit2html/pkg/cmd   (cached)
ok      github.com/stdedos/junit2html/pkg/convert   (cached)
ok      github.com/stdedos/junit2html/pkg/logging   (cached)
ok      github.com/stdedos/junit2html/pkg/parse (cached)
ok      github.com/stdedos/junit2html/pkg/utils (cached)
Seed: 1720431084094005887
--- FAIL: TestSnapshots (0.00s)
    --- FAIL: TestSnapshots/my_first_test (0.00s)
        file_based_test.go:97: 
            - Snapshot - 1
            + Received + 0

            @@ -171,4 +171,3 @@

              </div>
              </div><hr></body></html>
              â†ĩ
            - 

            at ../my_first_test/result.html.snap:2

    --- FAIL: TestSnapshots/testcase_only_tag (0.00s)
        file_based_test.go:97: 
            - Snapshot - 1
            + Received + 0

              â†ĩ
            - 

            at ../testcase_only_tag/result.html.snap:2

FAIL

Snapshot Summary

✓ 3 snapshots passed
✕ 2 snapshots failed

‹ 1 snapshot file obsolete
  â†ģ  â€Ē /home/stdedos/Documents/WorkBulk/Ubuntu/repos/junit2html/tests/my_first_test/error.log.snap

To remove it, re-run tests with `UPDATE_SNAPS=clean go test ./...`

FAIL    github.com/stdedos/junit2html/tests 0.004s
FAIL

So I did that, and now I'm seeing

$ UPDATE_SNAPS=clean go test $(go list ./... | grep -v /junit2html/example)
ok      github.com/stdedos/junit2html   (cached)
ok      github.com/stdedos/junit2html/pkg/cmd   (cached)
ok      github.com/stdedos/junit2html/pkg/convert   (cached)
ok      github.com/stdedos/junit2html/pkg/logging   (cached)
ok      github.com/stdedos/junit2html/pkg/parse (cached)
ok      github.com/stdedos/junit2html/pkg/utils (cached)
Seed: 1720431090742641060
--- FAIL: TestSnapshots (0.00s)
    --- FAIL: TestSnapshots/my_first_test (0.00s)
        file_based_test.go:85: 
            - Snapshot - 2
            + Received + 2

              &fmt.wrapError{
            -     msg: "error decoding xml: EOF",
            -     err: &errors.errorString{s:"EOF"},
            +     msg: "error decoding xml: XML syntax error on line 3: invalid character entity &fmt.wrapError (no semicolon)",
            +     err: &xml.SyntaxError{Msg:"invalid character entity &fmt.wrapError (no semicolon)", Line:3},
              }

            at ../my_first_test/error.log.snap:2

        file_based_test.go:97: 
            - Snapshot - 173
            + Received   + 0

            @@ -1,174 +1 @@

            - <html><head><meta charset="UTF-8"><style>body {
            - ...
            - 

            at ../my_first_test/result.html.snap:2

    --- FAIL: TestSnapshots/testcase_only_tag (0.00s)
        file_based_test.go:97: 
            - Snapshot - 1
            + Received + 0

              â†ĩ
            - 

            at ../testcase_only_tag/result.html.snap:2

FAIL

Snapshot Summary

✓ 3 snapshots passed
✕ 3 snapshots failed

FAIL    github.com/stdedos/junit2html/tests 0.003s
FAIL

I tried alternating between plain and UPDATE_SNAPS=clean commands, but I don't seem to arrive at a "steady state".

Am I hacking the system too much, or is this an actual issue?

gkampitakis commented 4 months ago

Are you testing with code from the draft pr I have or from the main? btw the UPDATE_SNAPS=clean only removes the outdated snaps. UPDATE_SNAPS=true removes outdated snaps but also update the snapshot data as well.

stdedos commented 4 months ago

Are you testing with code from the draft pr I have or from the main?

main. I read your PR, and "it seems incomplete" (I'll wait for your "good enough" signal to start testing)

Are you testing with code from the draft pr I have or from the main? btw the UPDATE_SNAPS=clean only removes the outdated snaps. UPDATE_SNAPS=true removes outdated snaps but also update the snapshot data as well.

I did UPDATE_SNAPS=true go test $(go list ./... | grep -v /junit2html/example), but then tests/my_first_test/result.html.snap becomes empty.

I can for sure tell you that it is not:

image

stdedos commented 4 months ago

... maybe the "mixup" happens because of the

                defer func() {
                    if x := recover(); x != nil {
                        snaps.WithConfig(
                            snaps.Filename("error.log"),
                            snaps.Dir("./"+wd.Name()),
                        ).MatchSnapshot(tt, x)
                    }
                }()

it should just be

                defer func() {
                    x := recover()
                    snaps.WithConfig(
                        snaps.Filename("error.log"),
                        snaps.Dir("./"+wd.Name()),
                    ).MatchSnapshot(tt, x)
                }()
gkampitakis commented 4 months ago

Yes this sounds like a setup issue, if you conditionally call snapshots you are probably comparing different snapshots hence the issue. Did you manage to find the problem, if not and you think it's a bug with go-snaps maybe worth opening another issue to keep this around the "standalone" snapshots.

gkampitakis commented 4 months ago

@stdedos the pr should be in a good state if you want to have a test on it and share some feedback. On my side I want to run some more manual tests in case I missed something but it should be mostly there. Also do you mind if I hide your last three messages as off topic?

stdedos commented 4 months ago

@stdedos the pr should be in a good state if you want to have a test on it and share some feedback.

Sure; I'll try it on soon

Also do you mind if I hide your last three messages as off topic?

Sure - feel free to do maintenance âĪïļ Happy to see someone else takes that kind of stuff seriously