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.

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

You should also bring that up at a reviewers' meeting!

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

It's not as easy as that.  The assertion doesn't read back to me well at all 
in terms of what it's asserting.

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

No, I did it on the existing MP.

> And I have to ask you to pleaes revert revision 11096 on that branch. :-P

Thanks Henning.  I've reverted that revision :)  I still disagree with it 
though :(

J