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__)
>