launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Needs Fixing

Mostly excellent; just a few style nits and some missed preloading.

Diff comments:

> 
> === modified file 'lib/lp/snappy/model/snap.py'
> --- lib/lp/snappy/model/snap.py	2016-05-11 00:00:47 +0000
> +++ lib/lp/snappy/model/snap.py	2016-05-14 00:27:07 +0000
> @@ -27,10 +27,13 @@
>  from zope.security.interfaces import Unauthorized
>  from zope.security.proxy import removeSecurityProxy
>  
> +

Superfluous blank line.

> +from lp.app.browser.tales import DateTimeFormatterAPI
>  from lp.app.enums import PRIVATE_INFORMATION_TYPES
>  from lp.app.interfaces.security import IAuthorization
>  from lp.buildmaster.enums import BuildStatus
>  from lp.buildmaster.interfaces.processor import IProcessorSet
> +from lp.buildmaster.interfaces.buildqueue import IBuildQueueSet

Please keep imports sorted within each block.  utilities/format-new-and-modified-imports (or utilities/format-imports) can help with this.

>  from lp.buildmaster.model.processor import Processor
>  from lp.code.interfaces.branch import IBranch
>  from lp.code.interfaces.branchcollection import (
> @@ -338,6 +341,39 @@
>          result.order_by(order_by)
>          return result
>  
> +    def getBuildSummariesForSnapBuildIds(self, snap_build_ids):
> +        """See `ISnap`."""
> +        result = {}
> +        if snap_build_ids is None:
> +            return result
> +        filter_term = SnapBuild.id.is_in(snap_build_ids)
> +        order_by = Desc(SnapBuild.id)
> +        builds = self._getBuilds(filter_term, order_by)
> +
> +        # Prefetch data to keep DB query count constant
> +        getUtility(IBuildQueueSet).preloadForBuildFarmJobs(builds)
> +
> +        for build in builds:
> +            if build.date is not None:
> +                when_complete = DateTimeFormatterAPI(build.date).displaydate()
> +            else:
> +                when_complete = None
> +
> +            if build.log:
> +                build_log_size = build.log.content.filesize

I think you may be missing preloading of the LibraryFileAlias/LibraryFileContent before you get to this.  Something like:

  lfas = load_related(LibraryFileAlias, builds, ["log_id"])
  load_related(LibraryFileContent, lfas, ["contentID"])

> +            else:
> +                build_log_size = None
> +
> +            result[build.id] = {}
> +            result[build.id]["status"] = build.status.name
> +            result[build.id]["buildstate"] = build.status
> +            result[build.id]["when_complete"] = when_complete
> +            result[build.id]["when_complete_estimate"] = build.estimate
> +            result[build.id]["build_log_url"] = build.log_url
> +            result[build.id]["build_log_size"] = build_log_size

I'd probably use a dict literal.

> +
> +        return result
> +
>      @property
>      def builds(self):
>          """See `ISnap`."""
> 
> === modified file 'lib/lp/snappy/tests/test_snap.py'
> --- lib/lp/snappy/tests/test_snap.py	2016-05-06 09:45:45 +0000
> +++ lib/lp/snappy/tests/test_snap.py	2016-05-14 00:27:07 +0000
> @@ -364,6 +365,70 @@
>              snap.destroySelf()
>          self.assertFalse(getUtility(ISnapSet).exists(owner, u"condemned"))
>  
> +    def test_getBuildSummariesForSnapBuildIds(self):
> +        snap1 = self.factory.makeSnap()
> +        snap2 = self.factory.makeSnap()
> +        build11 = self.factory.makeSnapBuild(snap=snap1)
> +        build12 = self.factory.makeSnapBuild(snap=snap1)
> +        build2 = self.factory.makeSnapBuild(snap=snap2)
> +        build3 = self.factory.makeSnapBuild()
> +        summary1 = snap1.getBuildSummariesForSnapBuildIds(
> +            [build11.id, build12.id])
> +        summary2 = snap2.getBuildSummariesForSnapBuildIds([build2.id])
> +        self.assertEqual([build11.id, build12.id], summary1.keys())

Doesn't this need to be assertContentEqual instead?  There's no guarantee on the ordering of dict.keys().

> +        self.assertEqual([build2.id], summary2.keys())
> +
> +    def test_getBuildSummariesForSnapBuildIds_empty_input(self):
> +        snap = self.factory.makeSnap()
> +        self.factory.makeSnapBuild(snap=snap)
> +        self.assertEqual({}, snap.getBuildSummariesForSnapBuildIds(None))
> +        self.assertEqual({}, snap.getBuildSummariesForSnapBuildIds([]))
> +        self.assertEqual({}, snap.getBuildSummariesForSnapBuildIds(()))
> +        self.assertEqual({}, snap.getBuildSummariesForSnapBuildIds([None]))
> +
> +    def test_getBuildSummariesForSnapBuildIds_not_matching_snap(self):
> +        # Should not return build summaries of other snaps.
> +        snap1 = self.factory.makeSnap()
> +        snap2 = self.factory.makeSnap()
> +        build1 = self.factory.makeSnapBuild(snap=snap1)
> +        build2 = self.factory.makeSnapBuild(snap=snap2)
> +        summary1 = snap1.getBuildSummariesForSnapBuildIds([build2.id])
> +        self.assertEqual({}, summary1)
> +
> +    def test_getBuildSummariesForSnapBuildIds_when_complete_field(self):
> +        # Summary "when_complete" should be None unless estimate date or
> +        # finish date is available.
> +        snap = self.factory.makeSnap()
> +        build = self.factory.makeSnapBuild(snap=snap)
> +        self.assertIsNone(build.date)
> +        summary = snap.getBuildSummariesForSnapBuildIds([build.id])
> +        self.assertIsNone(summary[build.id]["when_complete"])
> +        removeSecurityProxy(build).date_finished = UTC_NOW
> +        summary = snap.getBuildSummariesForSnapBuildIds([build.id])
> +        self.assertEqual("a moment ago", summary[build.id]["when_complete"])
> +
> +    def test_getBuildSummariesForSnapBuildIds_log_size_field(self):
> +        # Summary "build_log_size" should be None unless the build has a log.
> +        snap = self.factory.makeSnap()
> +        build = self.factory.makeSnapBuild(snap=snap)
> +        self.assertIsNone(build.log)
> +        summary = snap.getBuildSummariesForSnapBuildIds([build.id])
> +        self.assertIsNone(summary[build.id]["build_log_size"])
> +        removeSecurityProxy(build).log = self.factory.makeLibraryFileAlias(
> +            content='x' * 12345, db_only=True)
> +        summary = snap.getBuildSummariesForSnapBuildIds([build.id])
> +        self.assertEqual(12345, summary[build.id]["build_log_size"])
> +
> +    def test_getBuildSummariesForSnapBuildIds_query_count(self):
> +        # DB query count should remain constant regardless of number of builds.
> +        snap = self.factory.makeSnap()
> +        recorder1, recorder2 = record_two_runs(
> +            lambda: snap.getBuildSummariesForSnapBuildIds(
> +                build.id for build in snap.builds),
> +            lambda: self.factory.makeSnapBuild(snap=snap),
> +            1, 5)

I think this is why you missed the LFA/LFC preloading above; the item_creator here is going to need to set build.log for each build it creates.

> +        self.assertThat(recorder2, HasQueryCount.byEquality(recorder1))
> +
>  
>  class TestSnapDeleteWithBuilds(TestCaseWithFactory):
>  


-- 
https://code.launchpad.net/~maxiberta/launchpad/snap-update-build-status-ui/+merge/294403
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~maxiberta/launchpad/snap-update-build-status-ui into lp:launchpad.