Overwriting dirname causes test suite not to fail if snapshots unused

[joostlek]

Describe the bug

Within Home Assistant we have a SnapshotExtension to overwrite the default __snapshots__ dir name with snapshots. But today I actually noticed that this had the side effect that it doesn't fail the test suite if there are unused snapshots in the snapshots folder.

class HomeAssistantSnapshotExtension(AmberSnapshotExtension):
    """Home Assistant extension for Syrupy."""

    VERSION = "1"
    """Current version of serialization format.

    Need to be bumped when we change the HomeAssistantSnapshotSerializer.
    """

    serializer_class: type[AmberDataSerializer] = HomeAssistantSnapshotSerializer

    @classmethod
    def dirname(cls, *, test_location: PyTestLocation) -> str:
        """Return the directory for the snapshot files.

        Syrupy, by default, uses the `__snapshosts__` directory in the same
        folder as the test file. For Home Assistant, this is changed to just
        `snapshots` in the same folder as the test file, to match our `fixtures`
        folder structure.
        """
        test_dir = Path(test_location.filepath).parent
        return str(test_dir.joinpath("snapshots"))

To reproduce Add the SnapshotExtension. Have it generate a snapshot. Add a fake entry to the snapshot. Run the test suite.

Expected behavior

I'd expect it to fail the test suite because there are entries that are not found. I can imagine that we have quite a basic way of overwriting the directory and can imagine people with way more interesting setups for the snapshots which would probably break with the expected behaviour. So I would not be surprised if the failure is turned off when the dirname is overwritten, but I'd then at least expect to read something in a docstring or in the docs.

Screenshots

Environment (please complete the following information):

• OS: mac OS
• Syrupy Version: 4.6.1
• Python Version: 3.12

Additional context

[noahnu]

Ah this is an old issue. I'd definitely like to fix this.

I think this should be doable by updating the walk_snapshot_dir function to accept a dirname, then pass it along in discover_snapshots here: https://github.com/syrupy-project/syrupy/blob/d3f891ea4e561cd1b182e9b2c5d0414821187cd7/src/syrupy/extensions/base.py#L117

For reference, the walk_snapshot_dir function: https://github.com/syrupy-project/syrupy/blob/64b42653d1c3af5b56347ccd9afd24e87b29aa18/src/syrupy/utils.py#L40

[iamlucasvieira]

I've never contributed here before. Would be willing to pick it up.

[noahnu]

That'd be great! Let me know if you have any questions

[iamlucasvieira]

I wonder if we need the not in_snapshot_dir check.

Because, in discover_snapshots we already pass the location including the SNAPSHOT_DIRNAME through self.dirname:

for filepath in walk_snapshot_dir(self.dirname(test_location=test_location)):

So every file that we find in this walk, is always within the SNAPSHOT_DIRNAME(That is also the case for a custom self.dirname.

I removed this check and all tests passed expect test_walk_dir_skips_non_snapshot_path, which is expected, as we pass to walk_snapshot_dir a path outside the SNAPSHOT_DIRNAME

What do you think?

[iamlucasvieira]

Add the SnapshotExtension. Have it generate a snapshot. Add a fake entry to the snapshot. Run the test suite.

I also tested this using the default snapshots dir and tests do not fail when I added a fake entry

[noahnu]

Hmm, yeah I think it can be removed. Feel free to put up a pull request that removes it and updates the test case. I'd also add a test case that covers what @joostlek posted in the original post

[joostlek]

Any progress on this? I also wanted to look into the issue, so if I can pick it up, feel free to let me know :)

[iamlucasvieira]

You can pick up!

[joostlek]

Do you have a branch somewhere with what you have?

[iamlucasvieira]

The only change I made in the code was removing the check if file is inside SNAPSHOT_DIRNAME (which is a constant with the default dirname). I concluded this check is not needed. I was then not able to replicate the issue you had after changing that.

I removed this check and all tests passed expect test_walk_dir_skips_non_snapshot_path, which is expected, as we pass to walk_snapshot_dir a path outside the SNAPSHOT_DIRNAME

[noahnu]

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

The release is available on:

• v4.7.2
• GitHub release

Your semantic-release bot :package::rocket: