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