The `Popen` patching is a little fragile

[gipi]

If you use from multiprocess import Popen the library is not able to swap the implementation, I don't know if it's an intended behavior, maybe should be explicitly indicated in the documentation.

Minimal reproducible example: imagine you have the module benchmarking.py with the following function:

from subprocess import Popen as imported_Popen

def meow(cmdline):
    return imported_Popen(cmdline, stdout=subprocess.PIPE)

and then you have a separate file with your test

from benchmarking import meow

def test_echo_null_byte(fp):
    fp.register(["echo", "-ne", "\x00"], stdout=bytes.fromhex("00"))

    process = meow(["echo", "-ne", "\x00"])
    out, _ = process.communicate()

    assert process.returncode == 0
    assert out == b"\x00"

then the test it's gonna fail because (probably) at the moment the test is run it's too late to swap the implementation.

[aklajnert]

Ah, right - it may depend on the order of imports that in some cases the imported_Popen may be taken before patching. I don't think we can do anything about it. I'll think about it.

Feel free to file a PR for the documentation change, that would be very welcome.

[gipi]

thanks for the prompt feedback, I'll try to schedule some time to update the documentation.

[mrfroggg]

I also faced the issue where I imported my Popen like this:
from subprocess import Popen, PIPE
And fp.register() wasn't able to catch my subprocesses calling Popen() directly.
Putting

import subprocess
subprocess.Popen()

Fixed my issue for fp.register()

[aklajnert]

Yes, if you're using Popen by importing a whole subprocess it will work all the time. If you import Popen directly it can work, but the order of imports can cause a failure. I'll look it up if it could be fixed in some way, but I don't think it's possible.

[terencehonles]

What if you patched Popen.__new__ instead of patching subprocess.Popen itself? I was just driving by after looking for something like this and I may add this as a dependency, but I'm not 100% sure if you'd be able to insert a __new__ after class creation, but if you could you could return a reference to the FakeProcess instead of the Popen object and then existing references to Popen would still be using this code path.

[aklajnert]

That's definitely worth exploring - it could work. I'll try to play with it a bit during the weekend, thanks!

[aklajnert]

@terencehonles - I've just tried it, and unfortunately it won't work. Attempting to override __new__() method results in a following exception:

AttributeError: 'method' object attribute '__new__' is read-only

[foobacca]

Would it be worth adding an option to add an explicit patch step, where you pass in the dotted path. If you're using a third party library you can't just change how they do their imports. This is the approach taken by the testfixtures popen testing. You could keep the automatic patching for the majority of cases but have a documented way to explicitly patch when the automatic patching does not work.