tophat / syrupy

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

Subsequent asserts seem to ignore the matcher #800

Closed michaljelonek closed 11 months ago

michaljelonek commented 11 months ago

Hello! I've been recently exploring this tool and I wanted to create a fixture that I can re-use with my specific matcher, but it seems that subsequent calls in the same test ignore this matcher. I'm not sure if this is intended or not. I wasn't able to find a definitive answer. Apologies if this has been asked before!

To reproduce:

  1. Run the following test with pytest:
    
    @pytest.fixture
    def my_snapshot(snapshot):
    return snapshot(matcher=path_type({"field": (str,)}))

def test_snapshotting(my_snapshot): assert {"field": "string"} == my_snapshot assert {"field": "string"} == my_snapshot


This results in the following snapshot:

serializer version: 1

name: test_snapshotting

dict({ 'field': str, })

---

name: test_snapshotting.1

dict({ 'field': 'string', })

---



**Expected behavior**

I would've expected that both snapshots would compare the type, instead of only the first one is saved with the type and the second one saves the value like the matcher wasn't there at all.

Providing the matcher to both asserts with `my_snapshot` solves this issue, but I don't find this copy-paste feasible too much in a codebase with over a thousand tests. A generic fixture that I define once and re-use would be ideal.

**Screenshots**

<!-- If applicable, add screenshots to help explain your problem. -->

**Environment (please complete the following information):**

- OS: MacOS 13.4.1
- Syrupy Version: 4.4.0
- Python Version: 3.11

**Additional context**

<!-- Add any other context about the problem here. -->
noahnu commented 11 months ago

This is not currently supported, however I agree that there's value in supporting this. Would you be interested in contributing?

When we __call__ the snapshot assertion, all options are "temporary" and restored post assertion, which is why they're not getting persisted. The closest to what you're trying to do is our snapshot.use_extension method which instantiates a new version of the snapshot assertion class with a new extension (like amber/json) persisted. See: https://github.com/tophat/syrupy/blob/11b0722b91c5f0ed9434fa293dc1169088cc00b9/src/syrupy/assertion.py#L168.

There are a couple ways we can support this. We could update use_extension to take in the other parameters we'd want to persist (e.g. matcher, include, exclude). We could introduce a new use method that's more generic that "extension", or perhaps a snapshot.with_defaults to be more explicit.

Logic that resets the props: https://github.com/tophat/syrupy/blob/11b0722b91c5f0ed9434fa293dc1169088cc00b9/src/syrupy/assertion.py#L248 ("with_prop" has a post-assertion cleanup task).

michaljelonek commented 11 months ago

Yes, of course. I'd be happy to contribute.

I see, right, that makes sense to reset post every assertion in case those changes are for just one assertion and that's it. Though, I have to admit, it's not exactly clear while just using the tool that that is the case.

I think extending use_extension makes little sense, since that is very specific to precisely using extensions. As for the other two I have no strong opinions, but given use I think use_extension in that case could be deprecated and dropped later on to have just one method to modify the default behaviour (or well use different values). On the other hand with_defaults wouldn't really make sense to include the extension in there, but given that those could be chained I don't think it would be a big deal, i.e. snapshot.with_defaults(include=..., matcher=...).use_extension(...).

Granted, that could be all under use, but I think that would be less obvious. Then again, I don't really have any strong opinions here, so I'm happy to settle on what you think is best.

Edit: after diving into the code, I think actually that my previous statement that overriding the extension wouldn't fit under with_defaults can be ignored. I think it would fit actually, as it is treated as overriding the default like with any other option. I thought it's a specific case, but it seems, that not really.

michaljelonek commented 11 months ago

I've created https://github.com/tophat/syrupy/pull/802, I'm open to suggestions.

tophat-opensource-bot commented 11 months ago

:tada: This issue has been resolved in version 4.5.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: