Crash with SingleFileSnapshotExtension

[rotu]

trafficstars

Describe the bug

SingleFileSnapshotExtension crashes when trying to serialize a Dict with a number value.

To reproduce

run pytest --snapshot-update on a file containing the below test:

from syrupy.extensions.single_file import SingleFileSnapshotExtension
from syrupy.extensions.amber import AmberSnapshotExtension
def test_foo(snapshot):
    value = {"x": 0}
    # this works:
    assert value == snapshot(extension_class=AmberSnapshotExtension)
    #this does not:
    assert value == snapshot(extension_class=SingleFileSnapshotExtension)

Expected behavior

The test should generate a snapshot file without errors.

Screenshots

>       assert value == snapshot(extension_class=SingleFileSnapshotExtension)
E       assert [+ received] == [- snapshot]
E         TypeError: 'str' object cannot be interpreted as an integer
E         Traceback (most recent call last):
E           File ".../site-packages/syrupy/assertion.py", line 268, in _assert
E             serialized_data = self._serialize(data)
E                               ^^^^^^^^^^^^^^^^^^^^^
E           File ".../site-packages/syrupy/assertion.py", line 181, in _serialize
E             return self.extension.serialize(
E                    ^^^^^^^^^^^^^^^^^^^^^^^^^
E           File ".../site-packages/syrupy/extensions/single_file.py", line 52, in serialize
E             return self.get_supported_dataclass()(data)
E                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

test_x.py:24: AssertionError

Environment (please complete the following information):

• OS: macOS 13.5
• Syrupy Version: 4.0.2
• Python Version: CPython 3.11.3

[noahnu]

The single file snapshot extension is meant for string or byte data (like images). If you want to store a dict, you'll have to encode it somehow. Alternatively you can use the JSON extension which extends the single file extension.

[rotu]

The single file snapshot extension is meant for string or byte data

Gotcha. That makes sense, though it wasn't obvious to me. Here are a few places that I think this should be fixed:

The documentation makes it sound like the difference is just whether it creates one or multiple files:

• AmberSnapshotExtension: This is the default extension which generates .ambr files. Serialization of most data types are supported. ... SingleFileSnapshotExtension: Unlike the AmberSnapshotExtension, which groups all tests within a single test file into a singular snapshot file, this extension creates one .raw file per test case.

It should probably throw a more informative error message than "'str' object cannot be interpreted as an integer"

And assert {} == snapshot(extension_class=SingleFileSnapshotExtension) should probably fail with the same error that assert {'x':1} == ... does.

[noahnu]

The latter is python just doing an automatic coercion. We don't have any sort of type checking or validation of the value being snapshotted. I can definitely see a use case for some sort of extension specific validation rules. Could likely solve the more detailed error message at the same time.

Regarding documentation, are you interested in putting up a pull request?

[rotu]

After sleeping on it, I still think it's pretty unintuitive that "SingleFileSnapshotExtension" doesn't accept objects. I.e. I'd expect SingleFileSnapshotExtension to use the same SnapshotSerializer implementation but differ in implementation of SnapshotCollectionStorage.

---

I think the automatic coercion is inappropriate. The serialize function (which is documented in the base class as always returning a string (?)), behaves as either str(data) or bytes(data). I think the intent is better served with:

def serialize(self,data):
  if this._write_mode == WriteMode.TEXT:
    if not isinstance(data, str): raise TypeError(...)
    return data
  else:
    return bytes(memoryview(data))

https://github.com/tophat/syrupy/blob/33693162292af47d7662cb2ea9b930f54973d6da/src/syrupy/extensions/single_file.py#L45-L52

[noahnu]

how do you convert something like a dict to bytes?

[rotu]

how do you convert something like a dict to bytes?

It depends.

1. If the expectation here is to save the buffer byte-wise, then it ought to throw a TypeError if given a non-bytes-like object.

2. If the expectation here is to actually serialize an object, then you pickle it.

Doing a straight bytes function produces flat-out wrong results in the case of e.g. {} or {1:"foo"}. That's why memoryview - it produces the semantically correct TypeError.

[noahnu]

It'd be fairly straightforward to add a version of the amber serializer that generates a file per test case. You can write a custom extension for this. In fact, I'd welcome that extension to be contributed upstream to syrupy itself.

I acknowledge that the name can be a bit misleading considering the default serializer is the amber serializer. I'd consider the single file extension to be a base serializer used to create other serializers.

Changing this behaviour (beyond just improving the error message) would be a breaking change which I'd prefer not to make in this case.

For improving the error message and documentation, contributions are welcome.

[StephanMeijer]

I would like to contribute to this, as it helps our issue to be solved as well: https://github.com/tophat/syrupy/issues/837