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

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

Am 04.07.2010 01:40, schrieb Jelmer Vernooij:
> === modified file 'lib/lp/soyuz/model/archivearch.py'
> --- lib/lp/soyuz/model/archivearch.py 2010-02-25 15:25:30 +0000
> +++ lib/lp/soyuz/model/archivearch.py 2010-07-03 23:40:54 +0000
> @@ -13,7 +13,7 @@
> from canonical.launchpad.webapp.interfaces import (
> IStoreSelector, MAIN_STORE, DEFAULT_FLAVOR)
>
> -from storm.expr import Join, LeftJoin
> +from storm.expr import And, Join, LeftJoin
> from storm.locals import Int, Reference, Storm
>
>
> @@ -65,7 +65,8 @@
> ProcessorFamily,
> LeftJoin(
> ArchiveArch,
> - ArchiveArch.processorfamily == ProcessorFamily.id))
> + And(ArchiveArch.archive == archive.id,
> + ArchiveArch.processorfamily == ProcessorFamily.id)))
> result_set = store.using(*origin).find(
> (ProcessorFamily, ArchiveArch),
> (ProcessorFamily.restricted == True))
>
> === modified file 'lib/lp/soyuz/tests/test_archivearch.py'
> --- lib/lp/soyuz/tests/test_archivearch.py 2010-02-25 15:25:30 +0000
> +++ lib/lp/soyuz/tests/test_archivearch.py 2010-07-03 23:40:54 +0000
> @@ -11,6 +11,7 @@
>
> from lp.testing import TestCaseWithFactory
>
> +from lp.registry.interfaces.distribution import IDistributionSet

from canonical.launchpad.interfaces import ILaunchpadCelebrities

See below.

> from lp.registry.interfaces.person import IPersonSet
> from lp.soyuz.interfaces.archivearch import IArchiveArchSet
> from lp.soyuz.interfaces.processor import IProcessorFamilySet
> @@ -24,6 +25,8 @@
> super(TestArchiveArch, self).setUp()
>
> self.ppa = getUtility(IPersonSet).getByName('cprov').archive
> + ubuntu = getUtility(IDistributionSet)['ubuntu']
> + self.ubuntu_archive = ubuntu.main_archive

We have a celebrity that you should use for this.

getUtility(ILaunchpadCelebrities).ubuntu

> pss = getUtility(IProcessorFamilySet)
> self.cell_proc = pss.new(
> 'cell-proc', 'PS cell processor', 'Screamingly faaaaaaaaaaaast',
> @@ -32,7 +35,7 @@
> 'omap', 'Multimedia applications processor',
> 'Does all your sound & video', True)
>
> - def test_no_associations(self):
> + def test_no_restricted_uassociations(self):
> """Our archive is not associated with any restricted processor
> families yet."""
> result_set = list(
> @@ -40,7 +43,7 @@
> archivearches = [row[1] for row in result_set]
> self.assertTrue(all(aa is None for aa in archivearches))
>
> - def test_single_association(self):
> + def test_single_restricted_association(self):
> """Our archive is now associated with one of the restricted processor
> families."""
> getUtility(IArchiveArchSet).new(self.ppa, self.cell_proc)
> @@ -52,6 +55,29 @@
> { 'arm' : False, 'cell-proc' : True, 'omap' : False},

Leading space. ^ Copied&pasted further down ... ;)

> 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

> + """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.

        # 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!

> + 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?

> + 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?

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?

> + 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. ^

> + 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

> + 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.

My suggestion:

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

> +
>
> def test_suite():
> return unittest.TestLoader().loadTestsFromName(__name__)
>