Unit testing a commands class

Question

import itertools
import os
import re
from typing import List

class Command:
    """Base command class which all commands inherit from.

    Command class implements all common functions and utilities
    between commands, like file and directories handling, and specific
    commands like arbitrary, text, or picture commands inherit it.
    """

    def __init__(self, args):
        """Instantiate an object with the files/directory to work on."""
        self.dir_path = args.dir_path
        self._extract_files(args)
        self._files_paths = None

    def _extract_files(self, args):
        """
        Returns the files specified by the user, if any.
        Throws exception if no file matches user input.
        """
        self.files_list = None
        if args.name is not None:
            self.files_list = self._file_from_name(args.name)
        elif args.names_file is not None:
            self.files_list = self._names_from_file(args.names_file)
        elif args.rgx is not None:
            self.files_list = self._files_from_rgx(args.rgx)
        elif args.rgx_file is not None:
            self.files_list = self._rgx_from_file(args.rgx_file)
        else:
            self.files_list = self._extract_all_files()

        if len(self.files_list) == 0:
            raise FileNotFoundError("No files match")

    def _files_from_rgx(self, rgx):
        """
        Looks for files in the directory `self.dir_path` with names 
        matched to the provided regex and returns those files names.
        """
        f = []
        for (dir_path, dirnames, filenames) in os.walk(self.dir_path):
            f.extend(filenames)
            break
        prog = re.compile(rgx)
        m = []
        for file_name in f:
            matched = prog.match(file_name)
            # matched.span() => (match start index, match end index)
            # Check that the file name is matched as a whole
            if matched and matched.span()[1] == len(file_name):
                m.append(file_name)
        return m

import collections
import os
import pytest

from fipy.parser import parser
from fipy.commands.command import Command

class TestCommand:
    """Tests for the command class."""

    def _populate_tmp_dir(self, tmpdir, dirs, files):
        dirs = [tmpdir.mkdir(x) for x in dirs]
        files = [tmpdir.join(x).write('') for x in files]

    def test_files_from_rgx_no_match(self, tmpdir):
        dirs = ['abc', '1abc', 'x1.txt']
        files = ['x.txt', 'x.tsf', '1abc.png']
        self._populate_tmp_dir(tmpdir, dirs, files)
        args = parser.parse_args(
            ['-p', str(tmpdir), '-r', 's[0-9]*.txt',
            'any', 'rename', '-asc']
        )
        with pytest.raises(FileNotFoundError):
            command = Command(args)

First of all, is it a good practice to set self.dir_path and make the function use it, or pass it to the function as a parameter? (_files_from_rgx function)

And if I want to write unit tests, should I test write a test for the function and another for the constructor, or write tests for the constructor, which implicitly will enter the function which I have done in the test above?

Answer 1

There's a couple of things to consider when designing a class -> test relationship.

First, I'd try hard to avoid the situation you're setting up where a command class can crash in its own constructor. It's usually a better idea to make sure that constructors are exception-safe if possible, since you never know what calling code may want to do with them. So the first thing I'd consider is simply turning _extract_files() into a regular public method and not calling it in the constructor. Among other things, it simplifies the test code by allowing you to create test objects without pre-configuring things -- but it's also better in general.

Which leads to number 2: it's better not to rely on the state of a local hard drive in tests. Ideally a test suite runs in a completely reproducible, zero-entropy environment which never changes -- not even if your build machine is low on hard drive space, the network connection goes down, a user doesn't actually have a writable tempdir, and so on. These kinds of conditions are often hard to work around, but thinking them through tends to make for better testing - and that, in turn, leads to better code.

In your case, _files_from_rgx really wants to be operating on the state of a local directory. However the 'work' is really coming from the way the regex parses the file list. So, one way to get good coverage is to refactor Command so that the parsing logic works on a file list (or better yet, any iterable since that's more generous). In this case if you took the regex-filter function out you might get something like this for a start:

def _files_from_rgx(self, rgx):
    """
    Looks for files in the directory `self.dir_path` with names 
    matched to the provided regex and returns those files names.
    """
    f = []
    # incidentally since you're not using the recursion here,
    # you could probably replace this with  
    # f = os.listdir(self.dir_path)

    for (dir_path, dirnames, filenames) in os.walk(self.dir_path):
        f.extend(filenames)
        break

    return list(self._filter_by_regex(rgx, f))


def _filter_by_regex(self, rgx, inputs):
    """
    loop over <inputs> yielding out values which are full-name
    matches for the regex <rgx>
    """
    compiled_rgx = re.compile(rgx)

    for each_file in inputs:
        matched = compiled_rgx.match(each_file)
        if matched and matched.span()[1] == len(each_file):
            yield each_file

Now you can test the _filter_by_regex function in isolation :

  def test_filter_by_regex(self):
       example = Command(some_test_args):
       known_regex = ".*\.py"
       sample_data = ['a.py', 'b.py', 'c.cpp', 'd.exe', 'e.py']
       results = list(example._test_filter_by_regex(known_regex, sample_data))
       self.assetListEqual(results, ['a.py', 'b.py', 'e.py']

This doesn't require you to actually create temp files and so on (note that you have to be a good citizen if you do create temp files, because if the tests are running on a build server you don't want to be leaking test files all over the place!).

You didn't include source for _file_from_name() and _names_from_file but it would be a good idea to structure them and their tests so you can test them in the same way, without going to disk. You'll eventually have a function somewhere what will have to call os.listdir() or os.walk and feed those functions -- but at least the part of the logic that is all yours will be covered by tests.

The test you wrote does one thing that's really great, which is guaranteeing the exception raised by an empty file list. That's a very good test, because it guarantees predictable behavior for users. You can make that functionality independently testable with a similar small refactor that moves that into a separate function:

def _validate_file_list(self, file_list):
   if not file_list:
        raise FileNotFoundError("No files match")

And a matching test:

def test_validate_raises_on_empty_file(self):
       example = command(some_test_args)
       should_raise = lambda: example._validate_file_list([])
       self.assertRaises(FileNotFoundError, should_raise)

There's another approach you could take, but I would recommend learning how to do the "old-fashioned" way first. The mock module would allow you to simulate the state of a disk by replacing calls to things like os.walk with a dummy function that return the same results every time without looking at the disk. It's a very powerful technique, but it can encourage code that's otherwise not easily testable. I'd figure out more ways to isolate and test key code paths without mock before adding it in as a final level of testing.

For a good look at the philosophy of breaking out logic and IO, I highly recommend this talk by Python core dev Brandon Rhodes: https://youtu.be/PBQN62oUnN8