-----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?
> 
> Yeah I can fix that.

Awesome!

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

Great to see we agree. ;)

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

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.

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

> 
> I prefer the non-dictionary approach myself.  The (missing) third arg to 
> assertEqual should be a string that explains the failure.

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.

For example, if result_set contained 2 instead of the expected 1 elements,
you'd want to know what the two contain but this test would not even tell you
what the first contains because the asserts don't get called.

I quite like the idea to have only one assert per test method although I don't
always follow that myself.

> 
>> 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 :(

Well, insert a few newlines to improve it. ;)

> 
> 
> Cheers
> J
> 

Thanks for all the fixes!

Although we don't agree on all points, I don't see a blocker for landing this.
We should discuss and find a solution to our points of disagreement, though.

I just saw lp:~julian-edwards/launchpad/archive-arch-fix. Can you add an MP to
it, that I can approve? You will need that to request r-c, won't you? And I
have to ask you to pleaes revert revision 11096 on that branch. :-P

 review approve code

Cheers,
Henning
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkwxxOIACgkQBT3oW1L17iihOwCgvP5WB9mbVFVNhLoD0lqWdPV6
AKkAn0n23JPvfAWJnlSE+dJyWWnVtCv9
=O0Fv
-----END PGP SIGNATURE-----