retry on PermissionError during data export #5922
Conversation
|
|
|
Thanks for opening a draft pull request 🚀If you have any questions, you can direct them to @Ghostvv ✨ |
|
I wasn't sure how to handle the questionary.confirm in a test, so I added a simple mock. Is that test enough? |
|
|
||
| interactive.questionary.confirm = QuestionaryConfirmMock(3) | ||
|
|
||
| await interactive._retry_on_error(func, "export_path", True) |
There was a problem hiding this comment.
optional: use a lambda for func. I think it's more clear what you're testing then (currently I have to follow the bool parameter and then see what func is supposed to do in that case.
There was a problem hiding this comment.
I think lambdas would be clearer to read as well. However it is weird to raise exceptions from lambdas (lambda x: raise y) doesn't work and also argument passing is tested this way as well.
There was a problem hiding this comment.
optional: you could do something like this. Using the mock you could also test the passing of the args.
| await interactive._retry_on_error(func, "export_path", True) | |
| await interactive._retry_on_error(Mock(side_effect=PermissionError()), "export_path") |
|
rebased |
|
|
||
| interactive.questionary.confirm = QuestionaryConfirmMock(3) | ||
|
|
||
| await interactive._retry_on_error(func, "export_path", True) |
There was a problem hiding this comment.
optional: you could do something like this. Using the mock you could also test the passing of the args.
| await interactive._retry_on_error(func, "export_path", True) | |
| await interactive._retry_on_error(Mock(side_effect=PermissionError()), "export_path") |
|
@alwx Can you take over the review next week as I'm on vacation? Thanks 🙏 |
|
would be great to get this into the 2.0 release |
|
|
||
| m = Mock(side_effect=PermissionError()) | ||
| with pytest.raises(PermissionError): | ||
| await interactive._retry_on_error(m, "export_path", 1, a=2) |
There was a problem hiding this comment.
| await interactive._retry_on_error(m, "export_path", 1, a=2) | |
| interactive._retry_on_error(m, "export_path", 1, a=2) |
| m = Mock(side_effect=PermissionError()) | ||
| with pytest.raises(PermissionError): | ||
| await interactive._retry_on_error(m, "export_path", 1, a=2) | ||
| c = call("export_path", 1, a=2) | ||
| m.assert_has_calls([c, c, c]) |
There was a problem hiding this comment.
a unit test should always just test one case. I think it makes sense to move this to a separate test function with an appropriate name.
| m = Mock(side_effect=PermissionError()) | ||
| with pytest.raises(PermissionError): | ||
| await interactive._retry_on_error(m, "export_path", 1, a=2) | ||
| c = call("export_path", 1, a=2) |
There was a problem hiding this comment.
Can you please refer to call by it's module name? This is a code convention of ours to make it very explicit at the first glance where a function is coming from
| c = call("export_path", 1, a=2) | |
| c = mock.call("export_path", 1, a=2) |
Co-authored-by: Tom Bocklisch <[email protected]>
Co-authored-by: Tom Bocklisch <[email protected]>
Co-authored-by: Tobias Wochinger <[email protected]>
Co-authored-by: Tobias Wochinger <[email protected]>
Proposed changes:
I did not catch the error at the higher level, because then the individual files could not be retried and inconsistent data (like double appends) would be unavoidable.
This is my first pull request for this project. It would be great to have some feedback.
Status (please check what you already did):
black(please check Readme for instructions)