Using fixtures in tests

[levje]

After a quick discussion with @AlexVCaron, I am posting my question/suggestion here.

I don't want to switch everyone's perspective on this, as I would like a discussion on what you guys think about it before we change anything (if we change anything at all).

While doing some tests for recent PRs, I was wondering if there was a particular reason we weren't leveraging the power of pytest's fixtures. From what I've seen, most tests are using global variables for pretty much anything that is used or repeated across different tests. One (minor) issue I faced using global variables is that across two different test cases in the same file, you can't create a file with the same name twice when testing for the execution of a script that can't overwrite a file unless you force it to (-f or --force). However, since test cases should be independent from one another, that should theoretically not be a problem as it is often expected for a test case to have a "fresh" start.

The following example causes the tests to fail:

tmp_dir = tempfile.TemporaryDirectory()

def test_one(script_runner, monkeypatch):
    monkeypatch.chdir(os.path.expanduser(tmp_dir.name))
    in_bundle = os.path.join(SCILPY_HOME, 'filtering',
                             'bundle_4')

    # Creates the 'bundle_4_filtered.trk' in the tmpdir
    ret = script_runner.run('scil_tractogram_filter_by_length.py',
                            in_bundle,  'bundle_4_filtered.trk',
                            '--minL', '125', '--maxL', '130',
                            '--out_rejected', 'bundle_4_rejected.trk')
    assert ret.success
    [...]


def test_two(script_runner, monkeypatch):
    monkeypatch.chdir(os.path.expanduser(tmp_dir.name))
    in_bundle = os.path.join(SCILPY_HOME, 'filtering',
                             'bundle_4.trk')

    # This will fail because 'bundle_4_filtered.trk' already exists in the tmpdir
    ret = script_runner.run('scil_tractogram_filter_by_length.py',
                            in_bundle,  'bundle_4_filtered.trk',
                            '--minL', '125', '--maxL', '130',
                            '--out_rejected', 'bundle_4_rejected.trk')
    assert ret.success
    [...]

Now, instead of doing that and having to rename the files to different names across different tests, which could get complicated with more complex and exhaustive tests, we could leverage the power (!) of fixtures. Fixtures can be used for anything that needs to be setup for one or more tests. It allows to create a sort of construction-test-destruction workflow for your tests easily a reusable across many different tests. You can read more about fixtures here.

Now, as I've seen that creating temporary directories is common among our test suite, I've cooked up a simple example to showcase how we could use more of pytest's functionalities. I've illustrated two ways of creating temp directories: 1 by doing it manually using @pytest.fixture which allows us to also specify the scope of that fixture (e.g. re-run before each test? or run only once per session?) as I've also showcased with scope='function' and scope='session'. Another way to do that (especially if you don't do anything special and just want a temp directory), you could use pytest's built-in fixtures that creates a temp directory directly without having to do anything other than "requesting" tmp_path or tmp_path_factory in the tests cases that need a temp directory.

###########################
# This fixture/function will be called BEFORE EACH TEST
@pytest.fixture(scope='function')
def tmp_dir_fixture():
    with tempfile.TemporaryDirectory() as tmp_dir:
        setup_things_for_test()
        
        # yield instead of return to preserve the context
        # in the test that uses this fixture
        yield tmp_dir

        teardown_things_for_test()

# Using the fixture in a test case
def test_one(tmp_dir_fixture):
    output_path = tmp_dir_fixture + '/output.txt'
    do_something(output_path)
    ...

# Equivalent formulation using pytest's built-in fixture
def test_one_builtin(tmp_path):
    # tmp_path is a pathlib.Path object
    output_path = tmp_path / 'output.txt'
    do_something(output_path)
    ...

###########################
# This fixture/function will be called once PER TEST SESSION
@pytest.fixture(scope='session')
def tmp_dir_fixture_session():
    with tempfile.TemporaryDirectory() as tmp_dir:
        setup_things_for_test()
        
        # yield instead of return to preserve the context
        # in the test that uses this fixture
        yield tmp_dir

        teardown_things_for_test()

# Using the fixture in a test case
def test_two(tmp_dir_fixture_session):
    output_path = tmp_dir_fixture_session + '/output.txt'
    do_something(output_path)
    ...

# Equivalent formulation using pytest's built-in fixture
# tmp_path_factory which manages a temporary directory
# across the testing session.
def test_two_builtin(tmp_path_factory):
    # tmp_path_factory is a TempPathFactory object

    # getbasetemp() returns the temporary directory
    # shared across tests in the session.
    base_temp_path = tmp_path_factory.getbasetemp()
    out_file_1 = base_temp_path / 'output1.txt'
    
    # mktemp() creates a new temporary directory
    # under the base temporary directory.
    new_temp_path = tmp_path_factory.mktemp('test1')
    out_file_2 = new_temp_path / 'output2.txt'
    
    do_something(out_file_1, out_file_2)
    ...

This suggestion is probably trying to tackle a problem that's not actually a problem in practice, but it could be a good standard to use such a formulation to simplify the process of writing new tests. There's much more that you can do with pytest's fixtures which you can read about here such as caching information across different test runs for example. Also, I think it's a type of test organization that's used in the industry from what I've seen in my very early career.

Let me know what you think!