Code review comment for lp:~lifeless/launchpad/paralleltests

Direct mail from jml:
"""Wow. Thanks for this Rob. While I lament the fact that we have to do hacks
to bin/test, I really like that you're starting on this, and that you've kept
the hack very clean.

Sending this to you directly since you didn't create an MP, and I
assume that you didn't for a reason :)

Lots of this stuff should just land now, regardless of whether the parallel
support is any good. I'm thinking particularly of all the boilerplate you
deleted.

Most of my comments below are asking for more or clearer documentation. I've
tried to supply docs where I know what's going on.

Thanks,
jml

> === modified file 'lib/canonical/launchpad/testing/tests/test_pages.py'
> --- lib/canonical/launchpad/testing/tests/test_pages.py 2010-10-04 19:50:45 +0000
> +++ lib/canonical/launchpad/testing/tests/test_pages.py 2010-10-15 11:52:20 +0000
> @@ -9,6 +9,8 @@ import shutil
> import tempfile
> import unittest
>
> +from bzrlib.tests import iter_suite_tests
> +

You can get this from testtools under a different name. Probably you should.

> === added file 'lib/canonical/testing/parallel.py'
> --- lib/canonical/testing/parallel.py 1970-01-01 00:00:00 +0000
> +++ lib/canonical/testing/parallel.py 2010-10-15 11:52:20 +0000
> @@ -0,0 +1,168 @@
> +# Copyright 2010 Canonical Ltd. This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +from __future__ import with_statement
> +
> +"""Parallel test glue."""
> +
> +__metaclass__ = type
> +__all__ = ["main"]
> +
> +import itertools
> +import os
> +import subprocess
> +import sys
> +import tempfile
> +
> +from bzrlib.osutils import local_concurrency
> +from subunit import ProtocolTestCase
> +from subunit.run import SubunitTestRunner
> +from testtools.compat import unicode_output_stream
> +from testtools import (
> + ConcurrentTestSuite,
> + TestResult,
> + TextTestResult,
> + )
> +
> +
> +def prepare_argv(argv):
> + """Remove options from argv that ListTestCase will add in itself."""

This might seem pedantic, but I think the comment reads more clearly like
this:

 """Remove options from argv that would be added by ListTestCase."""

> +def find_load_list(args):

Add a docstring like:

 """Get the value passed in to --load-list=FOO."""

> +def find_tests(argv):
> + """Find tests to parallel run.
> +
> + :param argv: The argv given to the test runner, used to get the tests to
> + run.

Add:

 :return: A list of test IDs.

> + """
> + load_list = find_load_list(argv)
> + if load_list:
> + # just use the load_list
> + with open(load_list, 'rt') as list_file:
> + return [id for id in list_file.read().split('\n') if id]

I think you could also write that as:

 return filter(None, list_file.read().splitlines())

But better yet might be to do this:

 return [id for id in (line.strip() for line in list_file) if id]

or:

 return filter(None, (line.strip() for line in list_file))

But whatever. :)

> + # run in --list-tests mode
> + argv = prepare_argv(argv) + ['--list-tests', '--subunit']
> + process = subprocess.Popen(argv, stdin=subprocess.PIPE,
> + stdout=subprocess.PIPE)
> + process.stdin.close()
> + test = ProtocolTestCase(process.stdout)
> + result = GatherIDs()
> + test.run(result)
> + process.wait()
> + if process.returncode:
> + raise Exception('error listing tests: %s' % err)
> + return result.ids
> +
> +
> +class ListTestCase(ProtocolTestCase):
> +
> + def __init__(self, test_ids, args):
> + """Create a ListTestCase.
> +
> + :param test_ids: The ids of the tests to run.
> + :param args; The args to use to run the test runner (without

:param args: (colon), not :param args; (semi-colon)

> + --load-list - that is added automatically).
> + """
> + self._test_ids = test_ids
> + self._args = args
> +
> + def run(self, result):
> + with tempfile.NamedTemporaryFile() as test_list_file:
> + for test_id in self._test_ids:
> + test_list_file.write(test_id + '\n')
> + test_list_file.flush()
> + argv = self._args + ['--subunit', '--load-list', test_list_file.name]
> + process = subprocess.Popen(argv, stdin=subprocess.PIPE,
> + stdout=subprocess.PIPE, bufsize=1)
> + try:
> + # If it tries to read, give it EOF.
> + process.stdin.close()
> + ProtocolTestCase.__init__(self, process.stdout)
> + ProtocolTestCase.run(self, result)
> + finally:
> + process.wait()
> +

This makes me think that I'd really just like to be able to dump test ids via
stdin, rather than having to make a file first. Not for this branch though.

> +
> +def concurrency():
> + """Return the optimal testing concurrency for this machine."""
> + # TODO: limit by memory as well.
> + procs = local_concurrency()
> + return procs
> +

It's not clear what unit the return value of "optimal testing concurrency" is
measured in. I'm guessing it's the optimal number of parallel test "threads".

> +def partition_tests(test_ids, count):
> + """Partition suite into count lists of tests."""
> + # This just assigns tests in a round-robin fashion. On one hand this
> + # splits up blocks of related tests that might run faster if they shared
> + # resources, but on the other it avoids assigning blocks of slow tests to
> + # just one partition. So the slowest partition shouldn't be much slower
> + # than the fastest.
> + partitions = [list() for i in range(count)]
> + for partition, test_id in itertools.izip(itertools.cycle(partitions), test_ids):
> + partition.append(test_id)
> + return partitions
> +

You could probably write this in such a way that you don't have to build up a
list first, but meh.

> === added file 'lib/canonical/testing/tests/test_parallel.py'
> --- lib/canonical/testing/tests/test_parallel.py 1970-01-01 00:00:00 +0000
> +++ lib/canonical/testing/tests/test_parallel.py 2010-10-15 11:52:20 +0000
> @@ -0,0 +1,117 @@
> +# Copyright 2010 Canonical Ltd. This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +from __future__ import with_statement
> +
> +"""Parallel test glue."""
> +
> +__metaclass__ = type
> +
> +from StringIO import StringIO
> +import subprocess
> +import tempfile
> +
> +from testtools import (
> + TestCase,
> + TestResult,
> + )
> +from fixtures import (
> + PopenFixture,
> + TestWithFixtures,
> + )
> +
> +from canonical.testing.parallel import (
> + find_load_list,
> + find_tests,
> + ListTestCase,
> + main,
> + prepare_argv,
> + )
> +
> +
> +class TestMain(TestCase):
> +
> + def test_main_uses_load_list_partitioned(self):
> + pass
> +

This is not a test. :)

> +
> +class TestListTestCase(TestCase, TestWithFixtures):
> +
> + def test_run(self):
> + def check_list_file(info):
> + # A temp file should have been made.
> + args = info['args']
> + load_list = find_load_list(args)
> + self.assertNotEqual(None, load_list)
> + with open(load_list, 'rt') as testlist:
> + contents = testlist.readlines()
> + self.assertEqual(['foo\n', 'bar\n'], contents)
> + return {'stdout': StringIO(''), 'stdin': StringIO()}
> + popen = self.useFixture(PopenFixture(check_list_file))
> + case = ListTestCase(['foo', 'bar'], ['bin/test'])
> + self.assertEqual([], popen.procs)
> + result = TestResult()
> + case.run(result)
> + self.assertEqual(0, result.testsRun)
> + self.assertEqual(1, len(popen.procs))
> + self.assertEqual(['bin/test', '--subunit', '--load-list'],
> + popen.procs[0]._args['args'][:-1])
> +

I find it a bit hard to follow this test, mostly because I don't know what
PopenFixture is doing or what its callback is for. What exactly is the test
checking?

Could you please add some comments?

> +
> +class TestUtilities(TestCase, TestWithFixtures):
> +
...
> + def test_find_tests_live(self):
> + def inject_testlist(args):
> + self.assertEqual(subprocess.PIPE, args['stdin'])
> + self.assertEqual(subprocess.PIPE, args['stdout'])
> + self.assertEqual(
> + ['bin/test', '-vt', 'filter', '--list-tests', '--subunit'],
> + args['args'])
> + return {'stdin': StringIO(), 'stdout': StringIO(u"""
> +test: quux
> +successful: quux
> +test: glom
> +successful: glom
> +""")}
> + popen = self.useFixture(PopenFixture(inject_testlist))
> + self.assertEqual(
> + ['quux', 'glom'],
> + find_tests(['bin/test', '-vt', 'filter', '--parallel']))
>

Ditto for this test.
"""