Expose ProcessDispatcher._get_process as a public API or return a Popen result from register(...)

[MrMino]

It is impossible to properly unit-test functionalities like sending a signal, when the tested class facades subprocess.Popen without exposing the resulting process object. Rummaging in class internals during unit tests is a bad practice which leads to brittle testcases.

It would be nice if pytest-subprocess had a way of getting the result of faked Popen (or its proxy). That way we could assert some side effect without having to extract the Popen result from the unit under test:

def test_kills_the_process(fake_subprocess):
    process = fake_subprocess.register(["command"])
    run_and_kill("command")
    assert process.received_signals() == (signals.SIGKILL,)

[aklajnert]

Thanks for raising an issue. It's a fair point, and I'll think about how should I approach it.
The fake_subprocess.register() can't return a Popen instance as it is not created at that time yet and may never be, or can be created multiple times from one register() call. Returning some proxy class that would gather all Popen instances created from the single register() seems reasonable.

[MrMino]

I suggest returning a proxy object from register, with all the executed, argv, signals, etc. properties returning False / None, until the right Popen is actually called. Then make the proxy contain the result of that Popen and make the properties return real values.

It's the most non-invasive architecturally, from what I've seen so far. And, as a bonus, it's pretty intuitive to use.

I might be able to PR this, as I'm using pytest-subprocess in a project right now, and this would be a game changer for lots of tests - just let me know if the plan above sounds good to you.

[aklajnert]

I think it should have a list of Popen instances, as one register() may be used multiple times. The list will be initially empty and the FakePopen instances will be appended to it on spawn. That seems most reasonable to me.

I'll probably work on it during the weekend (no promises), but if you want to tackle it, let me know and I'll wait for your PR.

[aklajnert]

@MrMino - I've implemented that in #80 and merged it into master. Let me know if that's good for you, and I'll release the new version.

[MrMino]

Hi, sorry for a late response.

I'm still trying to grasp all of the implications of this and find the best way to fit it into tests. I think it does tick the boxes in the functionality department, but I'm not 100% sure about the usability of it yet.

E.g. there's at least one ugly usability oddity that this kind of design encourages. If I'm testing for signals, and I want the exceptions to be helpful for anyone debugging, I have to assert twice: first to check that the process has been called at least once, secondly to test the signal.

Either I copy the first assert to every test that does a check like this, or I fall into a trap: tests will raise a confusing IndexError if the Popen isn't called.

It would be nice if the indexing was done for me internally instead, and if there was no call - a custom exception with proper message was raised. Here's what I imagined could be a step in the right direction:

process_calls = fake_process.register(...)
...
assert process_calls.first_call.received_signals() == ...

Augmenting the returned (list-like) object with these kinds of amenities greatly helps readability too.

This idea is compatible with returning something that can be indexed: the indexing is still worthwhile for someone who tests for a number of calls.

[aklajnert]

Thanks for the feedback, I see your point. I didn't release the new version yet, so it's OK to modify it. Now, once the register() method returns a list it will be easy to create some object that does what you want and provides a better API than a list.

If you could propose a PR with your vision of the object it would be great, if not then I can work on it, but it probably won't be in this month.

I think we can modify the API and instead of having the object indexable, it should expose something like calls, from where individual calls could be extracted.

[MrMino]

I think we can modify the API and instead of having the object indexable, it should expose something like calls, from where individual calls could be extracted.

Yes, I would see that as a better approach. In my previous comment I evaluated the design as implemented on master.

If you could propose a PR with your vision of the object it would be great, if not then I can work on it, but it probably won't be in this month.

I'll try to do so if the time permits, next week perhaps. Please share any suggestions and use-cases you might have on your mind, I'll do my best to accommodate them.

[aklajnert]

I have no suggestions except to have the list of calls in a separate calls property instead of indexing/iterating over the returned object.

[aklajnert]

@MrMino - are you still intend to submit your suggestions, or should I give it another take?

[MrMino]

Sorry, I wasn't able to find the time for it. You can take it.

[aklajnert]

@MrMino - the fp.record() will now return ProcessRecorder instance. Feel free to open another issue if you still miss some functionality there.