=== Edwin-afk2 is now known as EdwinGrubbs === adeuring changed the topic of #launchpad-reviews to: On call: adeuring || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [12:10] adeuring: Hi! Are you ready to do a review? ;) [12:10] It will only be a minute. [12:10] ... until I have the mp ready [12:11] henninge: hrmmm, if it's not too long. Actually, I'm again struggling with §$"&§&§/ private librarian files... [12:12] adeuring: It's actually a fairly mechanical change, so it should be easy. [12:12] henninge: ok, where is the MP? [12:12] in a minute, still fighting lint ;) [12:16] adeuring: https://code.launchpad.net/~henninge/launchpad/db-devel-688479-kill-ut-in-tests-1/+merge/43342 [12:17] * adeuring is looking [12:34] henninge: overall, the changes look good. But the disabled tests conern me a bit: Couldn't you simply use the old factory method makeTranslationMessage() unitl the new method can be properly used in these tests? [12:34] s/conern/concern/ [12:52] adeuring: yes, I could. OTHOH that bug will be the next to fix after this bug and I want to remove the old factory method in the next branch for this bug. [12:52] adeuring: sorry for being afk, btw. [12:54] henninge: well, you can fix the factory method first and then change the tests. I am perhaps a bit paranoid, but I remeber a somewhat weird bug in Zope 2 which looked like somebody made a short coffee break while working on a feature but never properly finished that feature aufter the coffee break ;) [12:55] ;-) [12:55] The factory method will not be fixed but removed. [12:56] henninge: right, what I meant is: I think you should still use the old mehtod until the new one works properly for the currently disabled tests [12:56] adeuring: we keep running into these hen-egg problems with this branch. ;) [12:57] In this case it's not the factory that needs to be fixed but the method under test is still referring to the old model. [12:58] adeuring: but I can do that, if you insist ;-) [12:58] np [12:58] I'll have to keep the old factory around a little longer. [12:58] henninge: I would feel defeinetly better if all tests stay enabled ;) [12:59] adeuring: It is important that you feel well! [12:59] ;-) [12:59] ;) [12:59] henninge: ok, so, r=me [12:59] adeuring: thanks === bac changed the topic of #launchpad-reviews to: On call: adeuring, bac || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [14:12] hi adeuring. busy today? === bac changed the topic of #launchpad-reviews to: On call: adeuring, bac || Reviewing: -,- || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [14:13] bac: well, I must admit that I focused more on understanding the cause of a bad branch I committed yesterday... [14:13] adeuring: a worthy goal === mrevell_ is now known as mrevell [14:17] i've got one to throw on the queue, adeuring and bac: https://code.edge.launchpad.net/~jcsackett/launchpad/membership-status-matters-676494/+merge/43303 === jcsackett changed the topic of #launchpad-reviews to: On call: adeuring, bac || Reviewing: -,- || queue: [jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [14:17] jcsackett: i'll take it [14:17] bac: it's the stopgap for the TP stuff. [14:17] thanks, bac. === bac changed the topic of #launchpad-reviews to: On call: adeuring, bac || Reviewing: -,jcsackett || queue: [jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === matsubara is now known as matsubara-lunch [14:43] jcsackett: your new test runs in 1.27 seconds! dodged that bullet! [14:44] bac: whoo! [14:44] bac: it ran in 1.13 on my machine. you need something faster. :-) [14:46] hmm [14:46] my poor MBP is getting old [14:46] jcsackett: one suggestion: ACTIVE_STATES is now defined in two places. how's about moving that into interfaces/teammembership and fixing the two places that use their own copy? [14:47] bac: good suggestion, i'll do that. [14:47] thanks [14:52] jcsackett: this branch has been referred to as a 'stopgap' but i think these changes needed to be made regardless of what happens in the cleanup TP method [14:52] regardless, r=bac === bac changed the topic of #launchpad-reviews to: On call: adeuring, bac || Reviewing: -,- || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [14:52] bac: i think the long term plan is that the query this branch modifies gets replaces by a single way of cleaning up TP. [14:52] bac, thanks for the review. :-) === deryck is now known as deryck[lunch] === salgado is now known as salgado-brb === benji is now known as benji-lunch === sinzui changed the topic of #launchpad-reviews to: On call: adeuring, bac || Reviewing: -,- || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [17:48] bac: do you have time to review https://code.launchpad.net/~sinzui/launchpad/closed-teams-1/+merge/43375 [17:49] sinzui: of course i do. i was about to grab some lunch though [17:49] fab === adeuring changed the topic of #launchpad-reviews to: On call: bac || Reviewing: -,- || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [17:49] nice weekend everybody === deryck[lunch] is now known as deryck === salgado-brb is now known as salgado === benji-lunch is now known as benji === abentley_ is now known as abentley === matsubara is now known as matsubara-brb [19:53] hi sinzui [19:53] in your MP, for rule 1 you say [19:53] hi bac [19:53] Rule 1 means a closed team cannot become open if it is a member of [19:53] any team [19:54] that is wrong [19:54] ok [19:54] it looks wrong [19:54] any (closed) team? [19:55] closed teams cannot become open if they are a member of another closed team [19:55] I think I discovered that when writing the test...I should have updated the rule in my notes [19:56] that makes more sense [19:56] thanks [19:59] sinzui: on TeamSubPolicyError were you forced to define 'doc' to get around your problem? [19:59] yes [19:59] do you know why it happens? is that explained in 'Invalid'? [19:59] The Invalid class has def: doc(): return self.__doc__ [20:00] That appears to be insane [20:00] i'm not familar with the doc method. what should it do? [20:00] I understand that in the ideal that the cause of type of error is obvious, but then why do they all permit use pass a message? [20:02] bac I really cannot explain the issue well. I cannot think or a good reason why that method (rather than str()) is used to get the description of the issue in the Web UI [20:02] sinzui: odd [20:02] sinzui: i think this test is redundant and should just be 'else:' [20:03] elif policy != TeamSubscriptionPolicy.OPEN: [20:03] you just checked policy == OPEN [20:03] I did see that zope wants unicode, translatable docstrings, so it does funky operations like class.__doc__ = _('my translatable error description') [20:04] bac; I do not think that is right. The else clause covers safe transitions such as from moderated to restricted [20:05] sinzui: at line 95 you test that the policy == OPEN. so the next check that policy != OPEN must always be true [20:07] I think I should add something to the elif. [20:07] yeah, you need to check what you are coming from [20:07] i think 108 should be team.subscriptionpolicy !=OPEN [20:08] elif policy != TeamSubscriptionPolicy.OPEN and team.policy != TeamSubscriptionPolicy.OPEN [20:08] you can eliminate the first part because it is already known to be true [20:08] We only want to check the teams when we are certain that the team is transitioning from closed to open [20:09] 108 should test that it is transitioning to closed from another closed [20:10] okay I understand [20:10] * sinzui tries a rewrite [20:11] no, i'm wrong. 108 should test going from OPEN to closed, so the test should be old == OPEN, since we know the new is one is not OPEN [20:11] urgh. [20:11] so i think you have a coding mistake [20:12] why is there not a test case that is broken? [20:12] elif team.policy != TeamSubscriptionPolicy.OPEN [20:12] team.subscriptionpolicy === matsubara-brb is now known as matsubara [20:15] + elif team.subscriptionpolicy == TeamSubscriptionPolicy.OPEN: [20:15] ^ that makes the tests happy [20:19] sinzui: but the tests were already happy. i think you may be missing a test case [20:20] I do not think so. Do you want a test for moderated to restricted? [20:20] the else that has a pass in it? [20:21] i don't know [20:21] i'm just bummed that you had a logic error but no test failure [20:22] doesn't that indicate a lack of coverage? [20:22] okay. I will add a test from moderated to restricted [20:22] If I had pdb running in that test, I would have found that I never entered the pass block, I looked at the teams [20:22] yeah, i think it'll be an uninteresting test now but would have shown a failure before. [20:23] I want to avoid looking at teams [20:23] you can add the test and then shelve your code fix and see if it fails [20:23] it would not have failed, I was just making db checks that I intended to avoid [20:25] yes you are right [20:25] the error just caused you to do checks that are not needed in that case. there is no way it could fail [20:27] sinzui: r=bac with these changes. [20:27] almost finished the test [20:31] bac: http://pastebin.ubuntu.com/542016/ [20:31] sinzui: looks good === bac 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 === matsubara is now known as matsubara-afk