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

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.

[...]
> === 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.

> +
> + 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.)

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