Add key keyword argument to `assert_match`

[underyx]

Heya, awesome pytest plugin! I would suggest adding a key keyword arg which accepts a function that the actual and expected values both pass through.

Simple example

I want to test if my random data dump function outputs the same amount of data as the last time I snapshotted it. snapshot.assert_match(dump(), "random.dat", key=len) will save a snapshot of the data the first time we run it, and always compare just the lengths. I propose that --snapshot-update will then update the snapshot file only if the assertion fails.

Real-world example

I want to test if semgrep always outputs the same results when the CLI is invoked with the same arguments. I save JSON files, but the order of results is not deterministic. So I'd like to use snapshot.assert_match(run_semgrep_in_tmp("r2c"), "results.json", key=parse_json), where parse_json is responsible for turning a JSON list into a Python set, thereby satisfying equality. This would have the added benefit of more useful pytest error messages when a test fails, as it could tell us exactly which JSON key changed and how.

[joseph-roitman]

Thanks :)

Snapshot testing works best when updates to snapshot files are small which makes reviewing diffs easier. In your example, any time you would need to update the snapshot, the file would completely change. This would make CR hard since the whole file would need to be reviewed again.

It would be better to make the object you snapshot deterministic. In your case, couldn't you do this by taking the set s and snapshoting sorted(s) or sorted(s, key=repr)?

I also think that the snapshot file should be the exact output of the code, so snapshot.assert_match(dump(), "random.dat", key=len) would be misleading to developers that look at the snapshot file and assume that it contains the correct output.

The key function could still be used for more useful pytest error messages when a test fails. Do you have any examples of when this would be significantly better than seeing the textual diff in the snapshot file? The snapshot file should be human-readable to allow for easy CRs.

[underyx]

Thank you for such a detailed response!

I concede, the random.dat example was quite silly.

Taking the set and snapshotting sorted(s) is quite complicated, as it's deep inside a JSON object in one of our snapshots — the rules key points to a logical set, the order of the rules is arbitrary and it cannot have duplicates.

So for the above mentioned use case 1 I could add some sorting code in the application itself, but I'd rather not, as it's unnecessary code to maintain in our releases. I did instead add this code in the test, but I'm a bit unhappy about having to manage this code inside our test function code. I think it'd be more elegant if I could tell pytest-snapshot how to make the snapshot file deterministic.

Use cases 2 and 3: Pretty-printing and replacing some JSON keys with <masked in tests>. I think it'd be nice if I could do all this via a snapshot(key=...) as well. Then I'd maybe create an own fixture for it like so:

@pytest.fixture
def json_snapshot(snapshot):
    return partial(snapshot, key=_clean_json)

Use case 4: I'd use a key function for output snapshots that print tracebacks, to mask the code file paths. Currently we marked this traceback snapshot as expected failure.

[joseph-roitman]

I encountered many of the same problems you mentioned, so it is a recurring theme of snapshot testing. In my case, I was snapshot testing the contents of a mongodb collection. To allow for snapshot testing I needed to do the following:

• When the test queried the DB, I added sorting.
• The documents contained random-like data in certain keys, which I manually masked as you did by replacing there content with 'NORMALIZED'.
• The documents contained datetimes in various places, so I made a recursive function that masked them with the string 'NORMALIZED' (instead of manually masking them).

I wrapped this up in the function normalize_cars so my test looked something like

cars_list = db.cars.find({...})
snapshot.assert_match(normalize_cars(cars_list), 'cars.json')

I think this is an elegant solution since it is very strait forward what is going on, and the snapshot object does not have to deal with normalization at all.

I do not see the benefit in writing the line as

snapshot.assert_match(cars_list, 'cars.json', normalize=normalize_cars)

If you really wanted to you could further simplify the line by creating the fixture assert_cars_match so the line would be

assert_cars_match(cars_list, 'cars.json')

but I think this may be overkill.

json_normalizer Since snapshot testing a json-like object seems to be a recurring theme, perhaps I could add a helper function which generates a normalizing function, so in the example above, instead of writing the code for normalize_cars, it could be something like

normalize_cars = pytest_snapshot.json_normalizer(
    type_replace={datetime.datetime: 'NORMALIZED'}
    json_path_replace={'cars[].details.random_guid': 'NORMALIZED'}
    json_path_sort={
        'cars': lambda cars: sorted(cars, key=lambda car: car['name'],  # or maybe the next line?
        'cars': lambda car: car['name'],
    }
)

I am not sure if this is more readable compared to implementing the function by hand. What do you think?

traceback snapshot testing I think snapshot testing a full stack trace is a bad practice, the files/line numbers/code lines in your code and other libraries changes all the time so the snapshot would only work for very specific library versions.

On the other hand, it is a good idea to test error messages of exceptions. In most cases this should be done using

with pytest.raises(ValueError, match=r".* 123 .*"):
    myfunc()

as described in the docs.

I can see why snapshot testing can sometimes be useful for testing large error messages With the current code, you could do it with this code which is pretty awkward

def my_test(snapshot):
    try:
        my_code_that_should_raise_something()
    except Exception as e:
        snapshot.assert_match(f'{type(e).__name__}: {e}', 'error.txt')
    else:
        raise AssertionError('The tested code was supposed to raise an exception but did not.')

Perhaps I could add the context manager raises_assert_match which would behave like like this:

def my_test(snapshot):
    with snapshot.raises_assert_match('error.txt'):
        my_code_that_should_raise_something()

In this code, there is nowhere for the user to normalize the error message. Since the error message is always a string, the following could be used to normalize the message in most (but not all) cases

def my_test(snapshot):
    with snapshot.raises_assert_match('error.txt', replace={some_string: 'NORMALIZED'}):
        my_code_that_should_raise_something()

What do you think about all this?

[joseph-roitman]

I looked at your code and saw that you used a more elegant solution than raises_assert_match:

with pytest.raises(MyException) as excinfo:
    my_code_that_should_raise_something()
snapshot.assert_match(str(excinfo.value), "error.txt")

The user needs to learn one less function, and it allows the user to normalize the message if required. Maybe I should document this use case in the README?

[joseph-roitman]

I will close this issue for now, feel free to reopen if you think there is still something to discuss.