WIP Replace subprocess calls with a generic run function #109

Closed
floli wants to merge 2 commits from (deleted):run_function into main
Collaborator

Tests are still failing, see #107.

Tests are still failing, see #107.
witten reviewed 2018-12-02 22:41:35 +00:00
witten left a comment
Owner

Looks like progress! I gave some tips on fixing the tests. If you get stuck again, let me know and I can dig in.

Looks like progress! I gave some tips on fixing the tests. If you get stuck again, let me know and I can dig in.
@ -156,3 +156,2 @@
logger.debug(' '.join(full_command))
subprocess.check_call(full_command)
return run(full_command, capture_output = json)
Owner

Note that when you run the Black code formatter on this (part of running borgmatic's tox), it will complain about the format on lines like this one. You can always run tox -e black though to have it auto-reformat them for you.

Note that when you run the [Black](https://github.com/ambv/black) code formatter on this (part of running borgmatic's tox), it will complain about the format on lines like this one. You can always run `tox -e black` though to have it auto-reformat them for you.
@ -24,3 +23,2 @@
output = subprocess.check_output(full_command)
return output.decode() if output is not None else None
run(full_command, capture_output = json)
Owner

Needs to return the result of the run() invocation?

Needs to `return` the result of the `run()` invocation?
@ -214,2 +214,4 @@
remote_path=remote_path,
)
if args.json:
json_results.append(json.loads(output))
Owner

Would it make sense to do if output: instead of if args.json here? That way, you don't have the args.json-checking logic in two places (1. in create_archive() and, 2. here). Similar elsewhere in this function. Do no feel super strongly.

Would it make sense to do `if output:` instead of `if args.json` here? That way, you don't have the `args.json`-checking logic in two places (1. in `create_archive()` and, 2. here). Similar elsewhere in this function. Do no feel super strongly.
@ -0,0 +5,4 @@
logger = logging.getLogger(__name__)
def run(args, capture_output = False, **kwargs):
Owner

Would be nice to have a doc string for this function, even though it's pretty self-explanitory.

Also, given that this function is only used within borgmatic/borg/, this file might belong in that directory. (Do not feel strongly.)

Would be nice to have a doc string for this function, even though it's pretty self-explanitory. Also, given that this function is only used within `borgmatic/borg/`, this file might belong in that directory. (Do not feel strongly.)
@ -0,0 +6,4 @@
def run(args, capture_output = False, **kwargs):
logger.debug('Executing: ' + ' '.join(args))
Owner

Might be a little more idiomatic to do something like: logger.debug('Executing: {}'.format(...))

Might be a little more idiomatic to do something like: `logger.debug('Executing: {}'.format(...))`
@ -0,0 +11,4 @@
if capture_output:
return subprocess.check_output(args, **kwargs).decode()
subprocess.check_call(args, **kwargs)
Owner

This function could use some very basic test coverage. Basically: The capture output case, and the no capture output case. That'd require a little bit of mocking.

This function could use some very basic test coverage. Basically: The capture output case, and the no capture output case. That'd require a little bit of mocking.
@ -10,3 +10,1 @@
def insert_subprocess_mock(check_call_command, **kwargs):
subprocess = flexmock(module.subprocess)
subprocess.should_receive('check_call').with_args(check_call_command, **kwargs).once()
def insert_run_mock(command, capture_output = False, **kwargs):
Owner

I think capture_output isn't used here, and so should be removed.

I think `capture_output` isn't used here, and so should be removed.
@ -97,3 +94,3 @@
insert_subprocess_check_output_mock(
insert_run_output_mock(
('borg', 'list', '--short', 'repo', '--lock-wait', '5'),
result='archive1\narchive2\n'.encode('utf-8'),
Owner

I don't think you want this .encode() here. The unit under test (extract_last_archive_dry_run()) is expecting an already decoded result string returned from run(). Or in this case, the mock run().

I don't think you want this `.encode()` here. The unit under test (`extract_last_archive_dry_run()`) is expecting an already decoded result string returned from `run()`. Or in this case, the mock `run()`.
@ -101,3 +98,3 @@
insert_subprocess_mock(('borg', 'extract', '--dry-run', 'repo::archive2', '--lock-wait', '5'))
insert_run_mock(('borg', 'extract', '--dry-run', 'repo::archive2', '--lock-wait', '5'))
module.extract_last_archive_dry_run(repository='repo', lock_wait=5)
Owner

So I looked a bit into the causes of these tests failing. It looks like insert_run_output_mock() and insert_run_mock() both create mocks for run(), which interfere with one another. This wasn't an issue previously with insert_subprocess_check_output_mock() and insert_subprocess_mock(), because each one created a mock for a different underlying subprocess function.

Here's what I would recommend: Remove the .once() from the mock creation within each insert test function. That'll prevent the two mocks from interfering with one another. It does make the assertion a little weaker. (It's not actually asserting that it's called.) But that seems fine.

Also, I think you'll need to change insert_run_mock() not to expect capture_output for this to work.

So I looked a bit into the causes of these tests failing. It looks like `insert_run_output_mock()` and `insert_run_mock()` both create mocks for `run()`, which interfere with one another. This wasn't an issue previously with `insert_subprocess_check_output_mock()` and `insert_subprocess_mock()`, because each one created a mock for a different underlying `subprocess` function. Here's what I would recommend: Remove the `.once()` from the mock creation within each insert test function. That'll prevent the two mocks from interfering with one another. It does make the assertion a little weaker. (It's not actually asserting that it's called.) But that seems fine. Also, I think you'll need to change `insert_run_mock()` not to expect `capture_output` for this to work.
Owner

Please let me know if there's any way I can help with this one! It would be great to get it merged at some point.

Please let me know if there's any way I can help with this one! It would be great to get it merged at some point.
Owner

Closing this, just because it's so far from master at this point. But please feel free to re-open or re-file as a new PR! Thanks for taking the time to work on it, and it would be great to get a refactor like this in at some point.

Closing this, just because it's so far from master at this point. But please feel free to re-open or re-file as a new PR! Thanks for taking the time to work on it, and it would be great to get a refactor like this in at some point.
witten closed this pull request 2019-02-24 07:18:29 +00:00
Some checks failed
the build failed

Pull request closed

Sign in to join this conversation.
No reviewers
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#109
No description provided.