launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad-buildd/fake-backend into lp:launchpad-buildd with lp:~cjwatson/launchpad-buildd/refactor-fakeslave-waitingfiles as a prerequisite.

Commit message:
Add a FakeBackend, for use in tests of Operation subclass logic.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad-buildd/fake-backend/+merge/328621

I'd previously had the tests for individual operations spell out what the chroot backend should be doing at the level of subprocess calls and similar.  This required a lot of repetition of the tests of the chroot backend itself, and was going to require even more repetitions once we have a LXD backend.  Having a backend implementation that meets the contract of a Backend while doing nothing except remembering how it was called means that the operation tests can instead just be unit tests of the operation logic.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad-buildd/fake-backend into lp:launchpad-buildd.

=== modified file 'lpbuildd/target/backend.py'
--- lpbuildd/target/backend.py	2017-08-05 00:17:59 +0000
+++ lpbuildd/target/backend.py	2017-08-05 00:17:59 +0000
@@ -27,6 +27,10 @@
         if name == "chroot":
             from lpbuildd.target.chroot import Chroot
             backend_factory = Chroot
+        elif name == "fake":
+            # Only for use in tests.
+            from lpbuildd.tests.fakeslave import FakeBackend
+            backend_factory = FakeBackend
         else:
             raise KeyError("Unknown backend: %s" % name)
         return backend_factory(build_id, series=series, arch=arch)

=== modified file 'lpbuildd/target/kill_processes.py'
--- lpbuildd/target/kill_processes.py	2017-08-05 00:17:59 +0000
+++ lpbuildd/target/kill_processes.py	2017-08-05 00:17:59 +0000
@@ -21,3 +21,4 @@
         logger.info(
             "Scanning for processes to kill in build %s", self.args.build_id)
         self.backend.kill_processes()
+        return 0

=== modified file 'lpbuildd/target/operation.py'
--- lpbuildd/target/operation.py	2017-08-05 00:17:59 +0000
+++ lpbuildd/target/operation.py	2017-08-05 00:17:59 +0000
@@ -29,7 +29,7 @@
     def make_parser(self):
         parser = ArgumentParser(description=self.description)
         parser.add_argument(
-            "--backend", choices=["chroot", "lxd"],
+            "--backend", choices=["chroot", "lxd", "fake"],
             help="use this type of backend")
         parser.add_argument(
             "--series", metavar="SERIES", help="operate on series SERIES")

=== modified file 'lpbuildd/target/tests/test_add_trusted_keys.py'
--- lpbuildd/target/tests/test_add_trusted_keys.py	2017-08-05 00:17:59 +0000
+++ lpbuildd/target/tests/test_add_trusted_keys.py	2017-08-05 00:17:59 +0000
@@ -5,10 +5,6 @@
 
 import io
 
-from fixtures import (
-    EnvironmentVariable,
-    MockPatchObject,
-    )
 from testtools import TestCase
 
 from lpbuildd.target.add_trusted_keys import AddTrustedKeys
@@ -17,19 +13,12 @@
 class TestAddTrustedKeys(TestCase):
 
     def test_add_trusted_keys(self):
-        self.useFixture(EnvironmentVariable("HOME", "/expected/home"))
-        args = ["--backend=chroot", "--series=xenial", "--arch=amd64", "1"]
+        args = ["--backend=fake", "--series=xenial", "--arch=amd64", "1"]
         input_file = io.BytesIO()
         add_trusted_keys = AddTrustedKeys(args=args, input_file=input_file)
-        # XXX cjwatson 2017-07-29: With a newer version of fixtures we could
-        # mock this at the subprocess level instead, but at the moment doing
-        # that wouldn't allow us to test stdin.
-        mock_backend_run = self.useFixture(
-            MockPatchObject(add_trusted_keys.backend, "run")).mock
-        add_trusted_keys.run()
-
-        self.assertEqual(2, len(mock_backend_run.mock_calls))
-        mock_backend_run.assert_has_calls([
+        self.assertEqual(0, add_trusted_keys.run())
+        expected_run = [
             ((["apt-key", "add", "-"],), {"stdin": input_file}),
             ((["apt-key", "list"],), {}),
-            ])
+            ]
+        self.assertEqual(expected_run, add_trusted_keys.backend.run.calls)

=== modified file 'lpbuildd/target/tests/test_create.py'
--- lpbuildd/target/tests/test_create.py	2017-08-05 00:17:59 +0000
+++ lpbuildd/target/tests/test_create.py	2017-08-05 00:17:59 +0000
@@ -3,8 +3,6 @@
 
 __metaclass__ = type
 
-from fixtures import EnvironmentVariable
-from systemfixtures import FakeProcesses
 from testtools import TestCase
 
 from lpbuildd.target.create import Create
@@ -13,18 +11,10 @@
 class TestCreate(TestCase):
 
     def test_succeeds(self):
-        self.useFixture(EnvironmentVariable("HOME", "/expected/home"))
-        processes_fixture = self.useFixture(FakeProcesses())
-        processes_fixture.add(lambda _: {}, name="sudo")
         args = [
-            "--backend=chroot", "--series=xenial", "--arch=amd64", "1",
+            "--backend=fake", "--series=xenial", "--arch=amd64", "1",
             "/path/to/tarball"]
-        Create(args=args).run()
-
-        expected_args = [
-            ["sudo", "tar", "-C", "/expected/home/build-1",
-             "-xf", "/path/to/tarball"],
-            ]
+        create = Create(args=args)
+        self.assertEqual(0, create.run())
         self.assertEqual(
-            expected_args,
-            [proc._args["args"] for proc in processes_fixture.procs])
+            [(("/path/to/tarball",), {})], create.backend.create.calls)

=== modified file 'lpbuildd/target/tests/test_kill_processes.py'
--- lpbuildd/target/tests/test_kill_processes.py	2017-08-05 00:17:59 +0000
+++ lpbuildd/target/tests/test_kill_processes.py	2017-08-05 00:17:59 +0000
@@ -3,32 +3,16 @@
 
 __metaclass__ = type
 
-import os.path
-import signal
-
-from fixtures import EnvironmentVariable
-from systemfixtures import FakeFilesystem
 from testtools import TestCase
-from testtools.matchers import DirContains
 
 from lpbuildd.target.kill_processes import KillProcesses
-from lpbuildd.target.tests.testfixtures import KillFixture
 
 
 class TestKillProcesses(TestCase):
 
     def test_succeeds(self):
-        self.useFixture(EnvironmentVariable("HOME", "/expected/home"))
-        fs_fixture = self.useFixture(FakeFilesystem())
-        fs_fixture.add("/expected")
-        os.makedirs("/expected/home/build-1/chroot-autobuild")
-        fs_fixture.add("/proc")
-        os.mkdir("/proc")
-        os.mkdir("/proc/10")
-        os.symlink("/expected/home/build-1/chroot-autobuild", "/proc/10/root")
-        kill_fixture = self.useFixture(KillFixture())
-        args = ["--backend=chroot", "--series=xenial", "--arch=amd64", "1"]
-        KillProcesses(args=args).run()
-
-        self.assertEqual([(10, signal.SIGKILL)], kill_fixture.kills)
-        self.assertThat("/proc", DirContains([]))
+        args = ["--backend=fake", "--series=xenial", "--arch=amd64", "1"]
+        kill_processes = KillProcesses(args=args)
+        self.assertEqual(0, kill_processes.run())
+        self.assertEqual(
+            [((), {})], kill_processes.backend.kill_processes.calls)

=== modified file 'lpbuildd/target/tests/test_override_sources_list.py'
--- lpbuildd/target/tests/test_override_sources_list.py	2017-08-05 00:17:59 +0000
+++ lpbuildd/target/tests/test_override_sources_list.py	2017-08-05 00:17:59 +0000
@@ -5,45 +5,24 @@
 
 from textwrap import dedent
 
-from fixtures import (
-    EnvironmentVariable,
-    MockPatchObject,
-    )
 from testtools import TestCase
 
 from lpbuildd.target.override_sources_list import OverrideSourcesList
-from lpbuildd.tests.fakeslave import FakeMethod
-
-
-class MockCopyIn(FakeMethod):
-
-    def __init__(self, *args, **kwargs):
-        super(MockCopyIn, self).__init__(*args, **kwargs)
-        self.source_bytes = None
-
-    def __call__(self, source_path, *args, **kwargs):
-        with open(source_path, "rb") as source:
-            self.source_bytes = source.read()
-        return super(MockCopyIn, self).__call__(source_path, *args, **kwargs)
 
 
 class TestOverrideSourcesList(TestCase):
 
     def test_succeeds(self):
-        self.useFixture(EnvironmentVariable("HOME", "/expected/home"))
         args = [
-            "--backend=chroot", "--series=xenial", "--arch=amd64", "1",
+            "--backend=fake", "--series=xenial", "--arch=amd64", "1",
             "deb http://archive.ubuntu.com/ubuntu xenial main",
             "deb http://ppa.launchpad.net/launchpad/ppa/ubuntu xenial main",
             ]
         override_sources_list = OverrideSourcesList(args=args)
-        mock_copy_in = self.useFixture(MockPatchObject(
-            override_sources_list.backend, "copy_in", new=MockCopyIn())).mock
-        override_sources_list.run()
-
-        self.assertEqual(dedent("""\
-            deb http://archive.ubuntu.com/ubuntu xenial main
-            deb http://ppa.launchpad.net/launchpad/ppa/ubuntu xenial main
-            """).encode("UTF-8"), mock_copy_in.source_bytes)
-        self.assertEqual(
-            ("/etc/apt/sources.list",), mock_copy_in.extract_args()[0][1:])
+        self.assertEqual(0, override_sources_list.run())
+        self.assertEqual({
+            "/etc/apt/sources.list": dedent("""\
+                deb http://archive.ubuntu.com/ubuntu xenial main
+                deb http://ppa.launchpad.net/launchpad/ppa/ubuntu xenial main
+                """).encode("UTF-8"),
+            }, override_sources_list.backend.copied_in)

=== modified file 'lpbuildd/target/tests/test_remove.py'
--- lpbuildd/target/tests/test_remove.py	2017-08-05 00:17:59 +0000
+++ lpbuildd/target/tests/test_remove.py	2017-08-05 00:17:59 +0000
@@ -3,8 +3,6 @@
 
 __metaclass__ = type
 
-from fixtures import EnvironmentVariable
-from systemfixtures import FakeProcesses
 from testtools import TestCase
 
 from lpbuildd.target.remove import Remove
@@ -13,13 +11,7 @@
 class TestRemove(TestCase):
 
     def test_succeeds(self):
-        self.useFixture(EnvironmentVariable("HOME", "/expected/home"))
-        processes_fixture = self.useFixture(FakeProcesses())
-        processes_fixture.add(lambda _: {}, name="sudo")
-        args = ["--backend=chroot", "--series=xenial", "--arch=amd64", "1"]
-        Remove(args=args).run()
-
-        expected_args = [["sudo", "rm", "-rf", "/expected/home/build-1"]]
-        self.assertEqual(
-            expected_args,
-            [proc._args["args"] for proc in processes_fixture.procs])
+        args = ["--backend=fake", "--series=xenial", "--arch=amd64", "1"]
+        remove = Remove(args=args)
+        self.assertEqual(0, remove.run())
+        self.assertEqual([((), {})], remove.backend.remove.calls)

=== modified file 'lpbuildd/target/tests/test_start.py'
--- lpbuildd/target/tests/test_start.py	2017-08-05 00:17:59 +0000
+++ lpbuildd/target/tests/test_start.py	2017-08-05 00:17:59 +0000
@@ -3,13 +3,6 @@
 
 __metaclass__ = type
 
-import os.path
-
-from fixtures import EnvironmentVariable
-from systemfixtures import (
-    FakeFilesystem,
-    FakeProcesses,
-    )
 from testtools import TestCase
 
 from lpbuildd.target.start import Start
@@ -18,22 +11,7 @@
 class TestStart(TestCase):
 
     def test_succeeds(self):
-        self.useFixture(EnvironmentVariable("HOME", "/expected/home"))
-        processes_fixture = self.useFixture(FakeProcesses())
-        processes_fixture.add(lambda _: {}, name="sudo")
-        fs_fixture = self.useFixture(FakeFilesystem())
-        fs_fixture.add("/etc")
-        os.mkdir("/etc")
-        for etc_name in ("hosts", "hostname", "resolv.conf.real"):
-            with open(os.path.join("/etc", etc_name), "w") as etc_file:
-                etc_file.write("%s\n" % etc_name)
-            os.chmod(os.path.join("/etc", etc_name), 0o644)
-        os.symlink("resolv.conf.real", "/etc/resolv.conf")
-        args = ["--backend=chroot", "--series=xenial", "--arch=amd64", "1"]
-        Start(args=args).run()
-
-        # Tested in more detail in lpbuildd.target.tests.test_chroot.
-        self.assertIn(
-            ["sudo", "mount", "-t", "proc", "none",
-             "/expected/home/build-1/chroot-autobuild/proc"],
-            [proc._args["args"] for proc in processes_fixture.procs])
+        args = ["--backend=fake", "--series=xenial", "--arch=amd64", "1"]
+        start = Start(args=args)
+        self.assertEqual(0, start.run())
+        self.assertEqual([((), {})], start.backend.start.calls)

=== modified file 'lpbuildd/target/tests/test_stop.py'
--- lpbuildd/target/tests/test_stop.py	2017-08-05 00:17:59 +0000
+++ lpbuildd/target/tests/test_stop.py	2017-08-05 00:17:59 +0000
@@ -3,36 +3,33 @@
 
 __metaclass__ = type
 
-import os.path
+from textwrap import dedent
 
-from fixtures import EnvironmentVariable
-from systemfixtures import (
-    FakeFilesystem,
-    FakeProcesses,
-    )
+from fixtures import FakeLogger
 from testtools import TestCase
+from testtools.matchers import StartsWith
 
+from lpbuildd.target.backend import BackendException
 from lpbuildd.target.stop import Stop
-from lpbuildd.target.tests.testfixtures import SudoUmount
 
 
 class TestStop(TestCase):
 
     def test_succeeds(self):
-        self.useFixture(EnvironmentVariable("HOME", "/expected/home"))
-        processes_fixture = self.useFixture(FakeProcesses())
-        processes_fixture.add(SudoUmount(), name="sudo")
-        fs_fixture = self.useFixture(FakeFilesystem())
-        fs_fixture.add("/proc")
-        os.mkdir("/proc")
-        with open("/proc/mounts", "w") as mounts_file:
-            mounts_file.write(
-                "none {chroot}/proc proc rw,relatime 0 0".format(
-                    chroot="/expected/home/build-1/chroot-autobuild"))
-        args = ["--backend=chroot", "--series=xenial", "--arch=amd64", "1"]
-        Stop(args=args).run()
+        args = ["--backend=fake", "--series=xenial", "--arch=amd64", "1"]
+        stop = Stop(args=args)
+        self.assertEqual(0, stop.run())
+        self.assertEqual([((), {})], stop.backend.stop.calls)
 
-        # Tested in more detail in lpbuildd.target.tests.test_chroot.
-        self.assertIn(
-            ["sudo", "umount", "/expected/home/build-1/chroot-autobuild/proc"],
-            [proc._args["args"] for proc in processes_fixture.procs])
+    def test_fails(self):
+        logger = self.useFixture(FakeLogger())
+        args = ["--backend=fake", "--series=xenial", "--arch=amd64", "1"]
+        stop = Stop(args=args)
+        stop.backend.stop.failure = BackendException
+        self.assertEqual(1, stop.run())
+        self.assertEqual([((), {})], stop.backend.stop.calls)
+        self.assertThat(logger.output, StartsWith(dedent("""\
+            Stopping target for build 1
+            Failed to stop target
+            Traceback (most recent call last):
+            """)))

=== modified file 'lpbuildd/target/tests/test_update.py'
--- lpbuildd/target/tests/test_update.py	2017-08-05 00:17:59 +0000
+++ lpbuildd/target/tests/test_update.py	2017-08-05 00:17:59 +0000
@@ -3,84 +3,79 @@
 
 __metaclass__ = type
 
+import subprocess
 import time
 
-from fixtures import (
-    EnvironmentVariable,
-    FakeLogger,
-    )
-from systemfixtures import (
-    FakeProcesses,
-    FakeTime,
-    )
+from fixtures import FakeLogger
+from systemfixtures import FakeTime
 from testtools import TestCase
+from testtools.matchers import (
+    ContainsDict,
+    Equals,
+    MatchesDict,
+    MatchesListwise,
+    )
 
 from lpbuildd.target.update import Update
+from lpbuildd.tests.fakeslave import FakeMethod
+
+
+class RanAptGet(MatchesListwise):
+
+    def __init__(self, args_list):
+        super(RanAptGet, self).__init__([
+            MatchesListwise([
+                Equals((["/usr/bin/apt-get"] + args,)),
+                ContainsDict({
+                    "env": MatchesDict({
+                        "LANG": Equals("C"),
+                        "DEBIAN_FRONTEND": Equals("noninteractive"),
+                        "TTY": Equals("unknown"),
+                        }),
+                    }),
+                ]) for args in args_list
+            ])
 
 
 class TestUpdate(TestCase):
 
     def test_succeeds(self):
-        self.useFixture(EnvironmentVariable("HOME", "/expected/home"))
-        processes_fixture = self.useFixture(FakeProcesses())
-        processes_fixture.add(lambda _: {}, name="sudo")
         self.useFixture(FakeTime())
         start_time = time.time()
-        args = ["--backend=chroot", "--series=xenial", "--arch=amd64", "1"]
-        Update(args=args).run()
+        args = ["--backend=fake", "--series=xenial", "--arch=amd64", "1"]
+        update = Update(args=args)
+        self.assertEqual(0, update.run())
 
-        apt_get_args = [
-            "sudo", "/usr/sbin/chroot",
-            "/expected/home/build-1/chroot-autobuild",
-            "linux64", "env",
-            "LANG=C",
-            "DEBIAN_FRONTEND=noninteractive",
-            "TTY=unknown",
-            "/usr/bin/apt-get",
-            ]
         expected_args = [
-            apt_get_args + ["-uy", "update"],
-            apt_get_args + [
-                "-o", "DPkg::Options::=--force-confold", "-uy", "--purge",
-                "dist-upgrade",
-                ],
+            ["-uy", "update"],
+            ["-o", "DPkg::Options::=--force-confold", "-uy", "--purge",
+             "dist-upgrade"],
             ]
-        self.assertEqual(
-            expected_args,
-            [proc._args["args"] for proc in processes_fixture.procs])
+        self.assertThat(update.backend.run.calls, RanAptGet(expected_args))
         self.assertEqual(start_time, time.time())
 
     def test_first_run_fails(self):
+        class FailFirstTime(FakeMethod):
+            def __call__(self, run_args, *args, **kwargs):
+                super(FailFirstTime, self).__call__(run_args, *args, **kwargs)
+                if len(self.calls) == 1:
+                    raise subprocess.CalledProcessError(1, run_args)
+
         logger = self.useFixture(FakeLogger())
-        self.useFixture(EnvironmentVariable("HOME", "/expected/home"))
-        processes_fixture = self.useFixture(FakeProcesses())
-        apt_get_proc_infos = iter([{"returncode": 1}, {}, {}])
-        processes_fixture.add(lambda _: next(apt_get_proc_infos), name="sudo")
         self.useFixture(FakeTime())
         start_time = time.time()
-        args = ["--backend=chroot", "--series=xenial", "--arch=amd64", "1"]
-        Update(args=args).run()
+        args = ["--backend=fake", "--series=xenial", "--arch=amd64", "1"]
+        update = Update(args=args)
+        update.backend.run = FailFirstTime()
+        self.assertEqual(0, update.run())
 
-        apt_get_args = [
-            "sudo", "/usr/sbin/chroot",
-            "/expected/home/build-1/chroot-autobuild",
-            "linux64", "env",
-            "LANG=C",
-            "DEBIAN_FRONTEND=noninteractive",
-            "TTY=unknown",
-            "/usr/bin/apt-get",
-            ]
         expected_args = [
-            apt_get_args + ["-uy", "update"],
-            apt_get_args + ["-uy", "update"],
-            apt_get_args + [
-                "-o", "DPkg::Options::=--force-confold", "-uy", "--purge",
-                "dist-upgrade",
-                ],
+            ["-uy", "update"],
+            ["-uy", "update"],
+            ["-o", "DPkg::Options::=--force-confold", "-uy", "--purge",
+             "dist-upgrade"],
             ]
-        self.assertEqual(
-            expected_args,
-            [proc._args["args"] for proc in processes_fixture.procs])
+        self.assertThat(update.backend.run.calls, RanAptGet(expected_args))
         self.assertEqual(
             "Updating target for build 1\n"
             "Waiting 15 seconds and trying again ...\n",

=== modified file 'lpbuildd/tests/fakeslave.py'
--- lpbuildd/tests/fakeslave.py	2017-08-05 00:17:59 +0000
+++ lpbuildd/tests/fakeslave.py	2017-08-05 00:17:59 +0000
@@ -11,6 +11,8 @@
 import os
 import shutil
 
+from lpbuildd.target.backend import Backend
+
 
 class FakeMethod:
     """Catch any function or method call, and record the fact.
@@ -102,3 +104,21 @@
 
     def getArch(self):
         return 'i386'
+
+
+class FakeBackend(Backend):
+
+    def __init__(self, *args, **kwargs):
+        super(FakeBackend, self).__init__(*args, **kwargs)
+        fake_methods = (
+            "create", "start",
+            "run",
+            "kill_processes", "stop", "remove",
+            )
+        for fake_method in fake_methods:
+            setattr(self, fake_method, FakeMethod())
+        self.copied_in = {}
+
+    def copy_in(self, source_path, target_path):
+        with open(source_path, "rb") as source:
+            self.copied_in[target_path] = source.read()