feat: support labeling snapshots

[G-Rath]

Resolves #151

[G-Rath]

Ok I think this is good to go - I've opened google/osv-scanner#2538 to showcase it working, though note I didn't run our entire test suite with labels.

Interestingly, that PR also seems to have shown a bug that this might have been fixed?

[G-Rath]

I've realised it would probably be useful to also have the labels outputted in the diff, but I'll wait until @gkampitakis has had a chance to review before exploring that

(it might be best to land as a new PR 🤔)

[gkampitakis]

Hey 👋 sorry for not being very engaged. I wll try to review all open issues and prs by the end of week. Again thanks for all the work and help 🙇

[G-Rath]

@gkampitakis no worries, there's no particular rush - it would be nice to know though if you're happy with the direction this is going in before I spend anymore time, but if you're busy you're busy 🙂

[gkampitakis]

Is there a reason the label needs to be at the end of the testID? We have a lot of places where we assume the that the id ends in a number.

The reason I am asking is, wouldn't it be easier in terms of code changes and complexity to have something like this?

[TestCommand/no_lockfiles_without_recursion_but_with_allow_flag_are_fine - label - 1]

Also I am assuming Label is a noop for StandaloneSnapshots correct?

[G-Rath]

Is there a reason the label needs to be at the end of the testID?

Because it's optional so having it between two required fields makes it more ambiguous when parsing, especially as you can have a number as the label - so is [x - 1] a malformed test id with a label 1, or the second snapshot of test x?

Practically though its because imo it gives a nicer layout, consider:

[TestCommand/no_lockfiles_without_recursion_but_with_allow_flag_are_fine - 0]

[TestCommand/no_lockfiles_without_recursion_but_with_allow_flag_are_fine - stdout - 1]

[TestCommand/no_lockfiles_without_recursion_but_with_allow_flag_are_fine - stderr - 2]

vs

[TestCommand/no_lockfiles_without_recursion_but_with_allow_flag_are_fine - 0]

[TestCommand/no_lockfiles_without_recursion_but_with_allow_flag_are_fine - 1 - stdout]

[TestCommand/no_lockfiles_without_recursion_but_with_allow_flag_are_fine - 2 - stderr]

We have a lot of places where we assume the that the id ends in a number.

I'd say those places should all be using a single parseTestId type function that'd give a structured result (i.e. testName, testId, testLabel), to avoid exactly that kind of brittleness 🙃

(so far though, I've only found two places where that is actually happening?)

Also I am assuming Label is a noop for StandaloneSnapshots correct?

umm yeah, let's say yes at least for now 😅

I've not actually used standalone snapshots - I think ideally the label would be used in their name, though that'd require removing or replacing any characters that'd be invalid for filenames, but that's not too hard.

Overall, I think that's something that can be easily added as a follow-up

[gkampitakis]

That makes sense. Will have a more thorough review and probably approve later today or tomorrow if everything looks good.

Again thanks for the pr 😄

[gkampitakis]

I had a look on the pr. I think it mostly looks good. Also I think we can simplify the logic here https://github.com/G-Rath/go-snaps/blob/8594986f82173b23234f450e8c5e1507119be0b3/snaps/snapshot.go#L156-L181 and drop the

s.cleanup[snapPath] = make(map[string]int)

and instead use an s.testIDRegistry that will hold all the testIDs for the clean up later. https://github.com/G-Rath/go-snaps/blob/8594986f82173b23234f450e8c5e1507119be0b3/snaps/clean.go#L271-L294 so here we have only the one check you have added and drop building the https://github.com/G-Rath/go-snaps/blob/8594986f82173b23234f450e8c5e1507119be0b3/snaps/clean.go#L264-L265 occurrences dynamically ? 🤔

[G-Rath]

instead use an s.testIDRegistry that will hold all the testIDs for the clean up later

Yeah I feel overall like it might be possible to simplify some of the code, though probably best to do that in its own PR unless I'm misunderstanding what you're suggesting.

---

fwiw if you're interested in my thoughts, my current thinking would be to explore having a "snapshot manager" per file, which handles most of the logic, and let you store snapshot info easier e.g.

• the first time any action involved snapshots would trigger a parse of the file
• getPrevSnapshot would trigger a parse if needed, then just return the already-known data, and also mark the snapshot as "used"
• addNewSnapshot would add a new snapshot into the struct
• SnapshotManager.save would write everything to disk
• removing obsolete snapshots would be a case of removing any snapshots where !SnapshotManager.snapshots.Used (with accounting for -run of course)
• sorting would be a case of doing SnapshotManager.snapshots.sort
• etc etc

I think the main downsides with that are

• potentially more work upfront re "parsing" (but in practice I don't think it'd be that expensive, plus it would save re-opening the file for every snapshot)
• more memory because all the snapshots would be being stored in memory for longer (but in practice I would expect that to be pretty small... and there could be ways to optimize that?)
• snapshots wouldn't be written to disk immediately (which might be useful in some cases, but also you could have this by just calling save more often?)

That's just a back-of-napkin idea so it might not actually be a good one 😅

Let me know if that sort of change was what you were meaning, or if I've missed a mark and there's a far smaller one you had in mind

[gkampitakis]

@G-Rath The suggestion that I had in mind was a bit simpler. Your idea is interesting, we could explore it but that would need a lot of work and the past 6 months unfortunately I don't have the time to invest on it. For the parsing inline snapshots work in a similar way were we parse the file once.

In any case I will review your pr as is for now to unblock it.

[gkampitakis]

PR looks good to me to be merged. Can you please add the new config on the README.md?