[Bug]: Tampering with snapshots and `UPDATE_SNAPS=true go test` does not generate valid snapshot

[stdedos]

Description

I would be expecting that the file would adhere strictly to the

[TestName - Number]
<data>
---
[TestName - Number]
<data>
---
...

format, without any deviations.

(Maybe if you want to allow a // comment above each title, but either way it is not currently the spec)

Steps to Reproduce

1. Using https://github.com/stdedos/junit2html/blob/003747087d56ded6046b114e5c512c699522bc57/tests/file_based_test.go#L76-L105
2. Generate the snapshots (if any are missing - I just took a snapshot of the repo for this bug, YMMV)
3. Open any of them (e.g. https://github.com/stdedos/junit2html/blob/003747087d56ded6046b114e5c512c699522bc57/tests/testsuite_only_tag/__snapshots__/result.html.snap#L2)
4. Remove the first two lines
5. Run UPDATE_SNAPS=true go test $(go list ./... | grep -v /junit2html/example)

Expected Behavior

• Expected behavior: Snapshot is fixed (those two lines are re-added) via "deleting the file" and "re-creating it"
• Actual behavior: A new snapshot is appended, previous snapshot data are not removed

[gkampitakis]

This is not a bug. As you pointed out you are tampering the snapshot file. You can delete it manually and rerun it but the go-snaps can't just blindly delete it and recreate it

1. what if you are not running all the tests? those snapshots won't be recreated.
2. what if there are diffs on the snapshot and the input?

From the point of view of go-snaps the snapshot doesn't exist on the file it adds it. If you run with CI=true (or in a ci environemnt) the go-snaps will fail. So I think it's a good default instead of trying to guess the intent or the state the file should be.

[stdedos]

1. what if you are not running all the tests? those snapshots won't be recreated.

... but it can see that X test's snapshot is getting triggered, right? so X.snap should be "complete + correct" at the end of the test, right? 😕

2. what if there are diffs on the snapshot and the input?

I ... don't fully get what you are saying here 😅

If you run with CI=true (or in a ci environemnt) the go-snaps will fail.

I hear where you are coming from. And maybe also UPDATE_SNAPS=x is warranted (given that, e.g., you don't have the luxury of doing jest -u). In general, though, I am a bit "allergic" to "different experience" between CI and local. If I run the tests locally, and they pass, I would hope is "gg" and CI would serve more or less only as a gatekeeper.

---

We can exchange views ofc - but, if you are saying not-a-bug, I respect it

[gkampitakis]

... but it can see that X test's snapshot is getting triggered, right? so X.snap should be "complete + correct" at the end of the test, right? 😕

Yes but not all of them. There is no guarantee that all the tests inside a snapshot are executed at any time. You can do go test -run TestMyFeature and run only a subset.

In general, though, I am a bit "allergic" to "different experience" between CI and local. If I run the tests locally, and they pass, I would hope is "gg" and CI would serve more or less only as a gatekeeper.

For this case I disagree. You want to make sure your snapshots are "frozen" and won't get updated on your CI and your tests will just continue passing. Something similar to this https://jestjs.io/docs/snapshot-testing#are-snapshots-written-automatically-on-continuous-integration-ci-systems

For me this is expected and desired behaviour. go-snaps can't find the snapshot and adds it. I am surprised it was able to parse the file 😅 But thanks for raising this concern.

[stdedos]

I am surprised it was able to parse the file

What I'm trying to say I guess is: Failing to parse the file / file's syntax being invalid should be an outright error 🙃 (In CI or not)

[stdedos]

I tried (hard) to "override" your ci-detection, but it seems downright impossible to do it from within the test itself (I'd still want to test via go test, and override the CI-check).

However, more and more I come to the realization that my "issue" is more with

Failing to parse the file / file's syntax being invalid should be an outright error 🙃 (In CI or not)

rather than anything else.

---

If we take the unified diff file format as an example: (Using authoritative sources (?): https://www.gnu.org/software/diffutils/manual/html_node/Detailed-Unified.html, https://en.wikipedia.org/wiki/Diff#Unified_format, https://www.artima.com/weblogs/viewpost.jsp?thread=164293)

The unified diff format starts with the +++, --- headers. However, it doesn't say anywhere that the unified diff file starts with the +++, --- headers.

Similarly to your format, if you'd like to allow (but ignore) "top of the file" comments (and/or per-section), of course the way your parsing works right now it does it.

It was just surprising to me 🙃

[gkampitakis]

Thank you for opening the issue, sorry for being surprising but I guess "it's good" it's not breaking and it's a feature rather than a bug 😅 .