Code review comment for lp:~michael.nelson/launchpad/450124-findBuildCandidate_improvements

Gavin Panella wrote:
> Review: Approve
> Hi Michael,
>
> I have a few comments about the tests which you can take or
> leave. Other than that, all good as far as I can tell :)
>
> Gavin.

Thanks Gavin.

>
>
> [...]
>> === added file 'lib/lp/soyuz/tests/test_builder.py'
>> --- lib/lp/soyuz/tests/test_builder.py 1970-01-01 00:00:00 +0000
>> +++ lib/lp/soyuz/tests/test_builder.py 2009-10-14 10:55:53 +0000
>> @@ -0,0 +1,116 @@
>> +# Copyright 2009 Canonical Ltd. This software is licensed under the
>> +# GNU Affero General Public License version 3 (see the file LICENSE).
>> +
>> +"""Test Builder features."""
>> +
>> +import unittest
>> +
>> +from zope.component import getUtility
>> +
>> +from canonical.testing import LaunchpadZopelessLayer
>> +from lp.soyuz.interfaces.archive import ArchivePurpose
>> +from lp.soyuz.interfaces.builder import IBuilderSet
>> +from lp.soyuz.interfaces.build import BuildStatus
>> +from lp.soyuz.interfaces.publishing import PackagePublishingStatus
>> +from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
>> +from lp.testing import TestCaseWithFactory
>> +
>> +
>> +class TestFindBuildCandidate(TestCaseWithFactory):
>> +
>> + layer = LaunchpadZopelessLayer
>> +
>> + def setUp(self):
>> + """Publish some builds for the test archive."""
>> + super(TestFindBuildCandidate, self).setUp()
>> + self.publisher = SoyuzTestPublisher()
>> + self.publisher.prepareBreezyAutotest()
>> +
>> + # Create two PPAs and add some builds to each.
>> + self.ppa_joe = self.factory.makeArchive(name="joesppa")
>> + self.ppa_jim = self.factory.makeArchive(name="jimsppa")
>> +
>> + self.publisher.getPubSource(
>> + sourcename="gedit", status=PackagePublishingStatus.PUBLISHED,
>> + archive=self.ppa_joe).createMissingBuilds()
>> + self.publisher.getPubSource(
>> + sourcename="firefox", status=PackagePublishingStatus.PUBLISHED,
>> + archive=self.ppa_joe).createMissingBuilds()
>> +
>> + self.publisher.getPubSource(
>> + sourcename="gedit", status=PackagePublishingStatus.PUBLISHED,
>> + archive=self.ppa_jim).createMissingBuilds()
>> + self.publisher.getPubSource(
>> + sourcename="firefox", status=PackagePublishingStatus.PUBLISHED,
>> + archive=self.ppa_jim).createMissingBuilds()
>> +
>> + # Create two i386 builders ready to build PPA builds.
>> + builder_set = getUtility(IBuilderSet)
>> + self.builder1 = self.factory.makeBuilder(name='bob2')
>> + self.builder2 = self.factory.makeBuilder(name='frog2')
>> +
>> + # Grab the first build, ensure that it is what we expect
>> + # (ie. the first build from joesppa) and set it building.
>> + self.first_job = self.builder1.findBuildCandidate()
>> + self.failUnlessEqual('joesppa', self.first_job.build.archive.name)
>> + self.failUnlessEqual(
>> + u'i386 build of gedit 666 in ubuntutest breezy-autotest RELEASE',
>> + self.first_job.build.title)
>> + self.first_job.build.buildstate = BuildStatus.BUILDING
>> + self.first_job.build.builder = self.builder1
>
> That looks like a lot of set-up. If there's anything there that's
> specific to only one test_* method, consider moving it out.

It's used by both the PPA tests, but isn't necessary for the distro
archive test as you pointed out below...

>
>> +
>> + def test_findBuildCandidate_first_build_started(self):
>> + # Once a build for an ppa+arch has started, a second one for the
>> + # same ppa+arch will not be a candidate.
>> + next_job = self.builder2.findBuildCandidate()
>> + self.failIfEqual('joesppa', next_job.build.archive.name)
>> +
>> + def test_findBuildCandidate_first_build_finished(self):
>> + # When joe's first ppa build finishes, his second i386 build
>> + # will be the next build candidate.
>> + self.first_job.build.buildstate = BuildStatus.FAILEDTOBUILD
>> + next_job = self.builder2.findBuildCandidate()
>> + self.failUnlessEqual('joesppa', next_job.build.archive.name)
>> +
>> + def test_findBuildCandidate_for_private_ppa(self):
>> + # If a ppa is private it will be able to have parallel builds
>> + # for the one architecture.
>> + self.ppa_joe.private = True
>> + self.ppa_joe.buildd_secret = 'sekrit'
>> + next_job = self.builder2.findBuildCandidate()
>> + self.failUnlessEqual('joesppa', next_job.build.archive.name)
>> +
>> + def test_findBuildCandidate_for_non_ppa(self):
>> + # Normal archives are not restricted to serial builds per
>> + # arch.
>> + non_ppa = self.factory.makeArchive(
>> + name="primary", purpose=ArchivePurpose.PRIMARY)
>> +
>> + gedit_build = self.publisher.getPubSource(
>> + sourcename="gedit", status=PackagePublishingStatus.PUBLISHED,
>> + archive=non_ppa).createMissingBuilds()[0]
>> + firefox_build = self.publisher.getPubSource(
>> + sourcename="firefox", status=PackagePublishingStatus.PUBLISHED,
>> + archive=non_ppa).createMissingBuilds()[0]
>> +
>> + # Rescore our primary builds so that they'll be returned before
>> + # the PPA ones.
>> + gedit_build.buildqueue_record.manualScore(3000)
>> + firefox_build.buildqueue_record.manualScore(3000)
>> +
>> + next_job = self.builder2.findBuildCandidate()
>> + self.failUnlessEqual('primary', next_job.build.archive.name)
>> + self.failUnlessEqual(
>> + 'gedit', next_job.build.sourcepackagerelease.name)
>> +
>> + # Now even if we set the build building, we'll still get the
>> + # second non-ppa build for the same archive as the next candidate.
>> + next_job.build.buildstate = BuildStatus.BUILDING
>> + next_job.build.builder = self.builder2
>> + next_job = self.builder2.findBuildCandidate()
>> + self.failUnlessEqual('primary', next_job.build.archive.name)
>> + self.failUnlessEqual(
>> + 'firefox', next_job.build.sourcepackagerelease.name)
>
> Ah, a lot of that looks similar to what's in setUp(). Maybe split
> these out into two TestCases, one for PPAs, one for other archives? Or
> is the rescoring stuff relevant? (Sorry for my continued Soyuz
> ignorance.)

Good point - done. Incremental attached. Thanks!

>
>> +
>> +def test_suite():
>> + return unittest.TestLoader().loadTestsFromName(__name__)
>>
> [...]
>

--
Michael