Code review comment for lp:~allenap/launchpad/isolate-tests

On Tue, Mar 16, 2010 at 4:58 PM, Gavin Panella
<email address hidden> wrote:
>> On Mon, Mar 15, 2010 at 1:30 PM, Gavin Panella
>> <email address hidden> wrote:
>> > Latest incremental diff, r10440..
>> >
>> > === modified file 'lib/lp/services/job/runner.py'
>> > --- lib/lp/services/job/runner.py     2010-02-25 12:07:15 +0000
>> > +++ lib/lp/services/job/runner.py     2010-03-15 13:26:40 +0000
>> > @@ -401,9 +401,10 @@
>> >  class JobCronScript(LaunchpadCronScript):
>> >      """Base class for scripts that run jobs."""
>> >
>> > -    def __init__(self, runner_class=JobRunner):
>> > +    def __init__(self, runner_class=JobRunner, test_args=None):
>> >          self.dbuser = getattr(config, self.config_name).dbuser
>> > -        super(JobCronScript, self).__init__(self.config_name, self.dbuser)
>> > +        super(JobCronScript, self).__init__(
>> > +            self.config_name, self.dbuser, test_args)
>> >          self.runner_class = runner_class
>> >
>> >      def main(self):
>> >
>>
>> What's the motivation for this change?
>
> It makes testing easier. I'll explain more below.
>

>>
>> > === modified file 'lib/lp/services/job/tests/test_runner.py'
>> > --- lib/lp/services/job/tests/test_runner.py  2010-02-24 19:52:01 +0000
>> > +++ lib/lp/services/job/tests/test_runner.py  2010-03-15 13:26:40 +0000
>> > @@ -11,23 +11,24 @@
>> >  from unittest import TestLoader
>> >
>> >  import transaction
>> > -from canonical.testing import LaunchpadZopelessLayer
>> > +
>> >  from zope.component import getUtility
>> >  from zope.error.interfaces import IErrorReportingUtility
>> >  from zope.interface import implements
>> >
>> > +from canonical.launchpad.webapp import errorlog
>> > +from canonical.launchpad.webapp.interfaces import (
>> > +    DEFAULT_FLAVOR, IStoreSelector, MAIN_STORE)
>> > +from canonical.testing import LaunchpadZopelessLayer
>> > +
>> >  from lp.code.interfaces.branchmergeproposal import (
>> > -    IUpdatePreviewDiffJobSource,)
>> > -from lp.testing.mail_helpers import pop_notifications
>> > -from lp.services.job.runner import (
>> > -    JobCronScript, JobRunner, BaseRunnableJob, TwistedJobRunner
>> > -)
>> > +    IUpdatePreviewDiffJobSource)
>> >  from lp.services.job.interfaces.job import JobStatus, IRunnableJob
>> >  from lp.services.job.model.job import Job
>> > -from lp.testing import TestCaseWithFactory
>> > -from canonical.launchpad.webapp import errorlog
>> > -from canonical.launchpad.webapp.interfaces import (
>> > -    IStoreSelector, MAIN_STORE, DEFAULT_FLAVOR)
>> > +from lp.services.job.runner import (
>> > +    BaseRunnableJob, JobCronScript, JobRunner, TwistedJobRunner)
>> > +from lp.testing import TestCaseWithFactory, ZopeTestInSubProcess
>> > +from lp.testing.mail_helpers import pop_notifications
>> >
>> >
>> >  class NullJob(BaseRunnableJob):
>> > @@ -300,7 +301,7 @@
>> >          self.entries.append(input)
>> >
>> >
>> > -class TestTwistedJobRunner(TestCaseWithFactory):
>> > +class TestTwistedJobRunner(ZopeTestInSubProcess, TestCaseWithFactory):
>> >
>> >      layer = LaunchpadZopelessLayer
>> >
>> > @@ -325,7 +326,7 @@
>> >          self.assertIn('Job ran too long.', oops.value)
>> >
>> >
>> > -class TestJobCronScript(TestCaseWithFactory):
>> > +class TestJobCronScript(ZopeTestInSubProcess, TestCaseWithFactory):
>> >
>> >      layer = LaunchpadZopelessLayer
>> >
>> > @@ -351,7 +352,8 @@
>> >              source_interface = IUpdatePreviewDiffJobSource
>> >
>> >              def __init__(self):
>> > -                super(JobCronScriptSubclass, self).__init__(DummyRunner)
>> > +                super(JobCronScriptSubclass, self).__init__(
>> > +                    DummyRunner, test_args=[])
>>
>> How's this related to the Zope isolation change?
>
> It made this code easier to test. Without it, I can only test it (afaik)
> like so:
>
>  bin/test test_runner
>
> I cannot, for example, do:
>
>  bin/test -t 'test_(runner|checkwatches)|externalbugtracker.txt'
>
> because sys.argv contains stuff that LaunchpadScript tries to digest. I
> have to override that, and that's what test_args does.
>

OK, makes sense.

>
>> >                  self.logger = ListLogger()
>> >
>> >          old_errorlog = errorlog.globalErrorUtility
>> >
>> > === modified file 'lib/lp/testing/__init__.py'
>> > --- lib/lp/testing/__init__.py        2010-03-01 03:06:02 +0000
>> > +++ lib/lp/testing/__init__.py        2010-03-15 13:26:40 +0000
>> > @@ -29,6 +29,7 @@
>> >      'validate_mock_class',
>> >      'WindmillTestCase',
>> >      'with_anonymous_login',
>> > +    'ZopeTestInSubProcess',
>> >      ]
>> >
>> >  import copy
>> > @@ -38,6 +39,8 @@
>> >  from pprint import pformat
>> >  import shutil
>> >  import subprocess
>> > +import subunit
>> > +import sys
>> >  import tempfile
>> >  import time
>> >
>> > @@ -62,6 +65,7 @@
>> >  from zope.interface.verify import verifyClass, verifyObject
>> >  from zope.security.proxy import (
>> >      isinstance as zope_isinstance, removeSecurityProxy)
>> > +from zope.testing.testrunner.runner import TestResult as ZopeTestResult
>> >
>> >  from canonical.launchpad.webapp import errorlog
>> >  from canonical.config import config
>> > @@ -586,6 +590,57 @@
>> >          self.client.open(url=u'http://launchpad.dev:8085')
>> >
>> >
>> > +class ZopeTestInSubProcess:
>> > +    """Run tests in a sub-process, respecting Zope idiosyncrasies.
>> > +
>> > +    Use this as a mixin with an interesting `TestCase` to isolate
>> > +    tests with side-effects. Each and every test *method* in the test
>> > +    case is run in a new, forked, sub-process. This will slow down
>> > +    your tests, so use it sparingly. However, when you need to, for
>> > +    example, start the Twisted reactor (which cannot currently be
>> > +    safely stopped and restarted in process) it is invaluable.
>> > +
>>
>> For the record, vast swathes of code have been written for Twisted that don't
>> require testing starting the reactor. Generally, starting and stopping the
>> reactor in application code is a bad pattern.
>
> In the case of the job runner, it's because Twisted is being shoe-horned
> into an API that was designed before Twisted was introduced. In
> checkwatches, it's like that because it's a transitionary API, almost a
> learning point. Also, our scripts are expected to inherit from
> LaunchpadScript (or, at least, that's the legacy we're working with),
> not something suitable for twistd.
>
> Is it such a bad pattern to start and stop the reactor? Other than it
> doesn't work reliably. If it did work perfectly, what would be the
> problem?
>

I don't know. I'll have to think about it.

>>
>> > +    This is basically a reimplementation of subunit's
>> > +    `IsolatedTestCase` or `IsolatedTestSuite`, but adjusted to work
>> > +    with Zope. In particular, Zope's TestResult object is responsible
>> > +    for calling testSetUp() and testTearDown() on the selected layer.
>> > +    """
>> > +
>> > +    def run(self, result):
>> > +        assert isinstance(result, ZopeTestResult), (
>> > +            "result must be a Zope result object, not %r." % (result,))
>>
>> Why? Please add a comment explaining the case.
>
> It's because it does super(ZopeTestResult, result) later on. I could
> change it to not care, then later do:
>
>    if isinstance(result, ZopeTestResult):
>        # Skip all the Zope-specific result stuff
>        # by passing a super() of the result.
>        result = super(ZopeTestResult, result)
>
> But, for non-Zope test results, this could cause confusion (duplicated
> stdout/stderr reporting, for example).
>
> For example, the Zope test result object is responsible for calling
> testSetUp() and testTearDown(), and for handling post-mortem
> debugging. If another test result object did something similar, it would
> make no sense to do it in the parent process.
>

Fair enough. Please add a comment to the isinstance check pointing to
the super() call below.

>>
>> > +        pread, pwrite = os.pipe()
>> > +        pid = os.fork()
>> > +        if pid == 0:
>> > +            # Child.
>> > +            os.close(pread)
>> > +            fdwrite = os.fdopen(pwrite, 'w', 1)
>> > +            # Send results to both the Zope result object (so that
>> > +            # layer setup and teardown are done properly, etc.) and to
>> > +            # the subunit stream client so that the parent process can
>> > +            # obtain the result.
>> > +            result = testtools.MultiTestResult(
>> > +                result, subunit.TestProtocolClient(fdwrite))
>> > +            super(ZopeTestInSubProcess, self).run(result)
>> > +            fdwrite.flush()
>> > +            sys.stdout.flush()
>> > +            sys.stderr.flush()
>> > +            # Exit hard.
>> > +            os._exit(0)
>>
>> Personally, I don't need a comment to explain what os._exit(0) does. It's more
>> interesting to know why we want to exit hard.
>
> I copied this from subunit. I guess it's to avoid running onexit
> handlers, but perhaps there are other reasons. Ah, maybe SystemExit is
> caught and just logged as a test failure.
>

Thanks.

>>
>> > +        else:
>> > +            # Parent.
>> > +            os.close(pwrite)
>> > +            fdread = os.fdopen(pread, 'rU')
>> > +            # Accept the result from the child process. Skip all the
>> > +            # Zope-specific result stuff by passing a super() of the
>> > +            # result.
>>
>> This comment would be more helpful if it explained why we want to skip the
>> zope-specific result stuff.
>
> Done.
>

Thanks.

>>
>> > +            result = super(ZopeTestResult, result)
>> > +            protocol = subunit.TestProtocolServer(result)
>> > +            protocol.readFrom(fdread)
>> > +            fdread.close()
>> > +            os.waitpid(pid, 0)
>>
>> Thanks for persevering. Not long to go now :)
>
> Thanks for the review :)
>

No probs.