Code review comment for lp:~al-maisan/launchpad/spec-job-class-503839

Michael Nelson wrote:
[..]

Hello Michael,

thanks again for taking the time to review this branch despite your own
busy schedule. That's very much appreciated :)

> Some comments in line below.
Please see my replies as well as the enclosed incremental diff.

>> === modified file 'lib/lp/soyuz/configure.zcml'
>> --- lib/lp/soyuz/configure.zcml 2010-01-04 21:15:46 +0000
>> +++ lib/lp/soyuz/configure.zcml 2010-01-07 08:23:28 +0000
>> @@ -897,6 +897,16 @@
>> <allow
>> interface="lp.soyuz.interfaces.buildpackagejob.IBuildPackageJob"/>
>> </class>
>> + <!--
>> + The registration below is used to discover all build farm job classes
>> + that implement the `IBuildFarmJob` interface. Please see bug #503839
>> + for more detail.
>> + The 'name' attribute needs to be set to the appropriate
>> + `BuildFarmJobType` enumeration value.
>> + -->
>> + <utility component="lp.soyuz.model.buildpackagejob.BuildPackageJob"
>> + name="PACKAGEBUILD"
>> + provides="lp.buildmaster.interfaces.buildfarmjob.IBuildFarmJob"/>
>>
>> <!-- BinaryPackageBuildBehavior -->
>> <adapter
>>
>> === modified file 'lib/lp/soyuz/interfaces/buildqueue.py'
>> --- lib/lp/soyuz/interfaces/buildqueue.py 2009-11-27 13:33:55 +0000
>> +++ lib/lp/soyuz/interfaces/buildqueue.py 2010-01-07 08:23:28 +0000
>> @@ -55,6 +55,10 @@
>> title=_('The builder behavior required to run this job.'),
>> required=False, readonly=True)
>>
>> + specific_job_classes = Field(
>> + title=_('Job classes that may run on the build farm.'),
>> + required=True, readonly=True)
>> +
>> estimated_duration = Timedelta(
>> title=_("Estimated Job Duration"), required=True,
>> description=_("Estimated job duration interval."))
>>
>> === modified file 'lib/lp/soyuz/model/buildqueue.py'
>> --- lib/lp/soyuz/model/buildqueue.py 2010-01-04 14:30:52 +0000
>> +++ lib/lp/soyuz/model/buildqueue.py 2010-01-07 08:23:28 +0000
>> @@ -57,11 +57,31 @@
>> return IBuildFarmJobBehavior(self.specific_job)
>>
>> @property
>> + def specific_job_classes(self):
>> + """See `IBuildQueue`."""
>> + from zope.component import getSiteManager
>> + from lp.buildmaster.interfaces.buildfarmjob import IBuildFarmJob
>
> Is there a reason for these imports to be inline? If so, please add a
> comment or otherwise move them as per the guidelines.
No other code in the module uses them. I have added a comment to that
effect.

>> +
>> + job_classes = []
>> + components = getSiteManager()
>> + # Get all components that implement the `IBuildFarmJob` interface.
>> + implementations = sorted(components.getUtilitiesFor(IBuildFarmJob))
>> + # The above yields a collection of 2-tuples where the first element
>> + # is the name of the `BuildFarmJobType` enum and the second element
>> + # is the implementing class respectively.
>> + for job_enum_name, job_class in implementations:
>> + job_enum = getattr(BuildFarmJobType, job_enum_name)
>
> I was originally going to say that we should be checking first with
> safe_hasattr here, but then the logical thing to do would be to throw
> an informative exception if it wasn't there (due to a mis-configuration)
> and that's exactly what getattr will do :)
Yup.

>> + job_classes.append((job_enum, job_class))
>
> Does job_classes need to be a list? Couldn't it be defined as an empty
> dict above and then here simply do job_classes[job_enum] = job_class?
This is a very nice suggestion that I gladly adopted, see the
incremental diff.

>> +
>> + return dict(job_classes)
>> +
>> + @property
>> def specific_job(self):
>> """See `IBuildQueue`."""
>> + specific_class = self.specific_job_classes[self.job_type]
>> store = Store.of(self)
>> result_set = store.find(
>> - BuildPackageJob, BuildPackageJob.job == self.job)
>> + specific_class, specific_class.job == self.job)
>> return result_set.one()
>
> As mentioned at the top, this *could* simply be:
>
> def specific_job(self):
> return IBuildFarmJob(self)
>
> (with most of the above storm code moved into a factory) if you used an
> adapter, but there may be reasons for not doing so.
We have already discussed this at length.

>> @property
>>
>> === modified file 'lib/lp/soyuz/tests/test_buildqueue.py'
>> --- lib/lp/soyuz/tests/test_buildqueue.py 2010-01-04 10:58:09 +0000
>> +++ lib/lp/soyuz/tests/test_buildqueue.py 2010-01-07 08:23:28 +0000
>> @@ -12,6 +12,7 @@
>> IStoreSelector, MAIN_STORE, DEFAULT_FLAVOR)
>> from canonical.testing import LaunchpadZopelessLayer
>>
>> +from lp.buildmaster.interfaces.buildfarmjob import BuildFarmJobType
>> from lp.soyuz.interfaces.archive import ArchivePurpose
>> from lp.soyuz.interfaces.build import BuildStatus
>> from lp.soyuz.interfaces.builder import IBuilderSet
>> @@ -330,3 +331,80 @@
>> free_count = bq._freeBuildersCount(
>> build.processor, build.is_virtualized)
>> self.assertEqual(free_count, 1)
>> +
>> +
>> +class TestJobClasses(TestCaseWithFactory):
>> + """Tests covering build farm job type classes."""
>> + layer = LaunchpadZopelessLayer
>> + def setUp(self):
>> + """Set up a native x86 build for the test archive."""
>> + super(TestJobClasses, self).setUp()
>> +
>> + self.publisher = SoyuzTestPublisher()
>> + self.publisher.prepareBreezyAutotest()
>> +
>> + # First mark all builds in the sample data as already built.
>> + store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
>> + sample_data = store.find(Build)
>> + for build in sample_data:
>> + build.buildstate = BuildStatus.FULLYBUILT
>> + store.flush()
>> +
>> + # We test builds that target a primary archive.
>> + self.non_ppa = self.factory.makeArchive(
>> + name="primary", purpose=ArchivePurpose.PRIMARY)
>> + self.non_ppa.require_virtualized = False
>> +
>> + self.builds = []
>> + self.builds.extend(
>> + self.publisher.getPubSource(
>> + sourcename="gedit", status=PackagePublishingStatus.PUBLISHED,
>> + archive=self.non_ppa).createMissingBuilds())
>> +
>> + def test_BuildPackageJob(self):
>> + """`BuildPackageJob` is one of the job type classes."""
>> + from lp.soyuz.model.buildpackagejob import BuildPackageJob
>> + _build, bq = find_job(self, 'gedit')
>> +
>> + # This is a binary package build.
>> + self.assertEqual(
>> + bq.job_type, BuildFarmJobType.PACKAGEBUILD,
>> + "This is a binary package build")
>> +
>> + # The class registered for 'PACKAGEBUILD' is `BuildPackageJob`.
>> + self.assertEqual(
>> + bq.specific_job_classes[BuildFarmJobType.PACKAGEBUILD],
>> + BuildPackageJob,
>> + "The class registered for 'PACKAGEBUILD' is `BuildPackageJob`")
>> +
>> + # The 'specific_job' object associated with this `BuildQueue`
>> + # instance is of type `BuildPackageJob`.
>> + self.assertTrue(bq.specific_job is not None)
>> + self.assertEqual(
>> + bq.specific_job.__class__, BuildPackageJob,
>> + "The 'specific_job' object associated with this `BuildQueue` "
>> + "instance is of type `BuildPackageJob`")
>> +
>> + def test_OtherTypeClasses(self):
>> + """Other job type classes are picked up as well."""
>> + from zope import component
>> + from lp.buildmaster.interfaces.buildfarmjob import IBuildFarmJob
>> + class FakeBranchBuild:
>> + pass
>> +
>> + _build, bq = find_job(self, 'gedit')
>> + # First make sure that we don't have a job type class registered for
>> + # 'BRANCHBUILD' yet.
>> + self.assertTrue(
>> + bq.specific_job_classes.get(BuildFarmJobType.BRANCHBUILD) is None)
>> +
>> + # Pretend that our `FakeBranchBuild` class implements the
>> + # `IBuildFarmJob` interface.
>> + component.provideUtility(
>> + FakeBranchBuild, IBuildFarmJob, 'BRANCHBUILD')
>> +
>> + # Now we should see the `FakeBranchBuild` class "registered" in the
>> + # `specific_job_classes` dictionary under the 'BRANCHBUILD' key.
>> + self.assertEqual(
>> + bq.specific_job_classes[BuildFarmJobType.BRANCHBUILD],
>> + FakeBranchBuild)

Best regards

--
Muharem Hrnjadovic <email address hidden>
Public key id : B2BBFCFC
Key fingerprint : A5A3 CC67 2B87 D641 103F 5602 219F 6B60 B2BB FCFC