search

tophat / syrupy

:pancakes: The sweeter pytest snapshot plugin
https://tophat.github.io/syrupy
Apache License 2.0
490 stars 33 forks source link

Store snapshot assertion index in snapshot file #569

Open noahnu opened 2 years ago

noahnu commented 2 years ago

We had to wrap custom names in square brackets because our "does assertion name match name in file" logic is brittle. See: https://github.com/tophat/syrupy/pull/563 and related PR.

iamogbz commented 2 years ago

Damn this was missed in the v2 release, could probably have merged both number and string indexes to use the same pattern e.g # name: TestClass.test_class_method_name[1]

iamogbz commented 2 years ago

Maybe a larger change (since it's gonna be breaking) would be to split the index from the name in the snapshot header e.g.

# ---
# name: TestClass.test_class_method_name[parameterized1]
# index: 1
iamogbz commented 1 year ago

Hey 👋🏾 trying to figure how this can work with the current state of things, would it require a new snapshot file version, how does it work with TaintedSnapshotError? Catching up on the code base, but comments would be appreciated.

Also don't see any more tasks queued in project/up next, so prioritising the oldest/easiest issues.

noahnu commented 1 year ago

I'll try find some time towards the end of this week to do some general project maintenance (comments + prioritized backlog).

Will give a longer answer when at my computer but in general yes, we'd need a new serializer version. A new serializer version will automatically leverage the tainted snapshot error to force snapshots to be considered "invalid" and require updates. This is necessary because adding additional metadata around a snapshot, e.g. index, does not actually cause a snapshot inequality/failed match. So as long as you change the version, you don't have to worry about the taint error (it's automatic).

If you're interested, I think the old issue around supporting extremely large snapshots would be a good one to pick up.

iamogbz commented 1 year ago

I think this would need to be implemented with some backwards compatibility even if the version is incremented so that snapshot discovery can still be done on the old versions while issuing the warning. Except it's a straight up breaking change? Also thinking of picking this up now since the other todos are non blocking, thoughts?