Pytest plugin has side effect and is extremely unintuitive to use

[codethief]

This might not be considered a bug, but it is – at the very least – a severe code smell.

Describe the bug
Once pyfakefs is installed, the pytest plugin seems to get loaded automatically and register a fixture with pytest. This has severe consequences:

• I don't even have to import pyfakefs anywhere in my project's code and yet I still have a dependency on pyfakefs that is completely non-obvious.
• It is hard to read someone else's unit tests, as it is unclear where that fs parameter is coming from
• If I try to make the dependency on pyfakefs obvious by e.g. doing from pyfakefs.pytest_plugin import fs, the import will seem un-used and linters and code formatters will complain about it or even remove it automatically.
• I cannot have different fs fixtures in different tests. The pytest fixture that pyfakefs registers is global state.
• I cannot use the @patchfs decorator on one of my pytest unit tests to make the pyfakefs dependency more obvious as @patchfs uses the same named parameter fs and employs functools.wraps to make the wrapped function look like the original one (including its named parameters). (Attempting to use @patchfs on a unit test results in a recursion error. Let me know if I should file a separate bug report for this.)

I am aware that a large part of this issue is caused by pytest's weird approach of having a global registry of fixtures. However, the fixture in pyfakefs exacerbates this by orders of magnitude.

Suggestions:

• Wrap the pytest fixture in pyfakefs.pytest_plugin in a function that the user has to call explicitly somewhere in her/his code in order to register the fixture. (Clearly, this would be a breaking change.)
• As an alternative, get rid of the fixture – the same functionality could be obtained by using @patchfs and using fixtures is essentially global state. (This would be a breaking change, too.)
• At least make @patchfs pass the FakeFilesystem instance to the decorated function as the first parameter, instead of as a named parameter fs. This way, one could still use @patchfs on pytest unit tests without interfering with the fixture. (This could break some existing code.)
• At the very least: Document this very strange behavior. It even took me a while to understand that the example in the current docs is not just a code snippet that's missing setup code and imports but it's all the code that's needed. It is uncompletely unclear that the pytest plugin that's mentioned in the docs gets loaded automatically and the fixture will be registered by default. In fact, in order to learn all this I had to dig through pyfakefs's code.

While a breaking change seems little desirable, it is IMO the only real solution in the long term. One could try to smoothen the transition process by e.g. deprecating the fixture first.

[mrbean-bremen]

I agree with the behavior of the @patchfs decorator, which may indeed be a problem. This was made intentionally to make it work similar to pytest, though I see that this may be confusing. It would probaly help to make the problem more obvious, I will think about this.

Not so sure about the fixture. This is the standard behavior of pytest and all pytest plugins. While that may be weird, it is the de-facto standard as more and more projects are using pytest nowadays. Changing that would break all the existing usage of the fs fixture, which has been around for a few years. Looking at any pytest code that uses fixtures does not tell you where the fixture comes from.
The only difference is that pyfakefs is not obviously (and not only) a pytest plugin, so it may not be that obvious that it provides a pytest fixture. A possibility would be to separate the fixture intro a separate pytest-fs package, which depends on pyfakefs and will provide the fixture (instead of providing it in pyfakefs), but doing so now would also break existing code, and don't think this is worth it.

[codethief]

Wow, thanks a lot for responding so quickly!

While that may be weird, it is the de-facto standard as more and more projects are using pytest nowadays.

Sure, but I would argue that this doesn't mean one should follow this weird standard. ;) Global state is never a good idea, as demonstrated perfectly by this issue.

doing so now would also break existing code, and don't think this is worth it.

Would a breaking change in an upcoming major version really be an issue, though, especially when adapting to the breaking change in existing projects would involve only changing one single line to the list of dependencies? (Besides, as I said, one could add a deprecation warning in the current major version of pyfakefs to further smoothen the transition process.)

[mrbean-bremen]

Sure, but I would argue that this doesn't mean one should follow this weird standard. ;) Global state is never a good idea, as demonstrated perfectly by this issue.

Well, I disagree. Changing the established standard for pytest is an uphill battle that I don't want to fight. And yes, global state is not a good idea, but in this case it is something that a criticial mass of developers is used to, so there you are...

Anyway, I would like to hear the opinion of @jmcgeheeiv on this.

Going the way of splitting the fixture into an extra package is something we might indeed consider. We could add a new repository (pytest-fs), that would just provide the plugin and rely on pyfakefs for the implementation, and remove direct pytest support in pyfakefs in a later version. It would be a bit unfortunate documentation-wise... maybe point to different parts of the same documentation. We could also move this to the pytest-dev organization as most pytest plugins are...
This would not change the way tests would look like, though. It would just be a more conscious decision to use the fs plugin, as it is done with all other pytest plugins. Not sure if that even makes sense...

[codethief]

Fair enough, the decision is certainly yours, not mine, to make. Thanks again for even taking the time to consider this!

[mrbean-bremen]

I will try to change the keyword argument to a positional argument, as this would certainly be better.
As for the pytest plugin behavior--I may think about it, and will see what @jmcgeheeiv thinks, but I currently don't see much benefit in changing it.

I now remember why I didn't use a positional argument in the first place. While it is very easy to change, it does not play well with mock.patch, because the order of the expected arguments is not what you might expect (e.g. writing @patchfs before @mock.patch will put the fake fs argument before the mock argument instead of after it). This is due to some magic that mock does--I will try to wrap my head around this, but that may take a while.

[mrbean-bremen]

Hm, I probably will just document the behavior - not much I can do here, as all mock.patch calls are collected and their arguments added together contiguously. Anyway, I think this is still better then the keyword argument.

[codethief]

Sorry for not responding!

I now remember why I didn't use a positional argument in the first place. While it is very easy to change, it does not play well with mock.patch, because the order of the expected arguments is not what you might expect (e.g. writing @patchfs before @mock.patch will put the fake fs argument before the mock argument instead of after it)

Could elaborate on this? By "writing @patchfs before @mock.patch" do you mean

@patchfs
@mock.patch(…)
def test_foo(…):
    pass

or

@mock.patch(…)
@patchfs
def test_foo(…):
    pass

? In the first case I would expect fs to be the second argument of foo(), while in the latter case it should be the first argument.

[mrbean-bremen]

Yes, but in reality it is vice verse now. patch goes through some hops to make it the the other (expected) way around, but only if you have several patch decorators. They basically collect all patch decorators, and add the arguments in the reverse order afterwards. This does not play nice with other decorators who want to do the same, and I gave up on this and just documented the behavior for now.

[mrbean-bremen]

Well, I think we are not going to do anything else here. In principle, a pytest-fs plugin could make sense, but at this stage it would be too disruptive to remove the fs from pyfakefs, and it does not make much sense to duplicate the functionality. I'm closing this issue now.