joseph-roitman / pytest-snapshot

A plugin for snapshot testing with pytest.
MIT License
117 stars 12 forks source link

Automatic snapshot_dir and output file names #12

Closed lexicalunit closed 4 years ago

lexicalunit commented 4 years ago

Love this package! I've added it to my project's test suite today and it's working great. I did however add a small wrapper to the basic fixture because I found it annoying to have to specify the snapshot_dir and snapshot file name in every single test case.

@pytest.fixture
def snap(snapshot):
    snapshot.snapshot_dir = Path("tests") / "snapshots"
    snap.counter = 0

    def match(obj):
        test = inspect.stack()[1].function
        snapshot.assert_match(str(obj), f"{test}_{snap.counter}.txt")
        snap.counter += 1

    return match

This allows my test cases to look like:

    def test_something(self, client, snap):
        client.on_message(whatever)
        snap(client.last_sent_response)

       client.on_message(whatever)
        snap(client.last_sent_response)

       client.on_message(whatever)
        snap(client.last_sent_response)

And when I run the test suite to generate snapshots I automatically get snapshots laid out like this:

$ tree tests
tests
├── snapshots
│   ├── test_something_0.txt
│   ├── test_something_1.txt
│   └── test_something_2.txt
└── test_mypackage.py
joseph-roitman commented 4 years ago

Thanks :) One of the reasons I made this package instead of using an alternative such as snapshottest is that I didn't like the default locations and filenames for the snapshots.

This package gives the developer complete control over the snapshot locations and filenames. This lets the developer organize the snapshot files in a way that will make the project structure as human-readable as possible which helps when viewing pull requests that change the snapshots.

In the example you gave, it wouldn't be easy to understand which snapshot file belongs to which line in the code. Also, if you were to add a new snapshot in between two existing snapshots, then there would be many snapshot file renames which may be confusing.

I think that the following example would result in easier to understand snapshots and doesn't require much extra work. What do you think?

def test_something(self, client, snapshot):
    client.on_message(whatever)
    snapshot.assert_match(client.last_sent_response, 'response_0.txt')

    client.on_message(whatever)
    snapshot.assert_match(client.last_sent_response, 'response_1.txt')

    client.on_message(whatever)
    snapshot.assert_match(client.last_sent_response, 'response_2.txt')

I am willing to add defaults if they make sense. Perhaps I can add the following default snapshot directories which are very similar to what you suggested.

$ tree tests
tests
├── snapshots
│   └── test_mypackage
│       └── test_something
│           ├── response_0.txt
│           ├── response_1.txt
│           └── response_2.txt
└── test_mypackage.py

I would also have to deal with parametrized tests. In that case, I would probably choose the following directory structure. (Assuming that the test is parametrized with @pytest.mark.parametrize('request_args', ['first_request', 'second_request'])

$ tree tests
tests
├── snapshots
│   └── test_mypackage
│       └── test_something
│           ├── first_request
│           │   ├── response_0.txt
│           │   ├── response_1.txt
│           │   └── response_2.txt
│           └── second_request
│               ├── response_0.txt
│               ├── response_1.txt
│               └── response_2.txt
└── test_mypackage.py
lexicalunit commented 4 years ago

Good points! You're right about it being hard to understand which snapshot file belongs to which line in the code. I discovered this issue the other day when I had to do a big refactor. I might see if I can use inspect to get lines numbers as well (it may be possible, I haven't investigated yet). 🤔 Not sure that would help, but I'll probably give it a try and see how it feels if I can get it working.

And you're right, I wouldn't want this mode of operation to necessarily be the only way. I like the flexibility pytest-snapshot gives to let developers build a tailor made solution for their own use cases.

Your default directory structure idea sounds really awesome too, I'd use that if it were available!

joseph-roitman commented 4 years ago

I'll add the default directories.

I don't think line numbers are a good idea because

  1. They don't describe the contents of the snapshot, which would make reviewing pull requests hard.
  2. Line numbers will change any time you add/remove a line from the test file. This would cause many snapshot file renames.
joseph-roitman commented 4 years ago

I added default snapshot directories v0.4.0. Feel free to update :)

githorse commented 3 years ago

@lexicalunit I dug the shorthand of your snap() function. I modified it slightly to

  1. remove the redundancy of test_ everywhere
  2. assume a single snapshot per test, thereby eliminating the need to pass in a name for it.

I prefer smaller, targeted tests, so giving up multiple snapshot names per test is not a handicap for me. But obviously multiple snapshots per test is the more general solution. @joseph-roitman Maybe it would be cool to allow both somehow? Basically, default to a snapshot named after the test if that argument is left out of assert_match.

At any rate, here's my code for any interested parties:

def snap(snapshot, request):

    def match(obj):
        test_file_name = request.node.fspath.purebasename
        snapshot_dir = re.sub('^test_', '', test_file_name)
        full_snapshot_path = Path('test') / 'snapshots' / snapshot_dir

        test_name = inspect.stack()[1].function
        snapshot_name = re.sub('^test_', '', test_name)

        snapshot.snapshot_dir = full_snapshot_path
        snapshot.assert_match(str(obj), f"{snapshot_name}.txt")

    return match

So for a test like:

// test/test_pizza.py

def test_crunchy_crust(snap, pizza):
    snap(pizza.crust)

def test_melted_cheese(snap, pizza):
    snap(pizza.cheese)

I get fixtures:

test
└── snapshots
    └── pizza
        ├── crunchy_crust.txt
        └── melted_cheese.txt

2 directories, 2 files

Actually, my real code is slightly different because I have even more repetition across my test file names and test names, so I use a different regex to transform test file name and test name into the snapshot path. I've omitted that here so as not to confuse the issue. It would be nifty (but perhaps overkill) to maybe provide some infrastructure / hooks for users to programmatically transform the test path into the snapshot path. Some global option you could put in conftest.py:

def transform(test_file_path, test_name) -> str:
    return Path("snapshots") / re.sub('test_', '', test_file_path) / test_name

snapshot.snapshot_path_transform = transform
joseph-roitman commented 3 years ago

@githorse Your first suggestion is interesting, but I have a few small problems with it:

  1. You added the .txt file extension. This is problematic because people snapshot PNGs, PDFs, etc. There should be a way to control the file extension.
  2. You suggested removing the test_ prefix from snapshot names/dirs. While it seems like this is possible because pytest requires all tests to start with this prefix, there is two problems
    • This is not backwards compatible with the previous behavior of the library (default snapshot dirs)
    • This could cause confusing snapshot names. I would be confused if I saw files named system, eq, iter. On the other hand, if I saw test_system, test_eq, test_iter, I would understand where they came from.

Here are my suggestions, what do you think about them? Instead of using snapshot.assert_match(obj, 'snapshot.txt') and getting

tests
├── snapshots
│   └── test_mypackage
│       └── test_something
│           └── snapshot.txt
│       └── test_something_else
│           └── snapshot.txt
│       └── test_another_thing
│           └── snapshot.txt
└── test_mypackage.py

It would be simplified to snapshot.assert_match(obj, '*.txt') and getting

tests
├── snapshots
│   └── test_mypackage
│       └── test_something.txt
│       └── test_something_else.txt
│       └── test_another_thing.txt
└── test_mypackage.py

This solution makes the user choose the correct file extension. I added * at the beginning so as not to cause problems for users that want to create snapshots starting with a period. (e.g. .config). Alternatively to *.txt, we could use {test_name}.txt which is more explicit and doesn't take up * which could be used in some other future feature.

In regards to your second suggestion about a hook to set the snapshot_dir based on the path/file/test names of the tests, I think that it is too complex/generic of a feature. It would need to be something very simple to explain in the README and useful for everyone. Also, you don't need this feature from pytest-snapshot. You could implement this using any of pytests available hooks. Off the top of my head you could do something like this

@pytest.fixture(autouse=True)
def set_snapshot_dir(snapshot, request):
    snapshot.snapshot_dir = request.node.name