Launchpad itself

Code review comment for lp:~al-maisan/launchpad/disabled-copyarch-519665

disabled-copyarch-519665
Merge into devel

Revision history for this message

Muharem Hrnjadovic (al-maisan) wrote on 2010-02-11:

On 02/10/2010 06:00 PM, Jeroen T. Vermeulen wrote:
> Review: Needs Fixing code
> I have a few additional notes that Gavin thought were worth adding:
Hello,

thanks for posing these questions, they are all pertinent and good.

> * What did the pre-imp conclude about the problem of re-enabling
> archives? Is it not going to be a problem to un-suspend jobs when
> that happens?
We already have a mechanism that takes care of suspending/un-suspending
binary build jobs when the associated archive is disabled and enabled
respectively.
Please see Archive.enable() and Archive.disable() for more detail.

The problem in this specific case is that the so called copy archives
are instantiated in a disabled state. Hence there is no

"enabled -> disabled"

state transition for these and their binary build jobs are created in an
unsuspended state which is a bug.

Please note also that the branch at hand enforces the constraint the
binary builds for disabled archives should be (created) suspended
irrespective of archive type (although the problem manifested itself in
a copy archive context).

> * In build_not_pending_and_suspended, the "not" is ambiguous in
> scope. Can't you replace this with two separate checks?
Hmm .. maybe I can name it better. How about

- build_in_wrong_state() plus
- an explanation what "wrong" means in the case at hand

> * In production code we check "len(foo) == 0" to check for empty
> lists. But in tests it's nicer to compare "foo == []" so that a
> failure will give more information.
OK. Done.

> * With assertEqual, pass expected value first and actual value
> second! Again, better failure messages.
Oh, yes! Fixed that as well.

Please see also the enclosed incremental diff.

Best regards

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

disabled-copyarch-519665-inc.diff

« Back to merge proposal

 === modified file 'lib/lp/soyuz/scripts/tests/test_populatearchive.py'
 --- lib/lp/soyuz/scripts/tests/test_populatearchive.py	2010-02-10 15:54:40 +0000
 +++ lib/lp/soyuz/scripts/tests/test_populatearchive.py	2010-02-11 06:22:10 +0000
@@ -651,11 +651,11 @@
      def testBuildsPendingAndSuspended(self):
          """All builds in the new copy archive are pending and suspended."""
--        def build_not_pending_and_suspended(build):
--            """True if the given build is not pending and suspended."""
--            return (
--                build.buildstate != BuildStatus.NEEDSBUILD or
--                build.buildqueue_record.job.status != JobStatus.SUSPENDED)
++        def build_in_wrong_state(build):
++            """True if the given build is not (pending and suspended)."""
++            return not (
++                build.buildstate == BuildStatus.NEEDSBUILD and
++                build.buildqueue_record.job.status == JobStatus.SUSPENDED)
          hoary = getUtility(IDistributionSet)['ubuntu']['hoary']
          # Verify that we have the right source packages in the sample data.
@@ -667,11 +667,20 @@
          # Make sure the right source packages were cloned.
          self._verifyClonedSourcePackages(archive, hoary)
--        # Now check that we have the build records expected.
++        # Get the binary builds generated for the copy archive at hand.
          builds = list(getUtility(IBuildSet).getBuildsForArchive(archive))
++        # At least one binary build was generated for the target copy archive.
          self.assertTrue(len(builds) > 0)
--        wrong_builds = filter(build_not_pending_and_suspended, builds)
--        self.assertEqual(0, len(wrong_builds))
++        # Now check that the binary builds and their associated job records
++        # are in the state expected:
++        #   - binary build: pending
++        #   - job: suspended
++        builds_in_wrong_state = filter(build_in_wrong_state, builds)
++        self.assertEqual (
++            [], builds_in_wrong_state,
++            "The binary builds generated for the target copy archive "
++            "should all be pending and suspended. However, at least one of "
++            "the builds is in the wrong state.")
      def testPrivateOriginArchive(self):
          """Try copying from a private archive.