Snapshot of directory content

[alfechner]

Hi,

we've got a tool which generates a bunch of files in a nested directory structure. We test the tool by snapshotting the entire directory and save it in exactly the same structure for comparison. Would you be interested in a PR to incorporate this peace of functionality?

Cheers,
Alex

[joseph-roitman]

Hi,
That sounds like a great feature that would fit nicely in pytest-snapshot :)
What API did you think of?

Here are some requirements I thought of:

1. We should be able to snapshot both directories and files.
2. Updating the snapshot should respect the --allow-snapshot-deletion flag.
3. Binary files should be supported (don't use text diffs by default)

Other thoughts:

1. How should the code behave if it encounters a symlink in Windows / Linux? In my opinion the ideal behavior is to copy gits behavior (the snapshot symlink should be identical to a symlink that git clone would generate). I don't know how complex that is to implement. I would be OK with just not supporting symlinks for now.
2. I'm pretty sure that hard links would work as expected without any special handling.
3. Do we want to support snapshotting very large directories/files that don't fit in memory? I don't think its worth the added complexity for now. It would probably be a bad idea to create such large snapshots anyways.

Here are some possible APIs I thought of (feel free to suggest better names):

Extend assert_match_dir

We could extend assert_match_dir to support nested dictionaries.
Then your use case would be supported by calling

from pytest_snapshot import load_filesystem_as_dict
d = load_filesystem_as_dict('path/to/generated/directory/or/file')
snapshot.assert_match_dir(d, 'my-snapshot-name')

This is nice because it makes assert_match_dir much more powerful.
load_filesystem_as_dict doesn't have to be part of pytest_snapshot but since this is such a useful use case it could be.
This is a bit verbose, so perhaps one of the next solutions could be a sugar for calling this.

Adding support for registering custom snapshot types

Other people have asked for similar functionality. We could support

from pathlib import Path
snapshot.register_type_snapshot(Path, load_filesystem_as_dict)
snapshot.assert_match(Path('path/to/generated/directory/or/file'), 'my-snapshot-name')

I wouldn't want to call register_type_snapshot on Path by default, because that behavior is surprising.
To avoid forcing the user to call register_type_snapshot on Path , we could add a special class for your use case:

from pytest_snapshot import Filesystem
snapshot.assert_match(Filesystem('path/to/generated/directory/or/file'), 'my-snapshot-name')

It is a bit annoying that you need to import pytest_snapshot and its not obvious what is happening without reading the docs.

New function

The benefit here is that it is pretty straightforward what is happening, but the downside is that we are adding another method. Are we going to start adding more and more methods as time goes on?

snapshot.assert_match_filesystem('path/to/generated/directory/or/file', 'my-snapshot-name')

What do you think?

I am leaning towards extending assert_match_dir, adding register_type_snapshot, and maybe also Filesystem.

[alfechner]

Cool that you're so engaged, thanks for spending the time.

The Histroy behind this Request

My first naive approch was exactly what you suggested. I iterated my files, built up that directory and realized, that assert_match_dir doesn't support relative path's as filenames.

For some time we just replaced the / in the path's with _. That worked but with a growing directory strutcture it became cumbersome to assess the correctness. We want to see the structre as it would turn out in real.

So I changed the approach: Now we iterate over the directory and use assert_match for every single file. The downside is, that --allow-snapshot-deletion is missing obviously.

Proposel

I wold keep it simple and just support nested directories in assert_match_dir. As a user I kind of expected this behaviour when I used the function the first time. It felt natural to me that the file names would be relative to the directory specified in the second parameter of assert_match_dir.

The register_type_snapshot is nice but adds complexity to the API. Also a file path isn't that different from a filename. It's feels strange for me from the perspective of a user that I don't need to register anything for a filename but for a file path. I guess 'filename' is often used interchangable for 'relative filepath'.

I wouldn't include load_filesystem_as_dict. First to keep the package lean. Second I learned that building up that dict can be pretty spcial to the project. Take the following examples:

1. Our project is a boilerplate generator. Something like Create React App for our own project. Basically it takes a bunch of Jinja templates and renders them to a project directory. One of those files is .gitignore.jinja. It's rendered to .gitignore. The snapshot was then called .gitignore which caused some issues. Other snapshot were ignored by git because of the project's own .gitignore file. Therefore we suffixt our snapshot files with .snapshot (Jest uses the suffix .snap).
2. The project initilaizes a git repository. We needed to exclude the files in .git from the snapshot dict.

My point is that load_filesystem_as_dict would need to support many many use cases we don't even now yet.

Bottom Line

I'd just extend assert_match_dir to be able to handle relative path's or as you expressed it: support nested directories.

• The API doesn't change
• The behaviour feels natural and is actually expected
• No additional complexity
• Full flexibility
• Least effort

WDYT?

[joseph-roitman]

I agree :)
I also thought of other special edge cases in load_filesystem_as_dict which could have caused problems. For example, some files might be textual and some might be binary.

What do you feel about relative paths vs nested directories?
I think nested dictionaries is the more "default" method.

Do you want to implement it? It also needs a test case.

I could implement it in the coming week or two when I have some spare time.

[alfechner]

Binary files.. good point, yes.

For me the file names in the directory are relative paths. The snapshot will be created at the file names position relative to the snapshot directory, right? So a nested directory for me is basically a filename with a directory part, i.e. my-dir/my-file.txt.

I was wondering about the following case.

Given the following code:

dir = {
  "one.txt": "...",
  "my-dir/my-file.txt": "...",
  "/another-dir/another-file.txt": "..."
}

snapshot.match_dir(dir, "my-snapshots")

What snapshot files would you expect to be created?

The file names one.txt and my-dir/my-file.txt are relative while /another-dir/another-file.txt is an absolute file path.

Would you still expect the following snapshot files created?

1. my-snapshots/one.txt
2. my-snapshots/my-dir/my-file.txt
3. my-snapshots/another-dir/another-file.txt

Or would you expect an error?

I'd happy to give it shot but I cannot promise when I'll be able to do so. My driver is that we need this feature in our current project :)

[joseph-roitman]

In your case I would expect an exception.

So you prefer paths instead of nested dictionaries?
I understand that in your case it would simplify your load_filesystem_as_dict a bit.
But there are two problems:

It could cause problems if we were to support nested dictionaries in the future. Someone could send:

{"a/b.txt": "value1",
 "a": {"b.txt": "value2"}}

But I guess that is an edge case which we could throw an exception on.

The main downside is that someone might accidentally have a path separator in their snapshot names.
First of all, they would get a nested directory, which would be surprising.
Also, r'a\b' would create a nested directory on windows but not on linux.

So I think it would be best to support only nested dictionaries.
Do you want to implement it yourself? If not I can probably implement this on the weekend. Supporting only nested dictionaries should be pretty simple to implement.

[alfechner]

I see. I din't think of the implication of path's as string on i.e. Windows users. Very good point.

Also I didn't consider this nested dict structure. I like it. Creating the dict via i.e. os.walk() will be pretty simple this way. Also we don't have the issue with the absolute path case I mentioned above.

What is the current behaviour if a user passes a dict as a value? Is it stored in the file? If yes the nested dict structure you proposed is a change in behaviour.

I don't think I'll find time before next week, so I'd be happy if you want to start on it. If not I can jump in next week or the week after.

[joseph-roitman]

What is the current behaviour if a user passes a dict as a value?

That's a good question. The current behavior is to raise an exception (only str and bytes are supported) so it isn't a breaking change.

The library might in the future add support for snapshotting other types (as we discussed previously).
You are correct to point out that assert_match_dir would never be able to support snapshotting dict values after this change.
I think that is an OK tradeoff since there would be an easy workaround in that case, and your useful use case is currently impossible with the current API.

So the solutions are:

1. nested dicts - Downside: Cant add future support for snapshotting dict values in assert_match_dir.
2. Flat dict with path keys - Downsides: Accidental nesting may occur, Windows/linux discrepencies.
3. Both - Downsides: All previous downsides + invalid inputs + much more edge cases and hard code.

I think (1) is ideal.

[alfechner]

I'd vote for (1) as well.

If you want to support further types in future you might consider having Directory an actual type.

class Directory(dict):
    pass

dir = Directory()
dir['file_in_dir.txt'] = "..."

dict = {
  'my-dir': dir
}

snapshot.math_dir(dict, "my-snapshots")

[joseph-roitman]

That is an option but it feels overkill. I'd like simple dict expressions to work

snapshot.assert_match_dir({
   'people': {...},
   'another_dir': {...}}, 'my-snapshot')

instead of writing a paragraph in the docs of how to use assert_match_dir and why it is so confusing.

[alfechner]

I agree, I would rather not support other types and go with a "it's either a string (file content) or a dict (directory) policy". That's simple and consistent. The error raised for an invalid value type could even contain this information...

Great discussion, nice outcome, thanks for your efforts.

[joseph-roitman]

That is my current philosophy though someone might bring a convincing argument to implement something like register_type_snapshot in the future.

Thank you for bringing up this idea.

[joseph-roitman]

I implemented the nested dictionaries in v0.8.0. Update me if you have any problems.

[alfechner]

You sir, are a here.

I integrated it and it works like a charm. Thank you so much 😃