5
\$\begingroup\$

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!

\$\endgroup\$
3
  • \$\begingroup\$ 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. \$\endgroup\$
    – Raphael
    Jan 8 '20 at 8:53
  • \$\begingroup\$ you can check the tempfile module to make a temporary directory \$\endgroup\$ Jan 9 '20 at 8:56
  • 1
    \$\begingroup\$ 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. \$\endgroup\$
    – Heslacher
    Jan 9 '20 at 9:23
4
\$\begingroup\$

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 ;)

\$\endgroup\$
1
  • \$\begingroup\$ Very detailed answer! Thanks, I will update my code this way, it sounds relevant. \$\endgroup\$
    – FTG
    Jan 8 '20 at 12:44
0
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.