add missing await on delete_blob call per issue 459 #460
Conversation
|
tested this locally and appears to run fine. unsure if further testing is required |
|
Thanks, I think a unit test would be helpful to avoid future regressions. Can you add one? |
|
@TomAugspurger yea sure thing |
|
This issue might be a little more impactful than I originally thought. I initially thought we were just leaking the runtime warning but still cleaning up the directories. However, it looks like without await none of those I also didn't realize that this test suite could run out of the This change actually breaks this unit test It looks like this test and the one above it are mocking the All the files created in the Going to see if I can modify these tests in a way where we can still evaluate the call ordering but the combination of Mocking + async is a bit out of my wheelhouse |
| ) | ||
| def test_rm_recursive2(mock_delete_blob, storage): | ||
| @pytest.mark.filterwarnings("error") | ||
| def test_rm_recursive2(storage): |
There was a problem hiding this comment.
I modified this test to remove the mocking so the delete_blob calls are executed asynchronously. This test ensures that the files in data/rootare in fact removed. Also, the decorator will raise the RuntimeWarning emitted from the missing await (regression).
| assert "data/root" not in fs.ls("/data") | ||
|
|
||
| with pytest.raises(FileNotFoundError): | ||
| fs.ls("data/root") | ||
|
|
||
| last_deleted_paths = [call.args[1] for call in mock_delete_blob.mock_calls[-7:]] | ||
|
|
||
| async def test_rm_recursive_call_order(storage, mocker): |
There was a problem hiding this comment.
Since fs.rm validates that the files are actually removed, there wasn't an easy way to mock delete_blob so that still ran the deletes asynchronously as a side effect (someone might have a clever way to do this). So instead I just explicitly supply the file paths under data/root , call fs._rm_files, and assert the call order similar to how the previous test was working
|
@TomAugspurger changes pushed |
|
@TomAugspurger @efiop any chance we can get some eyes on this? |
|
Thanks @johnmacnamararseg! |
|
Thank you for fixing @johnmacnamararseg, I did not notice that folders were not deleted until you mentioned it. Looking forward to this release |
per #459
add missing await on
delete_blob