Test on remove function

Question

let's say I want to create a 'utils' package with several functions working on files and folders. I want to do it once properly so I can use them in all my personal projects without feeling to re invent the wheel each time. I also want to learn 2 items by the way:

• exception handling, already read a lot about EAFP vs LBYL, I think the following function suits well to EAFP.
• unit tests with pytest tool, it seems simple enough to start with.

My function is:

def removeanything(src):
 """
 remove files or folders
 """
 try:
 os.remove(src)
 print('File is removed')
 except IsADirectoryError:
 shutil.rmtree(src)
 print('Folder is removed')
 except FileNotFoundError:
 print('File/folder is not existing')
 except PermissionError:
 print('Not allowed to suppress this file')

Do you have any comment to do regarding my exception handling? Did I forget one or more exceptions?

To go ahead, I have written this test function for pytest:

from pathlib import Path
import utils
#import pytest
def test_removeanything(tmp_path):
 d = tmp_path / '.tmp_dir'
 d.mkdir()
 f = d / '.tmp_file'
 f.touch()
 # Test permission
 d.chmod(mode=0o555)
 utils.removeanything(f)
 assert Path.exists(f)
 # Test file deletion
 d.chmod(mode=0o777)
 utils.removeanything(f)
 assert not Path.exists(f)
 # Test folder deletion
 utils.removeanything(d)
 assert not Path.exists(d)

As I am pretty newbie at testing, I would like to know if I include all reasonable things to assert for my function? Are there other way to do it more properly? Is there a way to assert if expected error has been indeed raised by my function?

Thanks!

• Raphael: Just a general comment: I wouldn't make a function to remove files or directories, it may be dangerous. You can separate your function in two (or pass a flag) and make sure you will remove a file or a directory separately. I'm telling you this because I already had this problem. I removed an entire directory with results that took days to complete just because I passed the wrong path to a function like this one. Of course this is up to you, others may disagree.
• Maarten Fabré: you can check the tempfile module to make a temporary directory
• Heslacher: I have rolled back your latest edit. Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers .

Answer 1

Doc comments:

You can be more clear in the doc comment about what the function does:

• What is the input? (string, path)
• Does it return anything? (no)
• Does it throw exceptions? (yes, because os.remove and shutil.rmtree may throw)
• What is the state after calling the function? (file/folder has been removed, except if there was a permission error or another error)

Exceptions:

Someone who calls removeanything("myfile") may expect that myfile does not exist anymore after the function call. However, in case of a permission error, it still does exist. I think this is an exceptional situation, so I recommend that you do not catch the PermissionError and instead propagate it to the caller.

Output:

Currently, the function communicates via print statements. This means that callers of the function have no way of finding out what actually happened. You could add a return value that indicates whether a file, a directory, or nothing was removed. Then you may think about removing the print statement, or enabling/disabling it via function argument, because users may want to remove files silently.

Tests:

It may be useful to separate the single test test_removeanything into multiple tests test_removeanything_deletes_file, test_removeanything_deletes_directory, test_removeanything_handles_permission_error. This way, if a test fails, the test name gives you some more information about what went wrong.

Often, functions that remove directories require them to be non-empty. Therefore, it makes sense to test the removal of both empty and non-empty directories.

If you change removeanything so that the PermissionError propagates to the user, you can use pytest.raises to test whether the exception was raised correctly.

Misc:

I think the name removeanything can be more specific. After all, the function does not remove a CD from my CD drive ;)

• FTG: Very detailed answer! Thanks, I will update my code this way, it sounds relevant.

Answer 2

I have updated my code this way, using pytest.fixtures with temp file/folders provided by pytest.

from pathlib import Path
import os
import utils
import pytest
@pytest.fixture
def fd(tmp_path):
 d = tmp_path / '.tmp_dir'
 d.mkdir()
 f = d / '.tmp_file'
 f.touch()
 return f,d
# Test permission
def test_permissions(fd):
 with pytest.raises(PermissionError):
 f,d = fd
 d.chmod(mode=0o555)
 utils.removeanything(f)
# Test file deletion
def test_delete_file(fd):
 f,d = fd
 utils.removeanything(f)
 assert not Path.exists(f)
# Test non empty folder deletion
def test_delete_folder_nonempty(fd):
 f,d = fd
 utils.removeanything(d)
 assert not Path.exists(d)
# Test empty folder deletion
def test_delete_folder_empty(fd):
 f,d = fd
 f.unlink()
 utils.removeanything(d)
 assert not Path.exists(d)

I have added other tests taking into account possiblke empty directory. Regarding test_permissions, a context manager tracks if error is well raised.