| === 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 | ||
| henninge | adeuring: Hi! Are you ready to do a review? ;) | 12:10 |
|---|---|---|
| henninge | It will only be a minute. | 12:10 |
| henninge | ... until I have the mp ready | 12:10 |
| adeuring | henninge: hrmmm, if it's not too long. Actually, I'm again struggling with §$"&§&§/ private librarian files... | 12:11 |
| henninge | adeuring: It's actually a fairly mechanical change, so it should be easy. | 12:12 |
| adeuring | henninge: ok, where is the MP? | 12:12 |
| henninge | in a minute, still fighting lint ;) | 12:12 |
| henninge | adeuring: https://code.launchpad.net/~henninge/launchpad/db-devel-688479-kill-ut-in-tests-1/+merge/43342 | 12:16 |
| * adeuring is looking | 12:17 | |
| adeuring | 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 |
| adeuring | s/conern/concern/ | 12:34 |
| henninge | 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 |
| henninge | adeuring: sorry for being afk, btw. | 12:52 |
| adeuring | 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:54 |
| henninge | ;-) | 12:55 |
| henninge | The factory method will not be fixed but removed. | 12:55 |
| adeuring | 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 |
| henninge | adeuring: we keep running into these hen-egg problems with this branch. ;) | 12:56 |
| henninge | 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:57 |
| henninge | adeuring: but I can do that, if you insist ;-) | 12:58 |
| henninge | np | 12:58 |
| henninge | I'll have to keep the old factory around a little longer. | 12:58 |
| adeuring | henninge: I would feel defeinetly better if all tests stay enabled ;) | 12:58 |
| henninge | adeuring: It is important that you feel well! | 12:59 |
| henninge | ;-) | 12:59 |
| adeuring | ;) | 12:59 |
| adeuring | henninge: ok, so, r=me | 12:59 |
| henninge | adeuring: thanks | 12:59 |
| === 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 | ||
| bac | hi adeuring. busy today? | 14:12 |
| === 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 | ||
| adeuring | bac: well, I must admit that I focused more on understanding the cause of a bad branch I committed yesterday... | 14:13 |
| bac | adeuring: a worthy goal | 14:13 |
| === mrevell_ is now known as mrevell | ||
| jcsackett | 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 | 14:17 |
| === 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 | ||
| bac | jcsackett: i'll take it | 14:17 |
| jcsackett | bac: it's the stopgap for the TP stuff. | 14:17 |
| jcsackett | thanks, bac. | 14:17 |
| === 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 | ||
| bac | jcsackett: your new test runs in 1.27 seconds! dodged that bullet! | 14:43 |
| jcsackett | bac: whoo! | 14:44 |
| jcsackett | bac: it ran in 1.13 on my machine. you need something faster. :-) | 14:44 |
| bac | hmm | 14:46 |
| bac | my poor MBP is getting old | 14:46 |
| bac | 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:46 |
| jcsackett | bac: good suggestion, i'll do that. | 14:47 |
| bac | thanks | 14:47 |
| bac | 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 |
| bac | regardless, r=bac | 14:52 |
| === 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 | ||
| jcsackett | 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 |
| jcsackett | bac, thanks for the review. :-) | 14:52 |
| === 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 | ||
| sinzui | bac: do you have time to review https://code.launchpad.net/~sinzui/launchpad/closed-teams-1/+merge/43375 | 17:48 |
| bac | sinzui: of course i do. i was about to grab some lunch though | 17:49 |
| sinzui | fab | 17:49 |
| === 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 | ||
| adeuring | nice weekend everybody | 17:49 |
| === 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 | ||
| bac | hi sinzui | 19:53 |
| bac | in your MP, for rule 1 you say | 19:53 |
| sinzui | hi bac | 19:53 |
| bac | Rule 1 means a closed team cannot become open if it is a member of | 19:53 |
| bac | any team | 19:53 |
| sinzui | that is wrong | 19:54 |
| bac | ok | 19:54 |
| bac | it looks wrong | 19:54 |
| bac | any (closed) team? | 19:54 |
| sinzui | closed teams cannot become open if they are a member of another closed team | 19:55 |
| sinzui | I think I discovered that when writing the test...I should have updated the rule in my notes | 19:55 |
| bac | that makes more sense | 19:56 |
| bac | thanks | 19:56 |
| bac | sinzui: on TeamSubPolicyError were you forced to define 'doc' to get around your problem? | 19:59 |
| sinzui | yes | 19:59 |
| bac | do you know why it happens? is that explained in 'Invalid'? | 19:59 |
| sinzui | The Invalid class has def: doc(): return self.__doc__ | 19:59 |
| sinzui | That appears to be insane | 20:00 |
| bac | i'm not familar with the doc method. what should it do? | 20:00 |
| sinzui | 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:00 |
| sinzui | 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 |
| bac | sinzui: odd | 20:02 |
| bac | sinzui: i think this test is redundant and should just be 'else:' | 20:02 |
| bac | elif policy != TeamSubscriptionPolicy.OPEN: | 20:03 |
| bac | you just checked policy == OPEN | 20:03 |
| sinzui | I did see that zope wants unicode, translatable docstrings, so it does funky operations like class.__doc__ = _('my translatable error description') | 20:03 |
| sinzui | bac; I do not think that is right. The else clause covers safe transitions such as from moderated to restricted | 20:04 |
| bac | sinzui: at line 95 you test that the policy == OPEN. so the next check that policy != OPEN must always be true | 20:05 |
| sinzui | I think I should add something to the elif. | 20:07 |
| bac | yeah, you need to check what you are coming from | 20:07 |
| bac | i think 108 should be team.subscriptionpolicy !=OPEN | 20:07 |
| sinzui | elif policy != TeamSubscriptionPolicy.OPEN and team.policy != TeamSubscriptionPolicy.OPEN | 20:08 |
| bac | you can eliminate the first part because it is already known to be true | 20:08 |
| sinzui | We only want to check the teams when we are certain that the team is transitioning from closed to open | 20:08 |
| bac | 108 should test that it is transitioning to closed from another closed | 20:09 |
| sinzui | okay I understand | 20:10 |
| * sinzui tries a rewrite | 20:10 | |
| bac | 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 |
| bac | urgh. | 20:11 |
| bac | so i think you have a coding mistake | 20:11 |
| bac | why is there not a test case that is broken? | 20:12 |
| sinzui | elif team.policy != TeamSubscriptionPolicy.OPEN | 20:12 |
| bac | team.subscriptionpolicy | 20:12 |
| === matsubara-brb is now known as matsubara | ||
| sinzui | + elif team.subscriptionpolicy == TeamSubscriptionPolicy.OPEN: | 20:15 |
| sinzui | ^ that makes the tests happy | 20:15 |
| bac | sinzui: but the tests were already happy. i think you may be missing a test case | 20:19 |
| sinzui | I do not think so. Do you want a test for moderated to restricted? | 20:20 |
| sinzui | the else that has a pass in it? | 20:20 |
| bac | i don't know | 20:21 |
| bac | i'm just bummed that you had a logic error but no test failure | 20:21 |
| bac | doesn't that indicate a lack of coverage? | 20:22 |
| sinzui | okay. I will add a test from moderated to restricted | 20:22 |
| sinzui | 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 |
| bac | yeah, i think it'll be an uninteresting test now but would have shown a failure before. | 20:22 |
| sinzui | I want to avoid looking at teams | 20:23 |
| bac | you can add the test and then shelve your code fix and see if it fails | 20:23 |
| sinzui | it would not have failed, I was just making db checks that I intended to avoid | 20:23 |
| bac | yes you are right | 20:25 |
| bac | the error just caused you to do checks that are not needed in that case. there is no way it could fail | 20:25 |
| bac | sinzui: r=bac with these changes. | 20:27 |
| sinzui | almost finished the test | 20:27 |
| sinzui | bac: http://pastebin.ubuntu.com/542016/ | 20:31 |
| bac | sinzui: looks good | 20:31 |
| === 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 | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!