Testing Out Testing

Question

I've been learning about unit testing with pytest recently, trying to figure out what is and isn't a valuable thing to test. I'm very new to it, so I thought a review would be a good way to learn about holes I'm leaving and mistakes I'm making.

So, I'm primarily interested in the test functions being reviewed, though I'm open to feedback on the functions being tested too.

The tools.py functions are just common functions I often need across my scripts, so I wanted to collect them in the library for convenience. A progress bar for giving feedback on how far along a loop has run, a function to ensure that a folder exists and a quick connection function.

Note: Shotgun is a third party service I often hook into. A Shotgun object is just a tool for calling queries and doing updates on an external database. For these purposes, you mostly just need to know that it's an object that needs to authenticate.

I tried to focus my testing on things that could be broken by changes, or are likely to go wrong without immediately being noticed. So here's the scripts:

test_tools.py

import pytest
import os

from time import sleep

from tools import get_shotgun, create_folder, ProgressBar


@pytest.fixture(scope='module')
def shotgun():
    return get_shotgun()


def test_get_shotgun_returns_value(shotgun):
    assert shotgun is not None, "Shotgun object was not returned"


def test_get_shotgun_credentials(shotgun):
    shotgun.find_one("Project", [])


def test_create_folder_invalid_path_raises_errors():
    with pytest.raises(OSError):
        create_folder("\\\\\\")


def test_create_folder_swallow_existing_folder_exception():
    # Folder of current file definitely exists.
    folder = os.path.dirname(__file__)
    create_folder(folder)


def test_create_folder_returns_original_path():
    folder = "dummy"
    while os.path.isdir(folder):
        folder = folder + '_'

    assert folder == create_folder(folder), "Original path was not returned"

    try:
        os.rmdir(folder)
    except OSError:
        print("Couldn't delete temporary test folder.")
        raise


def test_ProgressBar_validate_input():
    with pytest.raises(ValueError):
        ProgressBar("zero")
    with pytest.raises(ValueError):
        ProgressBar(None)
    with pytest.raises(ValueError):
        ProgressBar(-1213)


def test_ProgressBar_finishing_above_full():
    print('\n')
    bar = ProgressBar(100)
    for _ in range(16):
        sleep(0.1)
        bar.iter(12)


def test_ProgressBar_context_manager():
    print('\n')
    with ProgressBar(10) as bar:
        for _ in range(10):
            sleep(0.1)
            bar.iter(1)


def test_ProgressBar_context_manager_exiting():
    print('\n')
    with ProgressBar(10) as bar:
        for i in range(10):
            sleep(0.1)
            bar.iter(1)
            if i > 6:
                break


def test_ProgressBar_context_manager_not_mask_errors():
    print('\n')
    with pytest.raises(ValueError):
        with ProgressBar(10) as bar:
            int('')

tools.py

import errno
import os
import sys

from shotgun_api3 import Shotgun


class ProgressBar(object):
    """A simple class for printing messages about progress.

    Takes an int `full`, to indicate the intended total progress to make.
    `iter()` increases current progress value and prints the result.
    A context manager is also supported, allowing
        with ProgressBar(123) as bar
    """

    def __init__(self, full):
        try:
            self.full = int(full)
            if self.full < 1:
                raise ValueError
        except (TypeError, ValueError):
            raise ValueError("ProgressBar can only take int values > 0.")

        self.progress = 0
        self.completed = False

    @property
    def finished(self):
        return self.progress >= self.full

    def iter(self, increment=1):
        """Increases progress and prints a message showing current progress.

        increment defaults to 1, but can increase by any value.
        Having more than `full` progress isn't validated, just counted as the 
        bar being fully finished.
        """

        self.progress += increment

        sys.stdout.flush()
        if self.finished:
            self.finish()
            return

        string = '=' * int((self.progress * 50.0) / self.full)
        percent_progress = int((self.progress * 100.0) / self.full)
        message = "\r|{:50}|In progress: {}%".format(string, percent_progress)
        sys.stdout.write(message)

    def finish(self):
        if not self.completed:
            sys.stdout.write("\r|{0:50}|COMPLETE\t\t\n".format("=" * 50))
            self.completed = True

    def __enter__(self):
        sys.stdout.flush()
        message = "\r|{:50}|Initialising".format(0)
        sys.stdout.write(message)
        return self

    def __exit__(self, *exception_values):
        if self.finished:
            self.finish()
            return

        string = '=' * int((self.progress * 50.0) / self.full)
        percent_progress = int((self.progress * 100.0) / self.full)
        message = "\r|{:50}|Exited at: {}%\t\n".format(string, percent_progress)
        sys.stdout.write(message)


def get_shotgun():
    SERVER_PATH = "REDACTED"
    SCRIPT_NAME = "REDACTED"
    SCRIPT_KEY = "REDACTED"
    return Shotgun(SERVER_PATH, SCRIPT_NAME, SCRIPT_KEY)


def create_folder(filepath, isfile=False):
    """Creates folder for file or folder at `filepath` if necessary.

    `isfile` is a boolean to signal whether the filepath is a file or folder.
    """

    if isfile:
        folder = os.path.dirname(filepath)
    else:
        folder = filepath

    try:
        os.makedirs(folder)
    except OSError as e:
        if e.errno != errno.EEXIST:
            raise e

    return filepath

You can run the test with py.test test_tools.py, the -v and -s flags are helpful. You may also need to pip install pytest first.

Answer 1

I often refactor my tests based on what I already written and at the beginning I start with tests like yours:

def test_get_shotgun_returns_value(shotgun):

After time though, I almost always delete those trivial cases, because every other test will fail if tested object is not created. This tests don't carry much value with them and also test are often run in random order, so that's another argument why this is not very useful.

I would also have test_fixture for every test with directories to "purge" temporaries from previous runs, so you're ALWAYS sure that you have same conditions

Names of your tests aren't the greatest - I don't know what they mean.

ProgressBar tests - you always do the same thing at the beginning of the test - extract it.

One more thing - do they test something? I don't have Shotgun installed, but I think that tampering with arguments of functions wouldn't do much.

You don't have any assertions in your ProgressBar tests - this doesn't feel right. Maybe you should return string and check it with some kind of prepared string?

And I think the most important thing I've learned is this - make variable names meaningful. Is ProgressBar(100) somehow different than ProgressBar(10)? If it is the case, then call them differently. Is 0.1 some kind of special interval? Assign it to a variable and make reader sure.

And one last thing - please don't use if's in UTs. It is hard enough to follow code logic and making ifs in unit tests can be painful.

Answer 2

Your tests could be broken up a bit more. Consider

def test_ProgressBar_validate_input():
    with pytest.raises(ValueError):
        ProgressBar("zero")
    with pytest.raises(ValueError):
        ProgressBar(None)
    with pytest.raises(ValueError):
        ProgressBar(-1213)

There are really three distinct tests here - that it is not a string name for a number, that it is not non-numeric, and that it is a not-negative integer. Those should each be distinct tests; that way if one fails there is no ambiguity as to what was the source of the problem.

Then you can also give more descriptive names. It would end up looking something like this

def test_ProgressBar_raises_if_negative():
    with pytest.raises(ValueError):
        ProgressBar(-1213)

def test_ProgressBar_raises_if_string_name():
    with pytest.raises(ValueError):
        ProgressBar("zero")

def test_ProgressBar_raises_if_not_convertable():
    with pytest.raises(ValueError):
        ProgressBar(None)

As an aside, I don't really see any value in the second and third tests being distinct - they seem to be basically the same thing (calling int on them fails).

I also don't like that you're actually connecting to Shotgun for your unit tests - connecting to services, databases, etc in a test makes it more of an integration test, and it can slow things down (connecting to a real service takes time) and can also cause problems if the service is the problem, not your code. If you use Shotgun for something inside of another app and want to test that functionality, then it should use DI and inject a mock service during testing.

The biggest problem is that your test just test's Shotgun's methods, which presumably that library should already have tested. Writing your own tests would only add value if their library is untested or unstable, and in those cases you might not want to use that library.