| jtv | bac, I take it you're off-duty now? | 07:34 |
|---|---|---|
| === jtv changed the topic of #launchpad-reviews to: On call: jtv || reviewing: - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| adeuring | jtv: could you please review this mp: https://code.edge.launchpad.net/~adeuring/launchpad/bug-333521-allow-mail-to-question-subscribers/+merge/23833 ? | 09:45 |
| jtv | adeuring: coming up | 09:48 |
| adeuring | jtv: thanks! | 09:48 |
| === jtv changed the topic of #launchpad-reviews to: On call: jtv || reviewing: adeuring || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| jtv | adeuring: does the test need to do its own login? Isn't it enough to pass the identity's email address to the TestCaseWithFactory constructor? | 09:51 |
| adeuring | jtv: ah, I did not know that | 09:52 |
| jtv | adeuring: better make sure that it's really the same, because you want this test to fail if the db permissions aren't enough. | 09:52 |
| jtv | Next, I don't think you really need to create product_owner explicitly. In the interest of test brevity, why not have factory.makeProduct create an owner and use self.product.owner.email in the test comparison? | 09:54 |
| jtv | Oh, I see you're doing switchDbUser anyway | 09:54 |
| jtv | adeuring: Does this test need Librarian at all? If not, ZopelessDatabaseLayer may be a better layer for your test. Faster to set up | 09:55 |
| adeuring | right, we don't need the librarian. | 09:55 |
| jtv | adeuring: and a definite no-no: don't compare the exact list of notification recipients. Use self.assertContentEqual to eliminate ordering issues. | 09:58 |
| adeuring | jtv: OK, makes sense... | 09:58 |
| jtv | adeuring: what is a "sub" in the context of this test? Not a sandwich? | 09:59 |
| jtv | See the docstring. | 10:00 |
| adeuring | that's plain nonsense. I'll remove it. | 10:00 |
| adeuring | the title should be ""Ensure that question subscribers are notified about bug expiration" | 10:00 |
| adeuring | jtv: can you give me an example where the DB user is passed as a constructor parameter to TestCaseWithFactory? | 10:05 |
| adeuring | jtv: ah, now I see what you meant... | 10:06 |
| adeuring | jtv: I need LPZopelessLayer because of the method switchDBUser() That is not avaliable in ZopelessDatabaseLayer | 10:10 |
| adeuring | jtv: current diff: http://paste.ubuntu.com/419731/ | 10:12 |
| jtv | adeuring: oic | 10:12 |
| === allenap changed the topic of #launchpad-reviews to: On call: jtv, allenap || reviewing: adeuring, - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| allenap | Morning jtv :) | 10:13 |
| jtv | allenap: morning! | 10:13 |
| jtv | adeuring: one thing I don't like about the test is the somewhat arbitrary division between setup and test. I think it'd help a lot of you could gut setUp, and put as much as possible in methods with meaningful names. | 10:17 |
| adeuring | jtv: so... something like a method "createBugProductQuestionAndLinkThem()"? | 10:19 |
| jtv | adeuring: that name reveals the problem—that it's doing too much. | 10:20 |
| adeuring | jtv code in the test method is needed there: make a snapshot of a bugtask, change the bugtask, call notify() for the change | 10:20 |
| jtv | adeuring: that sounds like a more useful breakdown. | 10:21 |
| adeuring | jtv: well... that's what setUp is for, IMHO: create the test data you need to run the tests ;) | 10:21 |
| jtv | adeuring: opinion is divided on that subject. It's easy to create a situation where future generations keep adding tests and depending in different ways on a convoluted setup. | 10:21 |
| adeuring | jtv: well, ok... but this is a a unit test for very special situation. I was suprised that it occured so late... | 10:22 |
| jtv | adeuring: ideally, the test would read as a series of logical steps very similar to the ones you just named. | 10:22 |
| adeuring | so, what about a comment in setUp: "we need a product, a bug and a question linked to the bug in order to..." | 10:23 |
| adeuring | ...perhaps enriched by "we want explicit person names so that we have stable test data just in case that the factory methos cahnge in a funny way" | 10:24 |
| jtv | adeuring: I don't even see why you need explicit person names, frankly. | 10:26 |
| jtv | You need known names, and that's no problem. | 10:26 |
| adeuring | jtv: yes, that's cruft. I'll remove it | 10:26 |
| adeuring | jtv: new diff | 10:31 |
| adeuring | http://paste.ubuntu.com/419745/ | 10:31 |
| jtv | adeuring: btw I do realize this is annoying... imagine me having a Rambo Moment here as I am reminded of experiences with convoluted tests. :) | 10:32 |
| jtv | adeuring: in the comment, I wouldn't say "don't clutter the test data with irrelevant notifications" (which would need a capital letter btw) but just something like "Flush pending notifications for question creation." | 10:33 |
| adeuring | jtv: sure | 10:33 |
| jtv | adeuring: also, I always forget... do you need to commit before switchDbUser? | 10:34 |
| jtv | I guess not, otherwise you wouldn't get the notification, right? | 10:34 |
| adeuring | jtv: right, otherwise the test wouldn't work... | 10:34 |
| jtv | adeuring: I still don't see why you want the fixed email addresses. I think it's actually clearer in the test to say "[product.owner.preferredemail, subscriber.preferredemail]" than to repeat the email address string. | 10:39 |
| adeuring | jtv: ok, no problem... | 10:40 |
| jtv | adeuring: then I think I've made it hard enough for you. :) | 10:41 |
| jtv | adeuring: approved. | 10:42 |
| adeuring | thanks | 10:42 |
| === jtv changed the topic of #launchpad-reviews to: On call: jtv, allenap || reviewing: -, - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === allenap changed the topic of #launchpad-reviews to: On call: jtv, allenap || reviewing: -, jtv || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === allenap changed the topic of #launchpad-reviews to: On call: jtv, allenap || reviewing: -, - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| jtv | allenap: was that a very fast review? | 10:50 |
| allenap | jtv: Yes, it was perfect :) | 10:51 |
| jtv | allenap: thanks! (hope it wasn't perfect in the sense of "the perfect chance to dash someone's hopes before my first coffee" or "at last a review I can type out in two letters using my right hand only") | 10:52 |
| wgrant | jtv: What, 'ok'? :P | 10:55 |
| jtv | wgrant: ooh hadn't thought of _that_ one... | 10:55 |
| jtv | allenap: me be relocatin | 11:37 |
| jtv | so copping out for now, sorry to leave you with this madhouse. :) | 11:37 |
| === jtv changed the topic of #launchpad-reviews to: On call: allenap || reviewing: - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| allenap | jtv: Heh, no worries :) | 11:37 |
| gmb | allenap, Do you have time to review a 931-line branch :D? | 11:39 |
| gmb | allenap, It's the first step to refactoring | 11:39 |
| allenap | gmb: Sure :) | 11:39 |
| === allenap changed the topic of #launchpad-reviews to: On call: allenap || reviewing: gmb || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| gmb | Renaming the existing BugWatchUpdater and moving checkwatches.updater to checkwatches.core. | 11:39 |
| gmb | allenap, https://code.edge.launchpad.net/~gmb/launchpad/cw-refactor-create-rename-bwu-567793/+merge/23842 | 11:39 |
| gmb | allenap, Note, I hate the new name for BugWatchUpdater; feel free to suggest a better one. I just picked it to stop myself bikeshedding | 11:40 |
| gmb | allenap, Your branch is a prerequisite; I don't know if the MP will DTRT now. | 11:40 |
| allenap | gmb: CheckwatchesTopBanana | 11:41 |
| gmb | allenap, How about CheckwatchesYesItWorksNowShutup | 11:42 |
| allenap | gmb: Yes, that'll do it. | 11:43 |
| === henninge_ is now known as henninge | ||
| allenap | gmb: TBH, I think CheckwatchesMaster is okay. | 11:43 |
| gmb | allenap, Fair enough. We can always change it later. | 11:44 |
| === StevenK changed the topic of #launchpad-reviews to: On call: allenap || reviewing: gmb || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| allenap | gmb: My grey matter is now grey blancmange, but r=me. | 11:48 |
| gmb | allenap, Cool. Your desserting is important to us. | 11:48 |
| gmb | Boom | 11:48 |
| gmb | Boom | 11:48 |
| allenap | gmb: Oh god. | 11:48 |
| gmb | My lame joke know no bounds. | 11:49 |
| === allenap changed the topic of #launchpad-reviews to: On call: allenap || reviewing: - || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === allenap changed the topic of #launchpad-reviews to: On call: allenap || reviewing: Edwin || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === allenap changed the topic of #launchpad-reviews to: On call: allenap || reviewing: - || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| allenap | StevenK: Hi, I don't see a merge proposal for you in https://code.edge.launchpad.net/launchpad/+activereviews. Can you point me to it? | 12:38 |
| StevenK | allenap: Certainly https://code.edge.launchpad.net/~stevenk/launchpad/fixes-bug-451396/+merge/21706 | 12:39 |
| allenap | StevenK: Okay, I think I'll ask adeuring to follow up on that. | 12:40 |
| === allenap changed the topic of #launchpad-reviews to: On call: allenap || reviewing: - || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| adeuring | allenap, StevenK: I'll look | 12:42 |
| StevenK | adeuring: Thanks! | 12:48 |
| adiroiban | henninge: Hi, any news about the landing of this branch https://code.edge.launchpad.net/~adiroiban/launchpad/bug-201749/+merge/21250 ? :) | 12:55 |
| adiroiban | also is there anything I need to do to have this branch ready for code review, https://code.edge.launchpad.net/~adiroiban/launchpad/bug-525992/+merge/23285 ? | 12:56 |
| === salgado-afk is now known as salgado | ||
| allenap | wgrant: Shall I land lp:~wgrant/launchpad/bug-565739-dont-retry-superseded-builds for you? | 13:06 |
| adeuring | StevenK: it seems thet problem still remains that the tests pass when the line "if self.isPPA():" is re-introduced in lib/lp/soyuz/model/queue.py . | 13:06 |
| StevenK | adeuring: Right, I'll revert the changes in queue.py and keep checking | 13:12 |
| adiroiban | danilos: hi, when you have some time can we have a preimplementation chat for bug 61081? This should be about deciding a range for POTemplate priority. https://code.edge.launchpad.net/~adiroiban/launchpad/bug-61081/+merge/23811 | 13:12 |
| mup | Bug #61081: PO template +edit form needs better validation for priority field <oops> <Launchpad Translations:In Progress by adiroiban> <https://launchpad.net/bugs/61081> | 13:12 |
| === mrevell is now known as mrevell-lunch | ||
| wgrant | allenap: Yes please. | 13:14 |
| danilos | adiroiban, lookin | 13:14 |
| danilos | looking even :) | 13:14 |
| danilos | adiroiban, fyi, on staging: http://paste.ubuntu.com/419817/ | 13:16 |
| gmb | allenap, I just found another reason not to do relative .foo imports: it makes grep-and-sed fragile :) | 13:19 |
| adiroiban | danilos: shoud I use those limits? Maybe -100 ... 5000000 ? | 13:19 |
| danilos | adiroiban, there is only one potemplate (pyroom) that has this priority, all the others are lower than 10k | 13:20 |
| danilos | adiroiban, as for negative ones, that sounds wrong and we should probably just reset them | 13:20 |
| danilos | adiroiban, 6 templates have negative priorities | 13:21 |
| adiroiban | danilos: what values do you suggest for the priority range? | 13:23 |
| danilos | adiroiban, I guess 0-100000 (i.e. 10x10000 to leave some room there) | 13:23 |
| adiroiban | danilos: ok. we can reset the values... but I suppose that user will be warned when the want to update again the priority and will have to fix it | 13:24 |
| adiroiban | danilos: is there anything on this bug that is worth mentioning or it is ready to code or I should just change the range, write tests and create MP ? | 13:26 |
| danilos | adiroiban, yeah, we can notify them as well | 13:26 |
| danilos | adiroiban, no, it should be ready to code; I'll add a comment to the bug explaining my "findings" | 13:26 |
| bac | hi mrevell-lunch | 13:30 |
| adiroiban | danilos: thanks. do you also have time to give a quick feedback based on some screenshots ? https://code.edge.launchpad.net/~adiroiban/launchpad/bug-146178/+merge/23760 :) | 13:30 |
| bac | mrevell-lunch: i agree with your email wrt line wrapping. keep it as is. | 13:31 |
| bac | adiroiban: thanks for your reply to my review yesterday. everything you said makes perfect sense. sorry for the silly suggestion about using a set. | 13:32 |
| danilos | adiroiban, added a comment to the MP: "Looking at the screenshots: any reason to still add 'Current' to the titles on distroseries:+translations page? And don't forget to decapitalize "Base" in "Current Base" on the +language-packs page (not to mention that it'd be nice to find better terminology for these, but I am letting you take care of that with the UI reviewer)" | 13:33 |
| danilos | adiroiban, and do NOT use tables there | 13:35 |
| danilos | :) | 13:35 |
| adiroiban | danilos: I will remove the "Current" and fix the capitalization and try to find better names | 13:35 |
| danilos | adiroiban, thanks | 13:36 |
| adiroiban | danilos: +language-packs has no table | 13:36 |
| danilos | adiroiban, you added one to distroseries-translations | 13:36 |
| adiroiban | but for the +translations page | 13:36 |
| danilos | adiroiban, right, so don't do that, it's not tabular data | 13:37 |
| adiroiban | I added one since this is what it was suggested durring an UI review for another bug | 13:37 |
| adiroiban | ok | 13:37 |
| danilos | adiroiban, hum, where? | 13:37 |
| bac | adiroiban: if you have no further changes to your branch i'll be happy to land it now. | 13:37 |
| adiroiban | danilos: https://bugs.edge.launchpad.net/rosetta/+bug/367844 ... I added this none on the MP | 13:37 |
| mup | Bug #367844: Language pack page needs ui improvements. <ui> <Launchpad Translations:Triaged> <https://launchpad.net/bugs/367844> | 13:37 |
| adiroiban | note | 13:37 |
| danilos | adiroiban, (wondering what review was that so if needed I can complain and resolve it with the UI team :) | 13:37 |
| adiroiban | danilos: ah... my bad. looks like the bug is talking about a different data | 13:38 |
| adiroiban | .. or not ? not sure :) | 13:39 |
| danilos | adiroiban, that bug is obsolete, for the old version of the page :) | 13:39 |
| adiroiban | bac: the branch should be ready for testing | 13:39 |
| adiroiban | danilos: ok. I will revert the table | 13:39 |
| danilos | adiroiban, thanks | 13:40 |
| bac | adiroiban: great. by 'land' i meant 'ec2 land'...full test and then submit. | 13:41 |
| === mrevell-lunch is now known as mrevell | ||
| mrevell | thanks bac | 13:47 |
| bac | adiroiban: could i get you to resolve the text conflict in your branch and repush? | 14:01 |
| adiroiban | bac: sure. I will do it now and add a commnent on the MP when it is done | 14:01 |
| bac | adiroiban: please ping me here too | 14:02 |
| adiroiban | bac: done :) | 14:09 |
| bac | adiroiban: thank | 14:09 |
| bac | s | 14:09 |
| bac | adiroiban: submitted to ec2. in a short 3.5 - 4 hours it'll be landed. :) | 14:24 |
| jtv | allenap: want me to take that one off the queue? | 15:03 |
| allenap | jtv: Yes please :) | 15:04 |
| allenap | jtv: No, actually, adeuring took that one already. | 15:04 |
| === allenap changed the topic of #launchpad-reviews to: On call: allenap || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| jtv | ah | 15:04 |
| === jelmer is now known as ctrlsoft | ||
| === kfogel is now known as kfogelunch | ||
| === EdwinGrubbs changed the topic of #launchpad-reviews to: On call: allenap, Edwin || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === salgado is now known as salgado-lunch | ||
| === allenap changed the topic of #launchpad-reviews to: On call: Edwin || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === gary_poster is now known as gary-lunch | ||
| adiroiban | bac: hi. I received an email with the subject „Test Runner FAILED” but the traceback does not give any hints about what test went wrong | 17:47 |
| === leonardr is now known as leonardr-lunch | ||
| bac | adiroiban: it was a failure in the ec2 script. | 18:13 |
| bac | adiroiban: the portion that sends email with the test results. so, i'm afraid, that run was wasted | 18:14 |
| === salgado-lunch is now known as salgado | ||
| === kfogelunch is now known as kfogel | ||
| === gary-lunch is now known as gary_poster | ||
| bac | jml: https://code.launchpad.net/~bac/launchpad/bug-567985/+merge/23868 | 18:56 |
| jml | bac: reviewed. | 18:59 |
| === leonardr-lunch is now known as leonardr | ||
| === adiroiban changed the topic of #launchpad-reviews to: On call: Edwin || reviewing: - || queue: [adiroiban(bug-61081)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| bac | adiroiban: ec2 has been fixed and i'm resubmitting your branch | 21:24 |
| adiroiban | bac: :) thanks! | 21:25 |
| === adiroiban changed the topic of #launchpad-reviews to: On call: Edwin || reviewing: - || queue: [adiroiban(UI bug-146178), adiroiban(bug-61081)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === salgado is now known as salgado-afk | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!