Allow fixing snapshots without disabling pytest rewriting

[alexmojaki]

Yesterday a teammate asked if --inline-snapshot=fix could be enabled by default. I also think I'd quite like this. But it seems that the main problem is that doing so disables pytest rewriting, making assertions worse even in tests that aren't using snapshots.

While I understand that pytest rewriting interferes with executing, it seems there are still two ways that fixing snapshots can work in common cases without disabling rewriting:

1. In Python 3.11+, executing still works even inside pytest assertions, right?
2. If executing can't find the node, try (approximately, I'm simplifying and not checking this code) statement = only(ex.stmts); node = only(child for child in ast.walk(statement) if isinstance(...) and child.func.id == 'snapshot') or maybe something like assert isinstance(statement, ast.Assert); node = statement.value.operands[1]).

Either of these would suffice for us 100% of the time. All our uses of snapshot are of the form assert ... == snapshot(...), and although we support Python 3.8 we develop in Python 3.12.

It also looks like working without rewriting might be helpful for #41.

[15r10nk]

if --inline-snapshot=fix could be enabled by default.

This would mean that your snapshots are always fixed and your tests would always pass. I don't think that this is what you mean.

I think that I understand you wrong here and that the problem is that you found some incorrect snapshots when you run pytest the first time and that you have to run your test again when you want to fix the snapshots.

If this is the case then #48 might be helpful, because it would allow you to run
pytest --inline-snapshot-ui which would open a browser if your tests fail and give you the option to fix your snapshots without running pytest twice (yes, you are basically open the browser ui in this case to press a button. I could also ask in some different way for approval, but the approval is important).

In Python 3.11+, executing still works even inside pytest assertions, right?

The problem is that pytest changes the location of all sub-expressions in the assertion to the source range of the whole assertion.

https://github.com/pytest-dev/pytest/blob/2cba2237cdbff964854e7964986cdbe6855bf75e/src/_pytest/assertion/rewrite.py#L966C1-L968C49

We would need to change this in pytest if executing should work for expressions in assert statements. There might still be some problems for ast nodes which are generated by pytest to instrument the source, but the general idea should work.

If executing can't find the node, try ...

There are probably a lot of corner cases, because snapshot() can be used almost anywhere.
I started a pull-request some time ago alexmojaki/executing#64 which I would prefer in this case.
It works similar to the PositionNodeFinder (uses lineno), but with some crazy mapping logic.
This should work for 3.8+.
I think the pull-request needs some more work, let me know what you think about the idea.

I can not promise 100% that everything will work, but there is a path.

It also looks like working without rewriting might be helpful for #41.

Definitely. This things are connected.

[alexmojaki]

This would mean that your snapshots are always fixed and your tests would always pass. I don't think that this is what you mean.

That's 100% what I mean. I'm sure that changes to snapshots would not go unnoticed in the git diffs. And this would of course be disabled in CI.

Personally the way I use inline-snapshot at the moment is that I have a 'run configuration' for every test file that contains snapshots, and the configuration sets --inline-snapshot=fix. So I'm very regularly running entire files with fix even when I expect all the tests to pass, just because it's convenient like that. I only don't have fix when running all the tests or when running one test function.

The problem is that pytest changes the location of all sub-expressions in the assertion to the source range of the whole assertion.

I wonder why I seem to remember PositionNodeFinder working inside pytest assertions. Looking now I can see that it doesn't. Did something change, or am I just completely misremembering?

There are probably a lot of corner cases, because snapshot() can be used almost anywhere.

It wouldn't always work, but it usually would. My thinking is that pytest rewriting would still be disabled by default but that this would be configurable. So there'd be a way to fix while still having pytest rewriting, and if inline-snapshot failed to reliably find a node using any method at all it would raise an error.

I started a pull-request some time ago alexmojaki/executing#64 which I would prefer in this case.
It works similar to the PositionNodeFinder (uses lineno), but with some crazy mapping logic.
This should work for 3.8+.
I think the pull-request needs some more work, let me know what you think about the idea.

Of course I would love this for executing even outside of inline-snapshot, but it's obviously a lot of work and I don't actually know if/when it'll actually happen, whereas I'm hoping that this proposal would be a lot easier.

[15r10nk]

I'm sure that changes to snapshots would not go unnoticed in the git diffs

The problem here is that I'm not so sure in the general case (for some random user at some time of the day (I know that I make a lot more mistakes when I'm tired 😄 .)).
I can not recommend your workflow for the average user which is using inline-snapshot for the first time. The probability that some snapshot are changed unintentionally is too height in my opinion.

I have nothing against your approach of making fix the default for you by creating a 'run configuration', if you know what you do.

Did something change, or am I just completely misremembering?

I don't know, it is also too long ago for me.

It wouldn't always work ...

The problem for inline-snapshot is that I would have to explain and document the cases where it does not work. It is not the best user experience when the user has to change his code almost randomly to make it work.

The way which I would prefer, is to change the code in pytest which currently changes the node locations. This should allow executing to find the nodes for 3.11+. This is also easy to explain for the user (feature x is only available for 3.11+).

[alexmojaki]

The docs would be something like:

--keep-pytest-rewriting

Normally --inline-snapshot=fix/update/create disables pytest's assertion rewriting so that snapshot() can work reliably. This flag prevents that so that assertion error messages are more useful. This means that it's more likely that snapshot() can't tell where it's being called from, particularly if there's more than one snapshot() call in a single assert statement. If this happens, snapshot() will raise an error. This flag is not recommended for most users.

Anyway, I agree that if you can make it just work in 3.11+ with pytest, that'd be good too.