Merge lp:~jelmer/launchpad/archive-arch-fix into lp:launchpad
- archive-arch-fix
- Merge into devel
Status: | Merged |
---|---|
Approved by: | Māris Fogels |
Approved revision: | no longer in the source branch. |
Merged at revision: | 11093 |
Proposed branch: | lp:~jelmer/launchpad/archive-arch-fix |
Merge into: | lp:launchpad |
Diff against target: |
91 lines (+31/-4) 2 files modified
lib/lp/soyuz/model/archivearch.py (+3/-2) lib/lp/soyuz/tests/test_archivearch.py (+28/-2) |
To merge this branch: | bzr merge lp:~jelmer/launchpad/archive-arch-fix |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Māris Fogels (community) | release-critical | Approve | |
Henning Eggers (community) | code | Approve | |
Review via email: mp+29160@code.launchpad.net |
Commit message
Description of the change
ArchiveArchSet.
This also includes a test for ArchiveArchSet.
Henning Eggers (henninge) wrote : | # |
Julian Edwards (julian-edwards) wrote : | # |
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(
> > + self.ubuntu_archive = ubuntu.main_archive
>
> We have a celebrity that you should use for this.
>
> getUtility(
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
> Looks like this naming is used throughout this file but the correct name
> would probably be "test_getRestri
>
> https:/
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(
> > + getUtility(
>
> Hm, maybe you could add a factory method to the LaunchpadObject
> 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(
>
> Else, the repeated calls to getUtility(
> Maybe you should use a local or instance variable to store that?
I can do that.
> Also, I think getRestrictedfa
> 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_
> > + """Test ArchiveArchSet.
> > + getUtility(
> > + getUtility(
> > + result_set = list(
> > + getUtility(
>
> Same comment about storing archive_arch_set
Yep.
>
> > + self.assertEqual...
Julian Edwards (julian-edwards) wrote : | # |
MY changes are here:
lp:~julian-edwards/launchpad/archive-arch-fix
Henning Eggers (henninge) wrote : | # |
-----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(
>>> + self.ubuntu_archive = ubuntu.main_archive
>>
>> We have a celebrity that you should use for this.
>>
>> getUtility(
>
> 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/
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
>> Looks like this naming is used throughout this file but the correct name
>> would probably be "test_getRestri
>>
>> https:/
>
> 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(
>>> + getUtility(
>>
>> Hm, maybe you could add a factory method to the LaunchpadObject
>> 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(
>>
>> Else, the repeated calls to getUtility(
>> Maybe you should use a local or instance variable to store that?
>
> I can do that.
Thanks.
>
>> Also, I think getRestrictedfa
>> proper camelCase. I know this is not your doing but could you please fix
>> that if it does not have too many call sites?
...
Julian Edwards (julian-edwards) wrote : | # |
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/
> 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(
> 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.
> ...
Māris Fogels (mars) : | # |
Preview Diff
1 | === modified file 'lib/lp/soyuz/model/archivearch.py' | |||
2 | --- lib/lp/soyuz/model/archivearch.py 2010-02-25 15:25:30 +0000 | |||
3 | +++ lib/lp/soyuz/model/archivearch.py 2010-07-03 23:45:44 +0000 | |||
4 | @@ -13,7 +13,7 @@ | |||
5 | 13 | from canonical.launchpad.webapp.interfaces import ( | 13 | from canonical.launchpad.webapp.interfaces import ( |
6 | 14 | IStoreSelector, MAIN_STORE, DEFAULT_FLAVOR) | 14 | IStoreSelector, MAIN_STORE, DEFAULT_FLAVOR) |
7 | 15 | 15 | ||
9 | 16 | from storm.expr import Join, LeftJoin | 16 | from storm.expr import And, Join, LeftJoin |
10 | 17 | from storm.locals import Int, Reference, Storm | 17 | from storm.locals import Int, Reference, Storm |
11 | 18 | 18 | ||
12 | 19 | 19 | ||
13 | @@ -65,7 +65,8 @@ | |||
14 | 65 | ProcessorFamily, | 65 | ProcessorFamily, |
15 | 66 | LeftJoin( | 66 | LeftJoin( |
16 | 67 | ArchiveArch, | 67 | ArchiveArch, |
18 | 68 | ArchiveArch.processorfamily == ProcessorFamily.id)) | 68 | And(ArchiveArch.archive == archive.id, |
19 | 69 | ArchiveArch.processorfamily == ProcessorFamily.id))) | ||
20 | 69 | result_set = store.using(*origin).find( | 70 | result_set = store.using(*origin).find( |
21 | 70 | (ProcessorFamily, ArchiveArch), | 71 | (ProcessorFamily, ArchiveArch), |
22 | 71 | (ProcessorFamily.restricted == True)) | 72 | (ProcessorFamily.restricted == True)) |
23 | 72 | 73 | ||
24 | === modified file 'lib/lp/soyuz/tests/test_archivearch.py' | |||
25 | --- lib/lp/soyuz/tests/test_archivearch.py 2010-02-25 15:25:30 +0000 | |||
26 | +++ lib/lp/soyuz/tests/test_archivearch.py 2010-07-03 23:45:44 +0000 | |||
27 | @@ -11,6 +11,7 @@ | |||
28 | 11 | 11 | ||
29 | 12 | from lp.testing import TestCaseWithFactory | 12 | from lp.testing import TestCaseWithFactory |
30 | 13 | 13 | ||
31 | 14 | from lp.registry.interfaces.distribution import IDistributionSet | ||
32 | 14 | from lp.registry.interfaces.person import IPersonSet | 15 | from lp.registry.interfaces.person import IPersonSet |
33 | 15 | from lp.soyuz.interfaces.archivearch import IArchiveArchSet | 16 | from lp.soyuz.interfaces.archivearch import IArchiveArchSet |
34 | 16 | from lp.soyuz.interfaces.processor import IProcessorFamilySet | 17 | from lp.soyuz.interfaces.processor import IProcessorFamilySet |
35 | @@ -24,6 +25,8 @@ | |||
36 | 24 | super(TestArchiveArch, self).setUp() | 25 | super(TestArchiveArch, self).setUp() |
37 | 25 | 26 | ||
38 | 26 | self.ppa = getUtility(IPersonSet).getByName('cprov').archive | 27 | self.ppa = getUtility(IPersonSet).getByName('cprov').archive |
39 | 28 | ubuntu = getUtility(IDistributionSet)['ubuntu'] | ||
40 | 29 | self.ubuntu_archive = ubuntu.main_archive | ||
41 | 27 | pss = getUtility(IProcessorFamilySet) | 30 | pss = getUtility(IProcessorFamilySet) |
42 | 28 | self.cell_proc = pss.new( | 31 | self.cell_proc = pss.new( |
43 | 29 | 'cell-proc', 'PS cell processor', 'Screamingly faaaaaaaaaaaast', | 32 | 'cell-proc', 'PS cell processor', 'Screamingly faaaaaaaaaaaast', |
44 | @@ -32,7 +35,7 @@ | |||
45 | 32 | 'omap', 'Multimedia applications processor', | 35 | 'omap', 'Multimedia applications processor', |
46 | 33 | 'Does all your sound & video', True) | 36 | 'Does all your sound & video', True) |
47 | 34 | 37 | ||
49 | 35 | def test_no_associations(self): | 38 | def test_no_restricted_associations(self): |
50 | 36 | """Our archive is not associated with any restricted processor | 39 | """Our archive is not associated with any restricted processor |
51 | 37 | families yet.""" | 40 | families yet.""" |
52 | 38 | result_set = list( | 41 | result_set = list( |
53 | @@ -40,7 +43,7 @@ | |||
54 | 40 | archivearches = [row[1] for row in result_set] | 43 | archivearches = [row[1] for row in result_set] |
55 | 41 | self.assertTrue(all(aa is None for aa in archivearches)) | 44 | self.assertTrue(all(aa is None for aa in archivearches)) |
56 | 42 | 45 | ||
58 | 43 | def test_single_association(self): | 46 | def test_single_restricted_association(self): |
59 | 44 | """Our archive is now associated with one of the restricted processor | 47 | """Our archive is now associated with one of the restricted processor |
60 | 45 | families.""" | 48 | families.""" |
61 | 46 | getUtility(IArchiveArchSet).new(self.ppa, self.cell_proc) | 49 | getUtility(IArchiveArchSet).new(self.ppa, self.cell_proc) |
62 | @@ -52,6 +55,29 @@ | |||
63 | 52 | { 'arm' : False, 'cell-proc' : True, 'omap' : False}, | 55 | { 'arm' : False, 'cell-proc' : True, 'omap' : False}, |
64 | 53 | results) | 56 | results) |
65 | 54 | 57 | ||
66 | 58 | def test_restricted_association_archive_only(self): | ||
67 | 59 | """Test that only the associated archs for the archive itself are | ||
68 | 60 | returned.""" | ||
69 | 61 | getUtility(IArchiveArchSet).new(self.ppa, self.cell_proc) | ||
70 | 62 | getUtility(IArchiveArchSet).new(self.ubuntu_archive, self.omap) | ||
71 | 63 | result_set = list( | ||
72 | 64 | getUtility(IArchiveArchSet).getRestrictedfamilies(self.ppa)) | ||
73 | 65 | results = dict( | ||
74 | 66 | (row[0].name, row[1] is not None) for row in result_set) | ||
75 | 67 | self.assertEquals( | ||
76 | 68 | { 'arm' : False, 'cell-proc' : True, 'omap' : False}, | ||
77 | 69 | results) | ||
78 | 70 | |||
79 | 71 | def test_get_by_archive_no_other_archives(self): | ||
80 | 72 | """Test ArchiveArchSet.getByArchive returns no other archives.""" | ||
81 | 73 | getUtility(IArchiveArchSet).new(self.ppa, self.cell_proc) | ||
82 | 74 | getUtility(IArchiveArchSet).new(self.ubuntu_archive, self.omap) | ||
83 | 75 | result_set = list( | ||
84 | 76 | getUtility(IArchiveArchSet).getByArchive(self.ppa)) | ||
85 | 77 | self.assertEquals(1, len(result_set)) | ||
86 | 78 | self.assertEquals(self.ppa, result_set[0].archive) | ||
87 | 79 | self.assertEquals(self.cell_proc, result_set[0].processorfamily) | ||
88 | 80 | |||
89 | 55 | 81 | ||
90 | 56 | def test_suite(): | 82 | def test_suite(): |
91 | 57 | return unittest.TestLoader().loadTestsFromName(__name__) | 83 | return unittest.TestLoader().loadTestsFromName(__name__) |
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: soyuz/model/ archivearch. py' soyuz/model/ archivearch. py 2010-02-25 15:25:30 +0000 soyuz/model/ archivearch. py 2010-07-03 23:40:54 +0000 launchpad. webapp. interfaces import ( processorfamily == ProcessorFamily .id)) .archive == archive.id, processorfamily == ProcessorFamily .id))) *origin) .find( y.restricted == True)) soyuz/tests/ test_archivearc h.py' soyuz/tests/ test_archivearc h.py 2010-02-25 15:25:30 +0000 soyuz/tests/ test_archivearc h.py 2010-07-03 23:40:54 +0000 interfaces. distribution import IDistributionSet
> === modified file 'lib/lp/
> --- lib/lp/
> +++ lib/lp/
> @@ -13,7 +13,7 @@
> from canonical.
> 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.
> + And(ArchiveArch
> + ArchiveArch.
> result_set = store.using(
> (ProcessorFamily, ArchiveArch),
> (ProcessorFamil
>
> === modified file 'lib/lp/
> --- lib/lp/
> +++ lib/lp/
> @@ -11,6 +11,7 @@
>
> from lp.testing import TestCaseWithFactory
>
> +from lp.registry.
from canonical. launchpad. interfaces import ILaunchpadCeleb rities
See below.
> from lp.registry. interfaces. person import IPersonSet interfaces. archivearch import IArchiveArchSet interfaces. processor import IProcessorFamilySet veArch, self).setUp() IPersonSet) .getByName( 'cprov' ).archive IDistributionSe t)['ubuntu' ]
> from lp.soyuz.
> from lp.soyuz.
> @@ -24,6 +25,8 @@
> super(TestArchi
>
> self.ppa = getUtility(
> + ubuntu = getUtility(
> + self.ubuntu_archive = ubuntu.main_archive
We have a celebrity that you should use for this.
getUtility( ILaunchpadCeleb rities) .ubuntu
> pss = getUtility( IProcessorFamil ySet) associations( self): restricted_ uassociations( self): (all(aa is None for aa in archivearches)) association( self): restricted_ association( self): IArchiveArchSet ).new(self. ppa, self.cell_proc)
> 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_
> + def test_no_
> """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
>
> - def test_single_
> + def test_single_
> """Our archive is now associated with one of the restricted processor
> families."""
> getUtility(
> @@ -52,6 +55,29 @@
> { 'arm' : False, 'cell-proc' : True, 'omap' : False},
Leading space. ^ Copied&pasted further down ... ;)
> results...