#109 WIP Replace subprocess calls with a generic run function

Closed
floli wants to merge 2 commits from floli/borgmatic:run_function into master
floli commented 5 months ago

Tests are still failing, see #107.

Tests are still failing, see #107.
witten reviewed 4 months ago
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 @@
156 156
 
157
-    logger.debug(' '.join(full_command))
158
-    subprocess.check_call(full_command)
157
+    return run(full_command, capture_output = json)
witten

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 @@
24 23
 
25
-    output = subprocess.check_output(full_command)
26
-    return output.decode() if output is not None else None
24
+    run(full_command, capture_output = json)
witten

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

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

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 @@
5
+logger = logging.getLogger(__name__)
6
+
7
+
8
+def run(args, capture_output = False, **kwargs):
witten

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 @@
6
+
7
+
8
+def run(args, capture_output = False, **kwargs):
9
+    logger.debug('Executing: ' + ' '.join(args))
witten

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 @@
11
+    if capture_output:
12
+        return subprocess.check_output(args, **kwargs).decode()
13
+
14
+    subprocess.check_call(args, **kwargs)
witten

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 @@
10
-def insert_subprocess_mock(check_call_command, **kwargs):
11
-    subprocess = flexmock(module.subprocess)
12
-    subprocess.should_receive('check_call').with_args(check_call_command, **kwargs).once()
10
+def insert_run_mock(command, capture_output = False, **kwargs):
witten

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 @@
97
-    insert_subprocess_check_output_mock(
94
+    insert_run_output_mock(
98 95
         ('borg', 'list', '--short', 'repo', '--lock-wait', '5'),
99 96
         result='archive1\narchive2\n'.encode('utf-8'),
witten

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 @@
101
-    insert_subprocess_mock(('borg', 'extract', '--dry-run', 'repo::archive2', '--lock-wait', '5'))
98
+    insert_run_mock(('borg', 'extract', '--dry-run', 'repo::archive2', '--lock-wait', '5'))
102 99
 
103 100
     module.extract_last_archive_dry_run(repository='repo', lock_wait=5)
witten

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.
witten commented 3 months ago
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.
witten commented 2 months ago
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.
Please reopen this pull request to perform a merge.
Sign in to join this conversation.
No Milestone
No Assignees
2 Participants
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
Cancel
Save
There is no content yet.