Merge lp:~gz/juju-release-tools/test_apply_patches into lp:juju-release-tools

[Martin Packman]

Add test coverage for apply_patches script

Somehow I forgot on Friday that we do actually have test infrastructure for juju-release-tools, so this branch adds coverage. There are some fiddly bits (`with open` is always annoying), and one relevent change with the addition of flushing for the parent process's output. This ensures the ordering of output is correct when mixed with the patch command, which currently is a confusion on the logs.

The addition of gettext for the user informational output is total vanity. Please forgive.

[Curtis Hovey (sinzui)]

Do we need print_now? I have strived to move away from it. I introduced it because the buffered stdout was not appearing near the output of other procs. Per you own example, I log.XXX() which is unbuffered stderr. I print() to stdout for user info. Do we have a buffering problem that this solves?

Removing it might be awkward because I can see a few tests are mocking it.

review: Needs Information (code)

[Martin Packman (gz)]

I agree print_now is ugly and generally the wrong thing.

The buffering issue this fixes can be seen in the log output at tarball creation at present:

    patching file gopkg.in/mgo.v2/session.go
    Applying 1 patches
    Applied patch '001-mgo.v2-issue-277-fix.diff'

The first line is from `patch` when it exits, but it should really appear between the two lines from the parent script for clarity. As stdout is buffered, those are only output when the parent process exits.

So, four possible fixes:

# Write to stderr not stdout
  - This is often the right thing for scripts. Use stdout for results - things that could actually be piped into another command - and use stderr for feedback to the user.
# Use logging module
  - Uses stderr like above, needs extra configuration but can be easier to reuse.
# Use print(flush=True)
  - Python 3.3 and above only
# Use a helper that calls .flush()

So, why did I not write to stderr... well, this is the kind of script I don't expect to have output beyond the statement of activity, so I've tended to put that on stdout. This is much like the patch command itself, which uses stdout. But, lets try the other way.

[lp:~gz/juju-release-tools/test_apply_patches]

323. By Martin Packman on 2016-07-21

Switch to using stderr for apply_patches output

[Curtis Hovey (sinzui)]

I accept print_now as a compromise for now.

> # Write to stderr not stdout
> - This is often the right thing for scripts. Use stdout for results - things that could
> actually be piped into another command - and use stderr for feedback to the user.

Your example is a little different from my recent experience where I did want my
output captured by other scripts.

> # Use logging module
> - Uses stderr like above, needs extra configuration but can be easier to reuse.

I considered this for my recent cases, We still configure a logger without timestamps,

> # Use print(flush=True)
> - Python 3.3 and above only

Been there. I think we can switch when we stop supporting precise or we stop using this
set of scripts on precise

> # Use a helper that calls .flush()

Yep. the helper is the cleanest path for now.

review: Approve (code)

[lp:~gz/juju-release-tools/test_apply_patches]

324. By Martin Packman on 2016-07-21

Remove mostly redundant test for apply_patches

[Curtis Hovey (sinzui)]

Thank you for choosing a solution.

review: Approve (code)

=== modified file 'apply_patches.py'
--- apply_patches.py	2016-07-15 16:28:29 +0000
+++ apply_patches.py	2016-07-21 16:44:36 +0000
@@ -4,6 +4,7 @@
 from __future__ import print_function
 
 from argparse import ArgumentParser
+import gettext
 import os
 import subprocess
 import sys
@@ -41,16 +42,21 @@
     try:
         patches = sorted(os.listdir(args.patchdir))
     except OSError as e:
-        parser.error("Could not list patchdir: {}".format(e))
+        parser.error("Could not list patch directory: {}".format(e))
     if not os.path.isdir(args.srctree):
-        parser.error("Given srctree '{}' not a directory".format(args.srctree))
-    print("Applying {} patches".format(len(patches)))
+        parser.error("Source tree '{}' not a directory".format(args.srctree))
+    patch_count = len(patches)
+    print(gettext.ngettext(
+        u"Applying {} patch", u"Applying {} patches", patch_count).format(
+        patch_count), file=sys.stderr)
     for patch in patches:
         patch_path = os.path.join(args.patchdir, patch)
         if apply_patch(patch_path, args.srctree, args.dry_run, args.verbose):
-            print("Failed to apply patch '{}'".format(patch))
+            print(gettext.gettext(
+                u"Failed to apply patch '{}'").format(patch), file=sys.stderr)
             return 1
-        print("Applied patch '{}'".format(patch))
+        print(gettext.gettext(
+            u"Applied patch '{}'").format(patch), file=sys.stderr)
     return 0
 
 

=== added file 'tests/test_apply_patches.py'
--- tests/test_apply_patches.py	1970-01-01 00:00:00 +0000
+++ tests/test_apply_patches.py	2016-07-21 16:44:36 +0000
@@ -0,0 +1,160 @@
+"""Tests for apply_patches script used in tarball building."""
+
+import contextlib
+import mock
+import os
+import StringIO
+import sys
+import unittest
+
+from apply_patches import (
+    apply_patch,
+    get_arg_parser,
+    main,
+)
+from utils import (
+    temp_dir,
+)
+
+
+class TestArgParser(unittest.TestCase):
+
+    def parse_args(self, args):
+        parser = get_arg_parser()
+        return parser.parse_args(args)
+
+    def test_defaults(self):
+        args = self.parse_args(["patches/", "."])
+        self.assertEqual("patches/", args.patchdir)
+        self.assertEqual(".", args.srctree)
+        self.assertEqual(False, args.dry_run)
+        self.assertEqual(False, args.verbose)
+
+    def test_dry_run(self):
+        args = self.parse_args(["--dry-run", "patches/", "."])
+        self.assertEqual("patches/", args.patchdir)
+        self.assertEqual(".", args.srctree)
+        self.assertEqual(True, args.dry_run)
+        self.assertEqual(False, args.verbose)
+
+    def test_verbose(self):
+        args = self.parse_args(["patches/", ".", "--verbose"])
+        self.assertEqual("patches/", args.patchdir)
+        self.assertEqual(".", args.srctree)
+        self.assertEqual(False, args.dry_run)
+        self.assertEqual(True, args.verbose)
+
+
+class TestApplyPatches(unittest.TestCase):
+
+    def assert_open_only(self, open_mock, filename):
+        self.assertEqual(
+            open_mock.mock_calls,
+            [
+                mock.call(filename),
+                mock.call().__enter__(),
+                mock.call().__exit__(None, None, None),
+            ]
+        )
+
+    def test_with_defaults(self):
+        open_mock = mock.mock_open()
+        with mock.patch("subprocess.call", autospec=True) as call_mock:
+            with mock.patch("apply_patches.open", open_mock, create=True):
+                apply_patch("some.patch", "a/tree")
+        self.assert_open_only(open_mock, "some.patch")
+        call_mock.assert_called_once_with(
+            ["patch", "-f", "-u", "-p1", "-r-"],
+            stdin=open_mock(),
+            cwd="a/tree",
+            )
+
+    def test_dry_run_verbose(self):
+        open_mock = mock.mock_open()
+        with mock.patch("subprocess.call", autospec=True) as call_mock:
+            with mock.patch("apply_patches.open", open_mock, create=True):
+                apply_patch("trial.diff", "a/tree", True, True)
+        self.assert_open_only(open_mock, "trial.diff")
+        call_mock.assert_called_once_with(
+            ["patch", "-f", "-u", "-p1", "-r-", "--dry-run", "--verbose"],
+            stdin=open_mock(),
+            cwd="a/tree",
+            )
+
+    def test_verbose(self):
+        open_mock = mock.mock_open()
+        with mock.patch("subprocess.call", autospec=True) as call_mock:
+            with mock.patch("apply_patches.open", open_mock, create=True):
+                apply_patch("scary.patch", "a/tree", verbose=True)
+        self.assert_open_only(open_mock, "scary.patch")
+        call_mock.assert_called_once_with(
+            ["patch", "-f", "-u", "-p1", "-r-", "--verbose"],
+            stdin=open_mock(),
+            cwd="a/tree",
+            )
+
+
+class TestMain(unittest.TestCase):
+
+    @contextlib.contextmanager
+    def patch_output(self):
+        messages = []
+
+        def fake_print(message, file=None):
+            self.assertEqual(file, sys.stderr)
+            messages.append(message)
+
+        with mock.patch("apply_patches.print", fake_print, create=True):
+            yield messages
+
+    def test_no_patchdir(self):
+        stream = StringIO.StringIO()
+        with temp_dir() as basedir:
+            with self.assertRaises(SystemExit):
+                with mock.patch("sys.stderr", stream):
+                    main(["test", os.path.join(basedir, "missing"), basedir])
+        self.assertRegexpMatches(
+            stream.getvalue(), "Could not list patch directory: .*/missing")
+
+    def test_no_srctree(self):
+        stream = StringIO.StringIO()
+        with temp_dir() as basedir:
+            with self.assertRaises(SystemExit):
+                with mock.patch("sys.stderr", stream):
+                    main(["test", basedir, os.path.join(basedir, "missing")])
+        self.assertRegexpMatches(
+            stream.getvalue(), "Source tree '.*/missing' not a directory")
+
+    def test_one_patch(self):
+        with temp_dir() as basedir:
+            patchdir = os.path.join(basedir, "patches")
+            os.mkdir(patchdir)
+            patchfile = os.path.join(patchdir, "sample.patch")
+            open(patchfile, "w").close()
+            with self.patch_output() as messages:
+                with mock.patch(
+                        "apply_patches.apply_patch", return_value=0,
+                        autospec=True) as ap_mock:
+                    main(["test", patchdir, basedir])
+            ap_mock.assert_called_once_with(patchfile, basedir, False, False)
+            self.assertEqual(messages, [
+                u"Applying 1 patch", u"Applied patch 'sample.patch'"
+            ])
+
+    def test_bad_patches(self):
+        with temp_dir() as basedir:
+            patchdir = os.path.join(basedir, "patches")
+            os.mkdir(patchdir)
+            patch_a = os.path.join(patchdir, "a.patch")
+            open(patch_a, "w").close()
+            patch_b = os.path.join(patchdir, "b.patch")
+            open(patch_b, "w").close()
+            with self.patch_output() as messages:
+                with mock.patch(
+                        "apply_patches.apply_patch", return_value=1,
+                        autospec=True) as ap_mock:
+                    main(["test", "--verbose", patchdir, basedir])
+            ap_mock.assert_called_once_with(patch_a, basedir, False, True)
+            self.assertEqual(messages, [
+                u"Applying 2 patches", u"Failed to apply patch 'a.patch'"
+            ])