Code review comment for ~jugmac00/launchpad:add_how_to_test_scripts

Revision history for this message
Colin Watson (cjwatson) wrote :

It's entirely possible to unit-test things that use `parser.error`, if a little bit ugly. `parser.error` calls `sys.exit` which raises `SystemExit`, so all you have to do is catch that exception and assert whether it's raised or not, as appropriate. If you grep for `SystemExit` you'll find a few examples of this, such as `lib/lp/scripts/utilities/tests/test_versioninfo.py`.

Note that the instance that uses `shell=True` is the one in `lib/lp/testing/__init__.py`, which wasn't the instance that I recommended switching to. I recommended the one in `lib/lp/testing/script.py`, which doesn't use `shell=True`.

I appreciate that it doesn't make sense to block on a complete refactoring, certainly. The bare minimum I'd like to see in this branch before approving it is to make it clear at the start of the document that this is only for smoke tests of scripts, and that most script tests should be done by unit-testing the relevant class instead.

I also think it wouldn't be too unreasonable to adjust this document so that it recommends the use of the version in `lp.testing.script` instead, which would look like this:

    from testtools.content import text_content

    from lp.testing.script import run_script

    returncode, stdout, stderr = run_script(
        script="scripts/script.py", args=["--help"]
    )
    self.addDetail("stdout", text_content(stdout))
    self.addDetail("stderr", text_content(stderr))
    self.assertEqual(0, returncode)

« Back to merge proposal