Merge lp:~allenap/maas/nose-select-bucket-plugin into lp:~maas-committers/maas/trunk

[Gavin Panella]

New --select-bucket option for the test scripts.

Tests can be split into a configurable number of buckets and then the tests for one of those buckets can be then run. This is a step towards being able to parallelise test runs. The mechanism for choosing buckets is stable so this will work locally or in a distributed environment.

[Mike Pontillo (mpontillo)]

Looks good to me; mostly-minor comments below.

I won't block you on it, but I would seriously consider using a standards-based hash algorithm for better pseudo-random distribution. See comments below.

review: Approve

[Mike Pontillo (mpontillo)]

Argh. Launchpad ate my most important comment, on line ~140 where you do the hash. (Mental note: press little save tickbox-button next time.) Basically I think you should do something like:

from hashlib import sha1

def _getHashValue(test_id):
    return int(sha1(test_id.encode('utf-8')).hexdigest(), 16)

You could then take the modulus of `_getHashValue(test.id())` to get your bucket.

Though my inner crypto nerd tells me otherwise, what you have is probably fine. But using a cryptographic-quality hash should lead to more evenly distributed test cases.

[Gavin Panella (allenap)]

Thanks for all your useful suggestions!

[Gavin Panella (allenap)]

> Argh. Launchpad ate my most important comment, on line ~140 where you
> do the hash. (Mental note: press little save tickbox-button next
> time.) Basically I think you should do something like:
>
> from hashlib import sha1
>
> def _getHashValue(test_id):
> return int(sha1(test_id.encode('utf-8')).hexdigest(), 16)
>
> You could then take the modulus of `_getHashValue(test.id())` to get
> your bucket.
>
> Though my inner crypto nerd tells me otherwise, what you have is
> probably fine. But using a cryptographic-quality hash should lead to
> more evenly distributed test cases.

This interested me, so I checked it out.

Using http://paste.ubuntu.com/23832921/, I printed out the bucket sizes
when between 2 and 10 buckets are desired, then calculated the mean
absolute deviation for each, and a mean of those figures for each
hashing method.

The results at http://paste.ubuntu.com/23832909/ indicate that:

- For the region, SHA1 is slightly fairer than the sum-of-code-points
  method, but that SHA256 is less fair.

- For the rack, sum-of-code-points is the most fair, and that SHA256 is
  fairer than SHA1.

That's certainly not what I would have expected.

For now I'm going to stick with sum-of-code-points because it's simple
and seems to be fair enough.

[MAAS Lander (maas-lander)]

The attempt to merge lp:~allenap/maas/nose-select-bucket-plugin into lp:maas failed. Below is the output from the failed tests.

Get:1 http://security.ubuntu.com/ubuntu xenial-security InRelease [102 kB]
Hit:2 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial InRelease
Get:3 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-updates InRelease [102 kB]
Hit:4 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-backports InRelease
Fetched 204 kB in 0s (481 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
    --no-install-recommends install apache2 archdetect-deb authbind avahi-utils bash bind9 bind9utils build-essential bzr bzr-builddeb chromium-browser chromium-chromedriver curl daemontools debhelper dh-apport dh-systemd distro-info dnsutils firefox freeipmi-tools git gjs ipython isc-dhcp-common isc-dhcp-server libjs-angularjs libjs-jquery libjs-jquery-hotkeys libjs-yui3-full libjs-yui3-min libnss-wrapper libpq-dev make nodejs-legacy npm postgresql pxelinux python3-all python3-apt python3-attr python3-bson python3-convoy python3-crochet python3-cssselect python3-curtin python3-dev python3-distro-info python3-django python3-django-nose python3-django-piston3 python3-dnspython python3-docutils python3-formencode python3-hivex python3-httplib2 python3-jinja2 python3-jsonschema python3-lxml python3-netaddr python3-netifaces python3-novaclient python3-oauth python3-oauthlib python3-openssl python3-paramiko python3-petname python3-pexpect python3-psycopg2 python3-pyinotify python3-pyparsing python3-pyvmomi python3-requests python3-seamicroclient python3-setuptools python3-simplestreams python3-sphinx python3-tempita python3-twisted python3-txtftp python3-tz python3-yaml python3-zope.interface python-bson python-crochet python-django python-django-piston python-djorm-ext-pgarray python-formencode python-lxml python-netaddr python-netifaces python-pocket-lint python-psycopg2 python-simplejson python-tempita python-twisted python-yaml socat syslinux-common tgt ubuntu-cloudimage-keyring wget xvfb
Reading package lists...
Building dependency tree...
Reading state information...
authbind is already the newest version (2.1.1+nmu1).
avahi-utils is already the newest version (0.6.32~rc+dfsg-1ubuntu2).
build-essential is already the newest version (12.1ubuntu2).
debhelper is already the newest version (9.20160115ubuntu3).
distro-info is already the newest version (0.14build1).
git is already the newest version (1:2.7.4-0ubuntu1).
libjs-angularjs is already the newest version (1.2.28-1ubuntu2).
libjs-jquery is already the newest version (1.11.3+dfsg-4).
libjs-yui3-full is already the newest version (3.5.1-1ubuntu3).
libjs-yui3-min is already the newest version (3.5.1-1ubuntu3).
make is already the newest version (4.1-6).
postgresql is already the newest version (9.5+173).
pxelinux is already the newest version (3:6.03+dfsg-11ubuntu1).
python-formencode is already the newest version (1.3.0-0ubuntu5).
python-lxml is already the newest version (3.5.0-1build1).
python-netaddr is already the newest version (0.7.18-1).
python-netifaces is already the newest version (0.10.4-0.1build2).
python-psycop...

=== modified file 'buildout.cfg'
--- buildout.cfg	2017-01-11 10:29:20 +0000
+++ buildout.cfg	2017-01-20 14:09:17 +0000
@@ -73,10 +73,8 @@
   # When running headless increase the verbosity so we can see the test
   # being run from a log file. An `options` list must be defined ahead
   # of the use of this snippet.
-  # *** NOSE PROGRESSIVE IS BROKEN IN PYTHON 3.5 ***
-  # ["--verbosity=0", "--with-progressive"]
   options += (
-    ["--verbosity=1"]
+    ["--verbosity=0", "--with-progressive"]
     if sys.stdout.isatty() else
     ["--verbosity=2"]
   )

=== modified file 'src/maasserver/djangosettings/development.py'
--- src/maasserver/djangosettings/development.py	2016-12-07 12:46:14 +0000
+++ src/maasserver/djangosettings/development.py	2017-01-20 14:09:17 +0000
@@ -65,11 +65,6 @@
 # Example: "/home/media/media.lawrence.com/media/"
 MEDIA_ROOT = abspath("media/development")
 
-INSTALLED_APPS += (
-    'maastesting',
-    'django_nose',
-)
-
 INTERNAL_IPS = ('127.0.0.1', '::1')
 
 # Make all nodes' metadata visible.  This is not safe; do not enable it
@@ -111,6 +106,8 @@
     "maastesting.noseplug.Resources",
     "maastesting.noseplug.Scenarios",
     "maastesting.noseplug.Select",
+    "maastesting.noseplug.SelectBucket",
+    "maastesting.noseplug.Subunit",
 ]
 
 # Fix crooked settings.

=== modified file 'src/maastesting/noseplug.py'
--- src/maastesting/noseplug.py	2016-12-14 09:12:04 +0000
+++ src/maastesting/noseplug.py	2017-01-20 14:09:17 +0000
@@ -1,4 +1,4 @@
-# Copyright 2015-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Nose plugins for MAAS."""
@@ -14,6 +14,8 @@
 import inspect
 import io
 import logging
+import optparse
+import sys
 import unittest
 
 from nose.case import Test
@@ -24,6 +26,21 @@
 from twisted.python.filepath import FilePath
 
 
+def flattenTests(tests):
+    """Recursively eliminate nested test suites.
+
+    A `TestSuite` can contain zero or more tests or other suites, or a mix of
+    both. This function undoes all of this nesting. Be sure you know what you
+    intend when doing this, because any behaviour that is endowed by those
+    nested test suites will be lost.
+    """
+    for test in tests:
+        if isinstance(test, unittest.TestSuite):
+            yield from flattenTests(test)
+        else:
+            yield test
+
+
 class Crochet(Plugin):
     """Start the Twisted reactor via Crochet."""
 
@@ -97,18 +114,10 @@
 
         :return: An instance of :class:`OptimisingTestSuite`.
         """
-        tests = self._flattenTests(test)
+        tests = flattenTests(test)
         tests = map(self._hoistResources, tests)
         return OptimisingTestSuite(tests)
 
-    def _flattenTests(self, tests):
-        """Recursively eliminate test suites."""
-        for test in tests:
-            if isinstance(test, unittest.TestSuite):
-                yield from self._flattenTests(test)
-            else:
-                yield test
-
     def _hoistResources(self, test):
         """Hoist resources from the real test to Nose's test wrapper."""
         if isinstance(test, Test):
@@ -262,6 +271,143 @@
         return inspect.getdoc(self)
 
 
+class SelectBucket(Plugin):
+    """Select tests from buckets derived from their names.
+
+    Each test's ID is hashed into a number. This number, modulo the given
+    number of "buckets", defines the "bucket" for the test. This bucket can
+    then be selected by an option. This gives a caller a rough but stable way
+    to split up a test suite into parts, for running in parallel perhaps.
+
+    Note that, when using this plug-in, the nose-progressive plug-in will be
+    inoperative. Both this and nose-progressive attempt to customise the test
+    runner, but this wins.
+    """
+
+    name = "select-bucket"
+    option_selected_bucket = "%s_selected_bucket" % name
+    log = logging.getLogger('nose.plugins.%s' % name)
+    score = 10001  # Run before nose-progressive.
+
+    def options(self, parser, env):
+        """Add options to Nose's parser.
+
+        :attention: This is part of the Nose plugin contract.
+        """
+        # This plugin is not compatible with django-nose. Nose passes in an
+        # optparse-based argument parser, which django-nose later strips,
+        # putting the options into an argparse-based parser. This breaks
+        # because argparse uses `type` and `action` instead of `callback` but
+        # django-nose does not provide a compatibility shim for that. We could
+        # probably work around that here, but django-nose is deprecated in
+        # MAAS so it's not worth the effort. Hence, when django_nose has been
+        # loaded, this plugin is simply disabled.
+        if "django_nose" in sys.modules:
+            return
+
+        super(SelectBucket, self).options(parser, env)
+        parser.add_option(
+            "--%s" % self.name, dest=self.option_selected_bucket,
+            action="callback", callback=self._ingestSelectedBucket,
+            type="str", metavar="BUCKET/BUCKETS", default=None, help=(
+                "Select the number of buckets in which to split tests, and "
+                "which of these buckets to then run, e.g. 8/13 will split "
+                "tests into 13 buckets and will run those in the 8th bucket "
+                "(well... the 7th; the bucket number is indexed from 1)."
+            ),
+        )
+
+    def configure(self, options, conf):
+        """Configure, based on the parsed options.
+
+        :attention: This is part of the Nose plugin contract.
+        """
+        super(SelectBucket, self).configure(options, conf)
+        if self.enabled:
+            bucket_buckets = getattr(options, self.option_selected_bucket)
+            if bucket_buckets is None:
+                self._selectTest = None
+            else:
+                bucket, buckets = bucket_buckets
+                offset = bucket - 1  # Zero-based bucket number.
+                self._selectTest = lambda test: (
+                    sum(map(ord, test.id())) % buckets == offset)
+
+    def _ingestSelectedBucket(self, option, option_string, value, parser):
+        """Callback for the `--select-bucket` option.
+
+        The recognises values that match \d+/\d+, i.e. two integers. The
+        latter is the total number of buckets, the first is which of those
+        buckets to select (starting at 1).
+        """
+        try:
+            value = self._parseSelectedBucket(value)
+        except ValueError as error:
+            raise optparse.OptionValueError(
+                "%s: %s" % (option_string, error))
+        else:
+            setattr(parser.values, option.dest, value)
+
+    def _parseSelectedBucket(self, option):
+        """Helper for `_ingestSelectedBucket`."""
+        if option is None:
+            return None
+        elif "/" in option:
+            bucket, buckets = option.split("/", 1)
+            try:
+                bucket = int(bucket)
+                buckets = int(buckets)
+            except ValueError:
+                raise ValueError("not bucket/buckets")
+            else:
+                if buckets <= 0:
+                    raise ValueError("buckets must be >= 0")
+                elif bucket <= 0:
+                    raise ValueError("bucket must be >= 0")
+                elif bucket > buckets:
+                    raise ValueError("bucket must be <= buckets")
+                else:
+                    return bucket, buckets
+        else:
+            raise ValueError("not bucket/buckets")
+
+    def prepareTestRunner(self, runner):
+        """Convert `runner` to a selective variant.
+
+        :attention: This is part of the Nose plugin contract.
+        """
+        if self._selectTest is not None:
+            return SelectiveTestRunner(runner, self._selectTest)
+
+    def help(self):
+        """Used in the --help text.
+
+        :attention: This is part of the Nose plugin contract.
+        """
+        return inspect.getdoc(self)
+
+
+class SelectiveTestRunner:
+    """Wrap a test runner in order to filter the test suite to run."""
+
+    def __init__(self, runner, select):
+        super(SelectiveTestRunner, self).__init__()
+        self._runner = runner
+        self._select = select
+
+    def run(self, test):
+        if isinstance(test, unittest.TestSuite):
+            tests = flattenTests(test)
+            tests = filter(self._select, tests)
+            test = type(test)(tests)
+        else:
+            raise TypeError(
+                "Expected test suite, got %s: %r" % (
+                    type(test).__class__.__name__, test))
+
+        return self._runner.run(test)
+
+
 class Subunit(Plugin):
     """Emit test results as a subunit stream."""
 
@@ -311,10 +457,9 @@
 def main():
     """Invoke Nose's `TestProgram` with extra plugins.
 
-    Specifically the `Crochet`, `Resources`, `Scenarios`, and `Select`
-    plugins. At the command-line it's still necessary to enable these with the
-    flags ``--with-crochet``, ``--with-resources``, ``--with-scenarios``,
-    and/or ``--with-select``.
+    At the command-line it's still necessary to enable these with the flags
+    ``--with-crochet``, ``--with-resources``, ``--with-scenarios``, and so on.
     """
-    plugins = Crochet(), Resources(), Scenarios(), Select(), Subunit()
-    return TestProgram(addplugins=plugins)
+    return TestProgram(addplugins=(
+        Crochet(), Resources(), Scenarios(), Select(), SelectBucket(),
+        Subunit()))

=== modified file 'src/maastesting/tests/test_noseplug.py'
--- src/maastesting/tests/test_noseplug.py	2016-12-14 09:12:04 +0000
+++ src/maastesting/tests/test_noseplug.py	2017-01-20 14:09:17 +0000
@@ -25,6 +25,7 @@
 from maastesting import noseplug
 from maastesting.factory import factory
 from maastesting.matchers import (
+    IsCallable,
     MockCalledOnceWith,
     MockNotCalled,
 )
@@ -33,6 +34,7 @@
     Resources,
     Scenarios,
     Select,
+    SelectBucket,
     Subunit,
 )
 from maastesting.testcase import MAASTestCase
@@ -362,6 +364,85 @@
             join(directory, factory.make_name("other-child"))))
 
 
+class TestSelectBucket(MAASTestCase):
+
+    def test__options_adds_options(self):
+        select = SelectBucket()
+        parser = OptionParser()
+        select.options(parser=parser, env={})
+        self.assertThat(
+            parser.option_list[-2:],
+            MatchesListwise([
+                # The --with-select-bucket option.
+                MatchesStructure.byEquality(
+                    action="store_true", default=None,
+                    dest="enable_plugin_select_bucket",
+                ),
+                # The --select-bucket option.
+                MatchesStructure.byEquality(
+                    action="callback", default=None,
+                    dest="select-bucket_selected_bucket",
+                    metavar="BUCKET/BUCKETS", type="string",
+                    _short_opts=[], _long_opts=["--select-bucket"],
+                )
+            ]))
+
+    def test__configure_parses_selected_bucket(self):
+        select = SelectBucket()
+        parser = OptionParser()
+        select.add_options(parser=parser, env={})
+        options, rest = parser.parse_args(
+            ["--with-select-bucket", "--select-bucket", "8/13"])
+        select.configure(options, sentinel.conf)
+        self.assertThat(select, MatchesStructure(_selectTest=IsCallable()))
+
+    @staticmethod
+    def _make_test_with_id(test_id):
+        test = unittest.TestCase()
+        test.id = lambda: test_id
+        return test
+
+    def test__prepareTestRunner_wraps_given_runner_and_filters_tests(self):
+        select = SelectBucket()
+        parser = OptionParser()
+        select.add_options(parser=parser, env={})
+        options, rest = parser.parse_args(
+            ["--with-select-bucket", "--select-bucket", "8/13"])
+        select.configure(options, sentinel.conf)
+
+        # We start at 65 because chr(65) is "A" and so makes a nice readable
+        # ID for the test. We end at 77 because chr(77) is "M", a readable ID
+        # once again, but more importantly it means we'll have 13 tests, which
+        # is the modulus we started with.
+        tests = map(self._make_test_with_id, map(chr, range(65, 78)))
+        test = unittest.TestSuite(tests)
+        self.assertThat(test.countTestCases(), Equals(13))
+
+        class MockTestRunner:
+            def run(self, test):
+                self.test = test
+
+        runner = runner_orig = MockTestRunner()
+        runner = select.prepareTestRunner(runner)
+        self.assertThat(runner, IsInstance(noseplug.SelectiveTestRunner))
+
+        runner.run(test)
+
+        self.assertThat(runner_orig.test, IsInstance(type(test)))
+        self.assertThat(runner_orig.test.countTestCases(), Equals(1))
+        # Note how the test with ID of "H" is the only one selected.
+        self.assertThat({t.id() for t in runner_orig.test}, Equals({"H"}))
+        self.assertThat(ord("H") % 13, Equals(7))
+
+    def test__prepareTestRunner_does_nothing_when_no_bucket_selected(self):
+        select = SelectBucket()
+        parser = OptionParser()
+        select.add_options(parser=parser, env={})
+        options, rest = parser.parse_args(["--with-select-bucket"])
+        select.configure(options, sentinel.conf)
+        self.assertThat(select.prepareTestRunner(sentinel.runner), Is(None))
+
+
 class TestSubunit(MAASTestCase):
 
     def test__options_adds_options(self):
@@ -411,10 +492,10 @@
         noseplug.main()
         self.assertThat(
             noseplug.TestProgram,
-            MockCalledOnceWith(addplugins=(ANY, ANY, ANY, ANY, ANY)))
+            MockCalledOnceWith(addplugins=(ANY, ANY, ANY, ANY, ANY, ANY)))
         plugins = noseplug.TestProgram.call_args[1]["addplugins"]
         self.assertThat(plugins, MatchesSetwise(
             IsInstance(Crochet), IsInstance(Resources),
             IsInstance(Scenarios), IsInstance(Select),
-            IsInstance(Subunit),
+            IsInstance(SelectBucket), IsInstance(Subunit),
         ))