Skip to main content
Code Review

Return to Answer

added 192 characters in body
Source Link
J_H
J_H
  • 35.2k
  • 3
  • 32
  • 129

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.

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.

added 920 characters in body
Source Link
J_H
J_H
  • 35.2k
  • 3
  • 32
  • 129

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.

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.

Source Link
J_H
J_H
  • 35.2k
  • 3
  • 32
  • 129

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.