Encapsulate subprocess calls in run() function #107

Closed
opened 2018-11-10 17:37:07 +00:00 by floli · 3 comments
Collaborator

This is a follow up to the discussion in #97.

I basically did the change above in https://projects.torsion.org/floli/borgmatic/src/branch/run_function but have a hard time mocking the function for the tests.

My main problems at mocking are:

  • To accept both variants of calling a function with an argument given the default value and the argument not given at all, which are essentially the same.

  • Previously, some tests mocked subprocess.check_call and subprocess.check_output. These are now both calls to run(capture_output = False) or run(capture_output = True). So I need to mock two calls to the same function with different argument sets.

Therefore my commit does not include any test changes and thus the tests will fail.

I am happy to make this a WIP pull-request, so at least @witten can contribute.

Best!

This is a follow up to the discussion in #97. I basically did the change above in https://projects.torsion.org/floli/borgmatic/src/branch/run_function but have a hard time mocking the function for the tests. My main problems at mocking are: * To accept both variants of calling a function with an argument given the default value and the argument not given at all, which are essentially the same. * Previously, some tests mocked `subprocess.check_call` and `subprocess.check_output`. These are now both calls to `run(capture_output = False)` or `run(capture_output = True)`. So I need to mock two calls to the same function with different argument sets. Therefore my commit does not include any test changes and thus the tests will fail. I am happy to make this a WIP pull-request, so at least @witten can contribute. Best!
Owner

Here are some ideas that may help you out with automated testing these changes:

.with_args() will help you set up separate mocks for each of run(capture_output=False) and run(capture_output=True) branches. For instance:

flexmock(module).should_receive('run').with_args(capture_output=False)
flexmock(module).should_receive('run').with_args(capture_output=True).and_return(...)

I believe that first mock may handle this case:

To accept both variants of calling a function with an argument given the default value and the argument not given at all, which are essentially the same.

If not, you can always add an additional mock as well that expects no arguments, e.g. .with_args(). (To test the default behavior.)

And you're welcome to make a WIP pull request if you like. I'm happy to contribute.

Here are some ideas that may help you out with automated testing these changes: [`.with_args()`](https://flexmock.readthedocs.io/en/latest/start/?highlight=with_args#mocks) will help you set up separate mocks for each of `run(capture_output=False)` and `run(capture_output=True)` branches. For instance: ```python flexmock(module).should_receive('run').with_args(capture_output=False) flexmock(module).should_receive('run').with_args(capture_output=True).and_return(...) ``` I believe that first mock may handle this case: > To accept both variants of calling a function with an argument given the default value and the argument not given at all, which are essentially the same. If not, you can always add an additional mock as well that expects no arguments, e.g. `.with_args()`. (To test the default behavior.) And you're welcome to make a WIP pull request if you like. I'm happy to contribute.
Owner

For anyone else looking at this ticket, here's the WIP PR: witten/borgmatic#109

For anyone else looking at this ticket, here's the WIP PR: https://projects.torsion.org/witten/borgmatic/pulls/109
Owner

Closing this since the PR is closed, but I'm happy to revisit in the future if you like.

Closing this since the PR is closed, but I'm happy to revisit in the future if you like.
Sign in to join this conversation.
No Milestone
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: borgmatic-collective/borgmatic#107
No description provided.