Code review comment for lp:~jelmer/launchpad/archive-arch-fix

On Monday 05 July 2010 11:07:27 Henning Eggers wrote:
> Hi Jelmer,
> thank you for this quick fix. I have a few comments about the tests and
> will wait for your reply before I approve the branch.
>
> Cheers,
> Henning

Thanks Henning.

I'm taking this over since Jelmer's away for 2 weeks.

> > + ubuntu = getUtility(IDistributionSet)['ubuntu']
> > + self.ubuntu_archive = ubuntu.main_archive
>
> We have a celebrity that you should use for this.
>
> getUtility(ILaunchpadCelebrities).ubuntu

We hardly ever use it in Soyuz code, I don't see point of changing it here,
celebs should be optional.

> > { 'arm' : False, 'cell-proc' : True, 'omap' : False},
>
> Leading space. ^ Copied&pasted further down ... ;)

I'll fix those.

>
> > results)
> >
> > + def test_restricted_association_archive_only(self):
> Looks like this naming is used throughout this file but the correct name
> would probably be "test_getRestrictedFamilies_archive_only" or similar.
>
> https://dev.launchpad.net/TestsStyleGuide#Python Test Cases

Yeah those are wrong, I'll fix the names.

> > + """Test that only the associated archs for the archive itself
> > are + returned."""
>
> This doc string violates our docstring conventions but you don't need a doc
> string for test methods anyway, just a comment.

I'll fix that too!

> # Test that only the associated archs for the archive itself are
> # returned.
>
> I see more instances of that, maybe you correct fix them as a drive-by fix?
> Thank you very much!

I need to get an R-C for this so I'll leave that for another time if you don't
mind :)

>
> > + getUtility(IArchiveArchSet).new(self.ppa, self.cell_proc)
> > + getUtility(IArchiveArchSet).new(self.ubuntu_archive, self.omap)
>
> Hm, maybe you could add a factory method to the LaunchpadObjectFactory for
> ArchiveArch? Or are there reasons against that?

We could do. But this is an R-C so we'll do it later!

>
> > + result_set = list(
> > + getUtility(IArchiveArchSet).getRestrictedfamilies(self.ppa))
>
> Else, the repeated calls to getUtility(IArchiveArchSet) clutter the code.
> Maybe you should use a local or instance variable to store that?

I can do that.

> Also, I think getRestrictedfamilies should be getRestrictedFamilies to be
> proper camelCase. I know this is not your doing but could you please fix
> that if it does not have too many call sites?

Yeah I can fix that.

> > + results = dict(
> > + (row[0].name, row[1] is not None) for row in result_set)
> > + self.assertEquals(
> > + { 'arm' : False, 'cell-proc' : True, 'omap' : False},
>
> Leading space. ^

Yep.

>
> > + results)
> > +
> > + def test_get_by_archive(self):
> > + """Test ArchiveArchSet.getByArchive."""
> > + getUtility(IArchiveArchSet).new(self.ppa, self.cell_proc)
> > + getUtility(IArchiveArchSet).new(self.ubuntu_archive, self.omap)
> > + result_set = list(
> > + getUtility(IArchiveArchSet).getByArchive(self.ppa))
>
> Same comment about storing archive_arch_set

Yep.

>
> > + self.assertEquals(1, len(result_set))
> > + self.assertEquals(self.ppa, result_set[0].archive)
> > + self.assertEquals(self.cell_proc, result_set[0].processorfamily)
>
> I quite liked the approach from the first test where you converted the
> complex result set to a dict. Maybe you can do something similar here to
> have only one assert in the end? In case of a test failure the failure
> message will be much clearer.

The assertions are also the wrong way around, we use:
assertEqual(actual value, expected value)

I prefer the non-dictionary approach myself. The (missing) third arg to
assertEqual should be a string that explains the failure.

A non-matching dictionary would look messy in comparison, no?

> My suggestion:
>
> result = [(row.archive, row.processorfamily) for row in result_set]
> self.assertEqual([(self.ppa, self.cell_proc)], result)

I find this quite hard to read as a test :(

Cheers
J