Merge lp:~jelmer/launchpad/archive-arch-fix into lp:launchpad

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

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

MY changes are here:
lp:~julian-edwards/launchpad/archive-arch-fix

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 05.07.2010 12:43, schrieb Julian Edwards:
> I'm taking this over since Jelmer's away for 2 weeks.

Hi Julian! ;)

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

Wow, all code is equal but some code is more equal than other? I am not sure I
am buying this. ;-) Is it not all Launchpad code, Julian? I don't think that
"we don't do that in Soyuz/Rosetta/Malone/etc. code" should ever be a valid
excuse to diverge from the coding styles. That just makes reviewing other
teams' code so much harder when you have to consider their conventions. And
think about the nightmare for community contributors ...

OK, both ideas, Soyuz code receiving special treatment in reviews and the
required use of celebs (or not) should be discussed among reviewers. Will you
add them to the agenda or shall I do it?

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

Thanks.

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

Great!

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

Good.

>
>> # 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 :)

Understood. Fair enough. Maybe record a tech-debt bug for this?

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

Ok. Again, a bug will help to remember it. ;)

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

Thanks.

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

On Monday 05 July 2010 12:43:24 Henning Eggers wrote:
> Review: Approve code

Cheers.

>
> Am 05.07.2010 12:43, schrieb Julian Edwards:
> > I'm taking this over since Jelmer's away for 2 weeks.
>
> Hi Julian! ;)

Hai!

> Wow, all code is equal but some code is more equal than other? I am not
> sure I am buying this. ;-) Is it not all Launchpad code, Julian? I don't
> think that "we don't do that in Soyuz/Rosetta/Malone/etc. code" should
> ever be a valid excuse to diverge from the coding styles. That just makes
> reviewing other teams' code so much harder when you have to consider their
> conventions. And think about the nightmare for community contributors ...

Well, consider that celebrities are essentially hard-coded special cases for a
moment. This in itself is pretty gross, but sometimes unfortunately
unavoidable.

For me, the use of a celebrity means that the code in question *requires* that
particular celebrity for some special case and that using a different value of
the same type won't work.

In the case of this branch, that's not true. It could be argued in fact that
the test should be using makeDistro instead, but I expect there's some bizarre
sample data weirdness (which unfortunately Soyuz depends on heavily) requiring
the use of Ubuntu.

It therefore makes perfect sense that the code is pulling "ubuntu" like that
instead of using the celebrity.

Did I explain that nuance well enough?

> OK, both ideas, Soyuz code receiving special treatment in reviews and the
> required use of celebs (or not) should be discussed among reviewers. Will
> you add them to the agenda or shall I do it?

Feel free if you want to discuss it.

> > I need to get an R-C for this so I'll leave that for another time if you
> > don't mind :)
>
> Understood. Fair enough. Maybe record a tech-debt bug for this?

Actually I ended up doing it, it wasn't many.

> The style guide says:
> "When asserting for equality use the form assertEqual(expected_results,
> actual_results, "...")".
> I remember that we agreed not to go around and change existing tests unless
> we touch them but it is definitely the recommended style for new tests.

Euargh. I could have sworn it was the other way around. This is going to
really confuse me now. I'd love to see the IRC logs for that conversation
that we had in the reviewers' meeting again though.

> I am not going into who you might be referring to by "we" here ... :-P

The royal We? :)

> Yes, I have always been a fan of those. The asserts in LaunchpadTestCase
> already provide strings and I guess that's why we don't see them a lot in
> our assert calls. I have actually had reviewers ask me to remove them.
>
> > A non-matching dictionary would look messy in comparison, no?
>
> You can test all three conditions at the same time. You can see how many
> rows were returned instead and what their values are. Much more useful.
>
> The problem with multiple assert statements in one test method is that not
> all will be executed if one fails and you are missing valuable information
> that you can only gain by changing the test code and running the test
> again.

That's an interesting point that I hadn't considered.

> ...