Code review comment for lp:~stevenk/launchpad/bpb-currentcomponent-assertion-part-4

Hi Steven,

I have a few comments, but they're all minor, and you could land
without addressing any of them.

Awesome doctest killing!

Gavin.

[1]

+ # We will also need to log into an admin to do the rescore

Full-stop. Ho ho.

[2]

Some lint:

./lib/lp/soyuz/doc/binarypackagebuild.txt
       1: narrative uses a moin header.
      48: source exceeds 78 characters.
     113: narrative uses a moin header.
     137: source exceeds 78 characters.
     138: source exceeds 78 characters.
     162: source exceeds 78 characters.
     163: source exceeds 78 characters.
./lib/lp/soyuz/tests/test_build.py
     219: W291 trailing whitespace
     219: Line has trailing whitespace.
./lib/lp/soyuz/tests/test_build_depwait.py
      25: E302 expected 2 blank lines, found 1
      71: W291 trailing whitespace
      71: Line has trailing whitespace.
      88: Line exceeds 78 characters.
./lib/lp/soyuz/tests/test_build_set.py
      31: E302 expected 2 blank lines, found 1
     192: W291 trailing whitespace
     192: Line has trailing whitespace.

I guess it's not worth addressing the binarypackagebuild.txt lint, but
please take a look at the others.

[3]

+ def test_update_dependancies(self):
+ # Calling .updateDependencies() on a build will update will remove
+ # those which are reachable.

"will update" or "will remove"?

[4]

+ # Commit to make sure stuff hits the database.
+ transaction.commit()

Is this absolutely needed? I believe it slows down the test tear-down,
but I could be wrong. Perhaps flush_database_updates() would be
sufficient? If so, there are several commits in the new tests that
might be better as flushes instead.

[5]

+ self.assertTrue(build.buildqueue_record.lastscore > 0)

In case of failure, this might be more informative if written as:

        self.assertThat(LessThan(0, build.buildqueue_record.lastscore))

(LessThan is from testtools.matchers.)

But it's not very important; I mention it really to make you aware of
matchers in case they're interesting later on. There are some LP
specific matchers in lp.testing.matchers too.

[6]

In TestBuildPrivacy.setUp():

+ [public_build] = public_spph.createMissingBuilds()
...
+ with person_logged_in(self.admin):
+ [private_build] = private_spph.createMissingBuilds()

Is the log-in when creating private_build necessary because it's
private?

[7]

+ def _get_build_title(self, build):
+ return build.title

This doesn't seem to be very useful!

[8]

+ self.assertRaises(
+ Unauthorized, getattr, private, 'getBuildRecords')

This might read better with a lambda:

            self.assertRaises(
                Unauthorized, lambda: private.getBuildRecords)

This appears twice.

[9]

+ expected_titles = []
+ for spph in spphs:
+ for das in (self.das_one, self.das_two):
+ expected_titles.append(
+ '%s build of %s %s in %s %s RELEASE' % (
+ das.architecturetag, spph.source_package_name,
+ spph.source_package_version,
+ self.distroseries.distribution.name,
+ self.distroseries.name))
+ build_titles = [build.title for build in builds]
+ self.assertEquals(sorted(expected_titles), sorted(build_titles))

Why did you use titles to compare the things returned? Is there a more
direct way to do the comparison?

[10]

To tidy up your imports please run:

  utilities/format-new-and-modified-imports -r :prev