Integration test for a script that writes a YAML file

Question

I have a script bin/create_workflow_input.py that is part of a larger application that launches bioinformatics workflows. The script itself (not shown) writes into STDOUT a YAML file that serves as input for launching a workflow.

I wrote an integration test for this script (below). I would like to make this integration test more pythonic and easier to maintain. Performance issues, such as the overhead of subprocess.run, are irrelevant. The resulting output yaml file is small (under 1kb), and easily fits in RAM for comparison with the expected yaml file.

To run the test, I use mkdir -p test/obs ; pytest.

To clean up, rm -rf test/obs

from difflib import unified_diff
import re
import subprocess

# Run the script to be tested, capture its stdout to a file:

def test_run_create_workflow_input():

    cmd = re.sub(
        r'\s+', r' ',
        """bin/create_workflow_input.py
        --fastq1_glob 'test/fixtures/create_workflow_input/*.1.fastq.gz'
        --fastq2_glob 'test/fixtures/create_workflow_input/*.2.fastq.gz'
        --params test/fixtures/create_workflow_input/foo.params.txt
        --template test/fixtures/create_workflow_input/foo.template.yml
        > test/obs/workflow_input.yml""")
    print(cmd)
    try:
        res = subprocess.run(cmd, shell=True, check=True)
        returncode = res.returncode
    except subprocess.CalledProcessError as e:
        returncode = e.returncode
        error_msg = str(e)

    assert returncode == 0, f'Failed command={cmd}, returncode={returncode}, error_msg={error_msg}'

# Compare observed and expected script output:

def test_workflow_input():
    with open('test/obs/workflow_input.yml') as f:
        obs_lines = f.readlines()
    with open('test/fixtures/workflow_input.yml') as f:
        exp_lines = f.readlines()
    diff = list(unified_diff(obs_lines, exp_lines))
    assert diff == [], 'Unexpected create_workflow_input.py output:\n' + ''.join(diff)

Answer 1

r-string

Style nit:

    cmd = re.sub(
        r'\s+', r' ',
        """bin/ ...

Habitually using raw strings for regexes is terrific, thank you for doing that.

Prefer a simple ' ' for that second argument, which is just plain text. (In contrast, prefer to always make regexes r-strings, even if they do not yet have any special characters where it would make a difference. It is a kindness to the reader.)

Consider breaking out a tiny trivial helper which turns newlines into SPACE, since this is just a source code formatting typographic detail we're dealing with here.

DRY

It's slightly tedious to see repetitions of test/fixtures/create_workflow_input. Consider assigning it to in_folder, and then an f-string can expand several mentions of it.

Also, the print(cmd) debug is perhaps a bit too chatty.

error paths

It's not completely obvious to me that the try is a win.

        returncode = res.returncode
    except ... as e:
        returncode = e.returncode

Then later we test whether that code is zero. Specifying , check=True is usually exactly what you want, so nothing will sneak past you. But here? Well, you're checking anyway, so maybe don't even specify it, don't request an exception.

        error_msg = str(e)
        ...
    ... f'Failed ... error_msg={error_msg}'

No need for the str() call, as an f-string will do that anyway.

The else clause and the assert message are tightly linked. If they weren't we would get "NameError: name 'error_msg' is not defined". This code is too tricky. If you retain the try, then do the error reporting within the else clause. It would be better to raise ValueError(...) or an app specific error, than to assert. I guess this is pytest showing its influence. I feel the current assert should "never" trigger, it is "impossible", while the one down in test_workflow_input() is essentially the self.assertEqual() check of results which might often show a Red test result.

f-string shorthand

Your f'... returncode={returncode}, ...' could be more conveniently expressed
with f'... {returncode=}, ...'

In recent interpreter versions, they mean the same thing. It's just shorthand for a commonly seen pattern.

docstrings

# Run the script to be tested, capture its stdout to a file:
def test_run_create_workflow_input():
...
# Compare observed and expected script output:
def test_workflow_input():

Those aren't really comments which explain some tricky code. They give the contract for each function, so they should be docstrings. Please """spell them properly""".

use Path

        > test/obs/workflow_input.yml""")
    ...
    with open('test/obs/workflow_input.yml') as f:

It's not terrible that we're repeating the filename. But it does make me slightly sad. Consider defining a Path which both use, or consider returning a Path for subsequent use.

    with open('test/obs/workflow_input.yml') as f:
        obs_lines = f.readlines()
    with open('test/fixtures/workflow_input.yml') as f:
        exp_lines = f.readlines()

Using a context handler is certainly a Good Thing. Consider making those .yml names Paths rather than strs. Then you could take advantage of the .read_text() convenience method offered by Path.

sequence

This troubles me deeply.

def test_run_create_workflow_input():
        ...
        > test/obs/workflow_input.yml""")
...
def test_workflow_input():
    with open('test/obs/workflow_input.yml') as f:

Please understand that pytest makes no guarantee about the sequence in which it executes each test_foo() function. The sequence is entirely arbitrary, and can change tomorrow, or when pip installs a new pytest version, or when you add a new pytest plugin.

You cannot rely on the order of execution, even if you observe that "it just works" when you run it today. The OP code is not maintainable, and should not be merged down to main in its current form.

The fix is very simple. Just make one of the functions a helper that is called by the other, instead of being called by pytest. That means that one of the functions should omit the test_ prefix.

Pypi does offer a pytest-order plugin. But I wouldn't advise going down that route. Best practice for unit tests, across {java, php, ruby} and other languages, is that we usually want each test to stand on its own. The sequence could be some random permutation each time we run it, and tests should still remain Green.

Of course, you don't need the pytest runner; it doesn't seem to be adding a ton of value-add in your use case. You could simply def main(): and execute this as an ordinary script.

Answer 2

DRY

You repeatedly use the following directory path as a string:

test/fixtures/create_workflow_input

Your code would be easier to maintain if you set a variable. You could even make it an input to the function if that makes sense in your code.

A similar suggestion applies for the repeated script file name: create_workflow_input.py.

Documentation

It is great that you have a comment for each function, but it would be more Pythonic to use docstrings as recommended by the PEP-8 style guide:

def test_run_create_workflow_input():
    """ Run the script to be tested, capture its stdout to a file """

Also add a docstring at the top of the code to summarize the purpose of the code.

Naming

f is not a very meaningful variable name. input_file would be a better choice.