ReviewerMeeting20100707

Not logged in - Log In / Register

ReviewerMeeting20100707

summary

logs

ameu

[14:59] <abentley> me
[15:00] <bac> #startmeeting
[15:00] <MootBot> Meeting started at 09:00. The chair is bac.
[15:00] <MootBot> Commands Available: [TOPIC], [IDEA], [ACTION], [AGREED], [LINK], [VOTE]
[15:00] <bac> hi, whose here?
[15:00] <bigjools> me
[15:00] <sinzui> me
[15:00] <gmb> me
[15:00] <jtv> me
[15:00] <bigjools> who's here, too :)
[15:00] <bac> :)
[15:00] <deryck> me
[15:00] <jtv> nice cascade we had there
[15:00] <mars> me
[15:00] <EdwinGrubbs> me
[15:00] <bac> bigjools: no, i found a very nice "here" and was wondering who'd claim it
[15:01] <gary_poster> me
[15:01] <henninge> me
[15:01] <gary_poster> I'll take it
[15:01] <noodles775> me
[15:01] <gary_poster> well, it's probably not mine
[15:02] <bac> well, let's get started then
[15:02] <adeuring> me
[15:02] <bac> not much on the agenda so we should zip right through
[15:02] <bac>  * Roll call
[15:02] <bac>  * Agenda
[15:02] <bac>  * Outstanding actions
[15:02] <bac>  * New topics
[15:02] <bac>    * PQM commit messages: I don't f***ing care what you're currently listening to.  See also https://dev.launchpad.net/PQMCommitMessages [bigjools]
[15:02] <bac>  * Peanut gallery
[15:02] <bac> * jml(?) to set a policy on what can live in lib/lp, lib/services, and lib/coop
[15:03] <bac> since bjornt left us, this item was assigned to jml...but i forgot to tell him about it, so clearly nothing was done.
[15:03] <bac> my bad
[15:03] <flacoste> me
[15:03] <bac> sinzui: how goes your lint replacement?  good, no?
[15:03] <sinzui> lint is queued for landing
[15:03] <bac> \o/
[15:04] <sinzui> I will send an email announcing this today
[15:04] <sinzui> + instructions about embedding it in your editor
[15:04] <bac> very nice, thank you.
[15:05] <bac> as to new items we only have one
[15:05] <bac> [topic] PQM commit messages: I don't f***ing care what you're currently listening to.  See also https://dev.launchpad.net/PQMCommitMessages [bigjools]
[15:05] <MootBot> New Topic:  PQM commit messages: I don't f***ing care what you're currently listening to.  See also https://dev.launchpad.net/PQMCommitMessages [bigjools]
[15:05] <bac> bigjools: the floor is yours.
[15:05] <bigjools> well first, I was in a bad mood when I added that topic title, but I think you get the gist
[15:06] <bigjools> the commit messages should follow the style guide
[15:06] <bigjools> EOT
[15:06] <sinzui> okay
[15:06]  * sinzui stops work to change editor
[15:06] <bac> any other thoughts?
[15:07] <henninge> Is that spec up to date?
[15:07] <henninge> e.g. Include bugs fixed in this landing. For each bug, include the bug number and the bug summary next to it. e.g. "Fixes: bug 12345: Mozilla Firefox lacks SVG support; bug 54321: Lack of spacing in new account form."
[15:07] <ubot5> Launchpad bug 12345 in isdnutils (Ubuntu) "isdn does not work, fritz avm (pnp?) (affected: 0, heat: 3)" [Medium,Fix released] https://launchpad.net/bugs/12345
[15:07] <ubot5> Launchpad bug 54321 in zope-archetypes (Ubuntu) "Please sync zope-archetypes 1.3.9-2 from Debian unstable (affected: 0, heat: 2)" [Undecided,Fix released] https://launchpad.net/bugs/54321
[15:08] <henninge> I never do that, I thought that's what [bug=xxx] is for
[15:08] <jtv> same here
[15:08] <bac> henninge: you are correct
[15:08] <bac> the guide is out of date
[15:08] <abentley> henninge, agreed.  ec2 land and lp-land use your syntax.
[15:08] <sinzui> bigjools, fixed
[15:08] <bac> i'll fix it this week
[15:09] <bac> thanks sinzui
[15:09] <bigjools> sinzui: sorry, I didn't meant to pick on you specifically.  I've just seen some poor commit messages lately.
[15:09] <bac> [action] bac to update https://dev.launchpad.net/PQMCommitMessages
[15:09] <MootBot> ACTION received:  bac to update https://dev.launchpad.net/PQMCommitMessages
[15:10] <sinzui> bigjools, I agree about the poor commit messages...I brought the issue up with Urshina when I was RM. There were 30 bugs un QAed and I could not say what they changed.
[15:10] <gary_poster> we're working on that and will have a report at epic
[15:11] <bigjools> totally
[15:11] <bigjools> oh cool
[15:11] <abentley> merge proposals have a slot for the commit message.  Maybe we should encourage people to fill it out as part of the review.
[15:11] <bac> [topic] other stuff
[15:11] <MootBot> New Topic:  other stuff
[15:11] <bac> abentley: +1
[15:11] <bigjools> +1
[15:11] <bac> anything else to discuss today
[15:11] <bac> ?
[15:12] <gary_poster> benji is a new launchpadder
[15:12] <gary_poster> he's doing new launchpaddy things now
[15:12] <gary_poster> but he will be asking for reviews
[15:12] <bac> great gary_poster
[15:12] <gary_poster> and will eventually want to go through the mentat process
[15:12] <henninge> I had a few issues come up in a recent review, let me pick one.
[15:12] <bac> everyone should make an effort to say hi to benji this week
[15:13] <gary_poster> +1, was hoping I could get him at this meeting
[15:13] <henninge> Anybody ever heard a guideline "one assert per test" in functional test cases?
[15:14] <bac> i have not
[15:14] <abentley> henninge, no.
[15:14] <henninge> The problem with multiple asserts is that the test will abort and the remaing asserts will not be executed.
=== Ursinha is now known as Ursinha-brb
[15:14] <abentley> henninge, the problem with one assert per test is that set-up time increases.
[15:14] <abentley> henninge, not to mention much more finger-typing.
[15:15] <henninge> abentley: it's either that or smarter asserts.
[15:15] <bac> and probably a less readable test
[15:15] <gmb> henninge, I'm not clear on how the abort is really a problem. For a start, it means you get to fix one test failure at a time rather than having to dig through many.
[15:15] <abentley> henninge, what's the disadvantage of having the test abort and not executing the remaining asserts?
[15:16] <henninge> hang on
[15:16] <henninge> Code like this:
[15:16] <henninge> > +        self.assertEquals(1, len(result_set))
[15:16] <henninge> > >>> +        self.assertEquals(self.ppa, result_set[0].archive)
[15:16] <henninge> > >>> +        self.assertEquals(self.cell_proc, result_set[0].processorfamily)
[15:16] <henninge> >>
[15:17] <henninge> You can test all three conditions at the same time. You can see how many rows
[15:17] <henninge> were returned instead and what their values are. Much more useful.
[15:17] <henninge> The problem with multiple assert statements in one test method is that not all
[15:17] <henninge> will be executed if one fails and you are missing valuable information that
[15:17] <henninge> you can only gain by changing the test code and running the test again.
[15:17] <henninge> For example, if result_set contained 2 instead of the expected 1 elements,
[15:17] <abentley> And how often would the test have failed anyway, because the remaining asserts assume the first assert passed?
[15:17] <henninge> you'd want to know what the two contain but this test would not even tell you
[15:17] <henninge> what the first contains because the asserts don't get called.
[15:17] <bigjools> abentley: point
[15:18] <abentley> henninge, so in your example, what if len==0?
[15:18] <henninge> abentley: sure, we should just make sure that is really the case. The dependency i mean.
[15:18] <henninge> The example is missing my proposed solution which would be comparing lists.
[15:18] <abentley> henninge, and since you're getting results you're not expecting, how do you know that result_set[0] is testing the right thing?
[15:18] <henninge> if len == 0, the list would be empty.
[15:19] <adeuring> you can have a test for "method X changes A into B". The test may need something like "assert(A); do_something(); assert(B)".  "assert(A)" may not be strictly needed, but it can help to ensure that you have a proper test setup.
[15:19] <abentley> henninge, right, and the rest of the asserts will raise IndexError.  Which seems like meaningless exceptions to me.
[15:20] <henninge> My suggestion:
[15:20] <henninge> result = [(row.archive, row.processorfamily) for row in result_set]
[15:20] <henninge> self.assertEqual([(self.ppa, self.cell_proc)], result)
[15:20] <henninge> From the review^
[15:20] <henninge> abentley: there are no other asserts to fail.
[15:21] <abentley> I thought self.assertEquals(self.ppa, result_set[0].archive) is an assert that would fail
[15:22] <henninge> abentley: with len== 0? yes
[15:22] <bac> henninge: i like your suggestion in this instance.  i don't think we'd want to generalize to a restriction of one assert per test case as other situations may not lend themselves so nicely.
[15:22] <abentley> henninge, If we followed a "one assert per test" rule, we might have the same test three times, with the different asserts.
[15:22] <henninge> I was not proposing a rule
[15:23] <bac> ok, i see you said 'guideline'.
[15:23] <henninge> just to have people think about smart asserts.
[15:23] <abentley> henninge, sorry, *guideline*
[15:23] <abentley> henninge, I like your solution.
[15:23] <abentley> henninge, but if you hadn't thought of it, I don't think applying the guideline would make sense.
[15:23] <sinzui> I recall the reason for "one assert per test" was not 10 asserts in a test, but that the test was actually several tests.
[15:24] <abentley> henninge, because it would multiply the number of tests and failures in ways that are not useful.
[15:24] <henninge> just make sure that functional tests suffer not from similar dependency problems like doctest might do.
[15:24] <sinzui> When an object is transitioned to a new state, it may be legitimate to very 3 aspects of the state, but not to then do another state change
[15:24] <sinzui> We had several tests that did that a few years ago
[15:25] <henninge> sinzui: jtv and I just looked at one this morning ... ;)
[15:25] <henninge> anyway, I have made my point.
[15:25] <jtv> henninge: I think that one did go through multiple state changes
[15:25] <jtv> ...with separate assertions
[15:25] <abentley> sinzui, I'm very much in favour of "test only one thing" as a guideline.
[15:26] <sinzui> abentley, I have read your tests, and I know you are
[15:26] <henninge> I had it mentioned to me in a review when I first started and just wanted to check if that is the general opinion.
[15:26] <bigjools> the exception to that is where the setup is very expensive
[15:26] <henninge> jtv: right.
[15:27] <abentley> bigjools, though often those are integration tests, so what you're testing is the integration, not the units.
[15:28] <bigjools> possibly
[15:28] <abentley> bigjools, YMMV
[15:28] <bac> henninge: thanks for bringing this up.  i think reducing the asserts, as you did in your example, is a good idea where practical.
[15:28] <jtv> Another form of testing-too-many-things is where the test sets up a large set of data, and then either performs the operation once and checks all the outputs; or several times with separate checks but without a clear reason why the output should be as expected.
[15:28] <bigjools> abentley: especially in Soyuz :)
[15:29] <bac> any other topics?
[15:29] <bac> 3
[15:29] <bac> 2
[15:29] <bac> 1
[15:29] <bac> thanks everyone
[15:29] <bigjools> thanks
[15:29] <bac> #endmeeting
[15:29] <MootBot> Meeting finished at 09:29.
[15:30] <bac> go read http://elliotmurphy.com/2010/07/06/teambuilding-and-culture-in-a-distributed-workplace/ for a canonical warm-n-fuzzy
[15:47] <bigjools> bac: that blog post is awesome, I read it yesterday and had a huge smile on my face

asiapac

22:33] <bac> thumper, rockstar: reviewer meeting?
[22:34] <thumper> bac: rockstar just went for a walk
[22:34] <bac> thumper: ok.  we didn't get up to much in the earlier meeting.
[22:34] <bac> thumper: main item was bigjools requesting that we follow the PQM submit message guidelines
[22:35] <bac> thumper: by that he meant cut out extraneous stuff like what music is playing, etc.
[22:35] <bac> everyone agreed
[22:35] <bac> henninge brought up the topic of a single assert per test case in functional tests which led to a pretty long discussion
[22:36] <bac> he was concerned that after the first assertion fails the test stops and you don't see the results of the later assertions
[22:36] <thumper> hmm
[22:36] <bac> we agreed that in the cases where you can do something clever to avoid multiple assertions then that was ok but we didn't want to codify such a thing
[22:36] <thumper> personally I don't care about the commit message thing
[22:37] <thumper> I'm happy to go either way
[22:37] <bac> henning agreed it should be a guildeline
[22:37] <bac> me too
[22:37] <thumper> I don't like the single assert in a test thing
[22:37] <thumper> if you want to see the others, I think there is a flag
[22:37] <bac> really?
[22:38]  * thumper thinks
[22:38] <thumper> maybe?
[22:38] <bac> dunno
[22:38] <thumper> lifeless would probably know
[22:38] <thumper> if there isn't one, we could add one
[22:38] <thumper> it isn't rocket science
[22:38] <bac> i'll look into it
[22:38] <thumper> ta
[22:39] <thumper> WRT unit tests
[22:39] <bac> often if an earlier assert dies then those following don't have much chance
[22:39] <bigjools> the commit message thing was more about writing something that actually explained the fix properly
[22:39] <thumper> I would say that the test should be short
[22:39] <thumper> and primarily test one thing
[22:39] <thumper> but that one thing may well need multiple asserts
[22:40] <bac> thumper: yes, you're channeling the gist of our discussion
[22:40] <thumper> ok
[22:40] <thumper> we're on the the same page then
[22:40] <bac> yeppers
=== salgado is now known as salgado-afk
[22:41] <bac> that was it.
[22:41] <bac> thumper: you have anything to pass on?
[22:41] <thumper> ok, I guess we are done then
[22:41] <bac> right.  have a good day.
[22:41] <thumper> not, not really
[22:41] <thumper> see you next week

ReviewerMeeting20100707 (last edited 2010-08-04 13:30:59 by bac)