[01:16] https://code.launchpad.net/~thumper/launchpad/moar-request-build-logging/+merge/45803 [03:25] thumper: There is no Tuesday morning APAC OCR [03:25] StevenK: oh well [03:25] Which is a bug, and needs to be fixed. [03:25] no rush tehn === mrevell__ is now known as mrevell === allenap changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [StevenK, allenap] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [10:22] adeuring: Would you be able to review a mostly straightforward branch for a new view? https://code.launchpad.net/~allenap/launchpad/sub-search-ui-bug-656823-4/+merge/45824 [10:22] allenap: sure [10:22] adeuring: Thanks. === mrevell_ is now known as mrevell [11:18] allenap: r=me === adeuring changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [11:18] adeuring: Thank you :) === henninge changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [11:33] adeuring: I will do StevenK's, I already looked at it yesterday. [11:34] just in case you were going to pick it up ... [11:34] henninge: cool, thanks === mrevell_ is now known as mrevel [14:19] hi allenap, another go? https://code.edge.launchpad.net/~bac/launchpad/bug-5927/+merge/45403 [14:20] bac: Yeah :) === matsubara is now known as matsubara-lunch [14:54] mrevell: can you do a UI review of https://code.launchpad.net/~thumper/launchpad/show-recipe-upload-issue/+merge/45804 [15:24] leonardr, mars: is one of you oncall? [15:24] EdwinGrubbs, I will be === mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [15:24] just tied up some loose ends [15:25] mars: can you review this branch when you get a chance? https://code.launchpad.net/~edwin-grubbs/launchpad/bug-664828-teammembership-delete-timeout/+merge/45805 === EdwinGrubbs changed the topic of #launchpad-reviews to: On call: mars || reviewing: - || queue: [Edwin] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [15:25] EdwinGrubbs, sure === matsubara-lunch is now known as matsubara [16:08] henninge: One assert per test makes me very very sad. [16:09] henninge: But I'd argue that each test only touches one concept [16:09] StevenK: I said I am not insisting on that. [16:09] ;) [16:09] henninge: Right, but it still makes me sad. :-) [16:09] but breaking them further down would be better. [16:10] :-( [16:10] Sorry to make you sad :( [16:10] Like I say, one concept per test [16:11] StevenK: the problem with many asserts in one test is that not all asserts get executed. [16:11] StevenK: so, when tests fail, you have to do the "fix-rerrun-fixnext" loop. [16:12] Well, yeah. I've had this fun while writing them. [16:14] henninge: But if you look at say, line 693 of the diff -- I seriously have to split those nine asserts into seperate tests? Which blows out the line count from 20 lines to 70? :-( [16:15] i don't think that sort of thing is the problem [16:16] the next test, starting at 714 in the diff is worse, because it's setup, exercise, verify, exercise some more and then verify again [16:16] oh actually no it's not [16:16] * mwhudson takes his oar out again [16:24] mwhudson: I agree that what you describe is the worst case but I did not find that here, either. [16:24] StevenK: did you consider writing a testtools matcher to make these assertions easier to read? [16:25] StevenK: *I would* split them up, also because that would be easier to read. I am not fussed about line count here. [16:31] I am, because I have to write it. You just have to read it. :-) [16:35] :-) [16:35] StevenK: I see you already did a lot of what I asked. Thanks. ;) [16:36] henninge: Have I done enough, though? [16:36] StevenK: is that a stray self.setUpBuilds() at the end of thta test file? [16:36] henninge: Er, yes. Well-spotted [16:37] henninge: Fixed locally [16:39] StevenK: thanks. r=me ;) [16:40] henninge: Thanks, let me push that fix === bigjools-afk is now known as bigjools [16:54] hi mars! Can I chuck one on your queue? [16:54] jtv, sure [16:55] mars: ftr it's https://code.launchpad.net/~jtv/launchpad/bug-611217/+merge/45883 === jtv changed the topic of #launchpad-reviews to: On call: mars || reviewing: - || queue: [Edwin, jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [16:55] thanks === benji is now known as benji-lunch === jtv is now known as jtv-afk === gary_poster is now known as gary-lunch === mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: - || queue: [Edwin, jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: EdwinGrubbs || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === gary-lunch is now known as gary_poster === mars changed the topic of #launchpad-reviews to: On call: mars || reviewing: jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === matsubara is now known as matsubara-afk [20:50] mars: I've responded to your review === mars changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [23:41] anyone available to review https://code.launchpad.net/~jcsackett/launchpad/shortlist-696913/+merge/45938?