Merge ~jugmac00/launchpad:add_how_to_test_scripts into launchpad:master

[Jürgen Gmach]

Add how-to test scripts

[Jürgen Gmach (jugmac00)]

screenshot see https://chat.canonical.com/canonical/pl/9hsbtrde3tdtjm69gerujgrfto

[Colin Watson (cjwatson)]

As well as the detailed point about the multiple versions of `run_script`, I also have a broader concern here, in that I think this documentation is too brief and ends up emphasizing the wrong thing as a result. In some cases we do indeed need to write functional tests that call scripts as subprocesses. However, it shouldn't be encouraged as the first thing that people reach for, for a couple of reasons:

 * It's very slow. If you run a script as a subprocess, it has to import large chunks of Launchpad from scratch, set up ZCML, and so on.
 * Tests that do this are limited to using the command-line interface exposed by the script and observing it as a black box. That can be a good thing, but in the majority of cases we want something more flexible.

Launchpad scripts are all written as subclasses of `LaunchpadScript` that declares the script-specific options and has methods that do the actual work. So, in most cases, the way to go about testing scripts is to write the top-level executable script as a very thin layer over such a class (look at `scripts/sync-signingkeys.py`, for example) and then have almost all the tests be written as unit tests of that class that just do all the testing in-process: this is much faster and much more flexible. Admittedly, it's usually a good idea to also have a smoke test case using `run_script`, but there should probably only be about one of those: it shouldn't be how most of the tests are written.

I'd really like this documentation to explain this, because otherwise I anticipate either having to make lots of code review comments to future developers who read this documentation, or else the test suite getting significantly slower.

review: Needs Fixing

[Jürgen Gmach (jugmac00)]

Thanks for the review.

For context, while working on adding tests for the uct importer, I have only left the wrapper in place, but I found no better way to test the following line
https://git.launchpad.net/launchpad/tree/scripts/uct-import.py#n38
as this directly uses `parser.error` from https://docs.python.org/3/library/optparse.html#module-optparse which exits with error status 2. But we can leave this discussion for a review of the upcoming MP.

While there are better ways to test with argparse, it is not what we currently use for the scripts. I recall Andrey was going to migrate the scripts :-/ For now I do not know a better way. FWIW I think we should convert all scripts at once, but I do not see this as an urgent task.

I was not aware that we have multiple instances of basically the same test helper. That is wild, and I think it is great idea to unify them, but that would be out of scope for this MP.

I created a ticket (https://warthogs.atlassian.net/browse/LP-1032). I currently prefer to avoid that rabbit hole.

I agree with you that using `shell=True` is generally a bad idea. Updating the documentation to explicitly using the instance with `shell=True` seems a bit counterintuitive. I would suggest to keep the current import until LP-1032 is done.

That leaves us with the intended audience of this piece of documentation. Indeed, I did not have very junior developers in mind. While we are currently only experienced devs in our team, we will probably hire some in near future. I would not be afraid "to make lots of code review comments to future developers", as after the first one this documentation would be updated.

Colin, given https://warthogs.atlassian.net/browse/LP-1032 what exactly needs to change to get an approval?

[Colin Watson (cjwatson)]

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)

[Jürgen Gmach (jugmac00)]

Thanks for the review, and sorry for mixing up the different `run_scripts` helpers.

I updated both the warning/note and the code example.

I hope you are ok that I did not introduce the testtools goodie (addDetail). While I think it is good to know about, it is not directly related to the topic I wanted to document.

Imho it would be a great idea compile a quick overview of testtools/fixtures highlights, or at least mention them at https://dev.launchpad.net/Testing

[Colin Watson (cjwatson)]

review: Approve

diff --git a/doc/how-to/index.rst b/doc/how-to/index.rst
index 5ebddba..ea7b441 100644
--- a/doc/how-to/index.rst
+++ b/doc/how-to/index.rst
@@ -25,6 +25,7 @@ Common development tasks
    land-update-for-loggerhead
    update-configuration-for-testing
    doc-theme
+   testing-scripts
 
 Operating development instances
 -------------------------------
diff --git a/doc/how-to/testing-scripts.rst b/doc/how-to/testing-scripts.rst
new file mode 100644
index 0000000..38eb115
--- /dev/null
+++ b/doc/how-to/testing-scripts.rst
@@ -0,0 +1,32 @@
+===================
+Testing CLI scripts
+===================
+
+.. note::
+
+    This is about testing scripts within the Launchpad context,
+    not a general guide about testing CLI scripts.
+
+    ``run_script`` should only be used for smoke tests as these kinds of
+    end-to-end tests slow down the test suite.
+
+Launchpad offers a convenient test helper for testing scripts,
+which are usually found in the top level ``scripts`` folder.
+
+.. code-block:: python
+
+    from lp.testing.script import run_script
+
+    returncode, stdout, stderr = run_script(
+        script="scripts/script.py",
+        args=["--help"],
+    )
+
+For more available options please have a look at the source code of
+``run_script``.
+
+.. note::
+
+    ``run_script`` uses a subprocess to run the script. This has impact on both
+    debugging via a Python debugger and on using ``strace``
+    (use ``strace -f``).