WIP Replace subprocess calls with a generic run function #109
No reviewers
Labels
No Label
bug
data loss
design finalized
good first issue
new feature area
question / support
security
waiting for response
No Milestone
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: borgmatic-collective/borgmatic#109
Loading…
Reference in New Issue
No description provided.
Delete Branch "(deleted):run_function"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Tests are still failing, see #107.
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)
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.@ -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)
Needs to
return
the result of therun()
invocation?@ -214,2 +214,4 @@
remote_path=remote_path,
)
if args.json:
json_results.append(json.loads(output))
Would it make sense to do
if output:
instead ofif args.json
here? That way, you don't have theargs.json
-checking logic in two places (1. increate_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):
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))
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)
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):
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'),
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 fromrun()
. Or in this case, the mockrun()
.@ -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)
So I looked a bit into the causes of these tests failing. It looks like
insert_run_output_mock()
andinsert_run_mock()
both create mocks forrun()
, which interfere with one another. This wasn't an issue previously withinsert_subprocess_check_output_mock()
andinsert_subprocess_mock()
, because each one created a mock for a different underlyingsubprocess
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 expectcapture_output
for this to work.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.
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.
Pull request closed