| === danilo_ is now known as danilos | ||
| === danilo_ is now known as danilos | ||
| === mrevell is now known as mrevell-lunch | ||
| === mrevell-lunch is now known as mrevell | ||
| === salgado is now known as salgado-lunch | ||
| 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 | welcome to the AMEU Reviewers meeting. | 15:00 |
| bac | who's here today? | 15:01 |
| flacoste | me | 15:01 |
| sinzui | me | 15:01 |
| henninge | me | 15:01 |
| mars | moo | 15:01 |
| adeuring | me | 15:01 |
| bac | EdwinGrubbs, deryck, gmb, BjornT: ping | 15:02 |
| EdwinGrubbs | me | 15:02 |
| gmb | me | 15:02 |
| gmb | Erk | 15:02 |
| deryck | me | 15:02 |
| bac | gary_poster: ping | 15:02 |
| gary_poster | bac: thank you, me | 15:02 |
| bac | team leads ping your peeps | 15:03 |
| gary_poster | done. May not get too many from my team. | 15:03 |
| danilos | me | 15:03 |
| bac | i know we have a lot of people missing today due to sprints, so let's move on. | 15:04 |
| henninge | bac: jtv is sprinting | 15:04 |
| bac | [topic] agenda | 15:04 |
| MootBot | New Topic: agenda | 15:04 |
| bac | yay, mootbot is not case sensitive! | 15:04 |
| gary_poster | heh | 15:04 |
| bac | * Roll call | 15:04 |
| bac | * Action items | 15:04 |
| bac | * Limit doctests to 200 lines [allenap] | 15:04 |
| bac | * Security checks: ``try:...except UnauthorizedError:...`` vs. canView [gary] | 15:04 |
| bac | * Cleaning up ImportFascist warnings [bac,sinzui] | 15:04 |
| bac | * Peanut gallery (anything not on the agenda) | 15:04 |
| allenap | me | 15:04 |
| intellectronica | me | 15:04 |
| danilos | allenap, (a nice trick to get pinged through agenda ;) | 15:05 |
| bac | [topic] outstanding actions | 15:05 |
| MootBot | New Topic: outstanding actions | 15:05 |
| bac | [topic] * intellectronica to draft guidelines for drive-by cleanups | 15:05 |
| MootBot | New Topic: * intellectronica to draft guidelines for drive-by cleanups | 15:05 |
| intellectronica | sorry, haven't done that. please keep it on the list and i'll get it for our next meeting | 15:06 |
| bac | intellectronica: excellent, thanks! | 15:06 |
| bac | [topic] invite other teams to do lazr-js code reviews? (See if this was sent to the ML.) [mars/bac] | 15:06 |
| MootBot | New Topic: invite other teams to do lazr-js code reviews? (See if this was sent to the ML.) [mars/bac] | 15:06 |
| bac | i searched through the ML's and did not see it had been discussed | 15:06 |
| bac | mars did you have some thoughts on the topic? | 15:06 |
| mars | bac, I'm going to add it to the agenda for the lazr-js taskforce call tomorrow | 15:07 |
| bac | mars: ok, so you can report back here next week. | 15:07 |
| mars | bac, you can keep it on the list here if you would like to see a wrapup item next week | 15:07 |
| bac | [action] mars to report on outcome of lazr-js review discussion | 15:07 |
| MootBot | ACTION received: mars to report on outcome of lazr-js review discussion | 15:07 |
| mars | yeah :) | 15:07 |
| bac | great, moving on to new items | 15:07 |
| bac | [topic] Limit doctests to 200 lines [allenap] | 15:08 |
| MootBot | New Topic: Limit doctests to 200 lines [allenap] | 15:08 |
| bac | gavin, take it away | 15:08 |
| allenap | Right, | 15:09 |
| allenap | Because of the accumulation of state in doctests | 15:09 |
| allenap | I think it would be good to limit new doctests to 200 lines or less. | 15:09 |
| allenap | gmb wrote a new doctest last week of just under 200 lines and it was still perfectly comprehensible. | 15:10 |
| allenap | So that's where the number comes from. | 15:10 |
| * gmb notes that it got changed into a unittest anyway during the review process... | 15:10 | |
| danilos | I don't think that'd be right for large classes and corresponding doctests, 200 sounds as artificially low limit | 15:10 |
| allenap | Although manuel (?) adds a reset-globs directive whihc might make this moot. | 15:11 |
| flacoste | and there is an alternative to the state problem | 15:11 |
| danilos | perhaps large classes are the problem, but we do have them | 15:11 |
| flacoste | exactly, reset-globs in manuel | 15:11 |
| gary_poster | agree with danilos. Also, agree with allenap's observation of manuel | 15:11 |
| sinzui | allenap: I have thought about breaking many registry doctests into smaller themes | 15:11 |
| flacoste | manuel also adds the option of running specific section of a doctest | 15:11 |
| sinzui | allenap: but my motivation it to control the layer the test is run on. | 15:11 |
| intellectronica | i think limiting the length is not the best way to handle this. instead i suggest dividing tests thematically | 15:12 |
| mars | I agree, I don't see the problem as much with larger narratives as it is with naughty test code state. | 15:12 |
| danilos | sinzui, I think the problem with existing doctests is that we don't have a good structure of tests in general (oh, I'd like to clean most of translations ones as well) | 15:12 |
| intellectronica | that is, stop and start a new file when you're really testing something new (with new state and so on) | 15:12 |
| bac | intellectronica: i agree | 15:12 |
| allenap | The main problem I have is trying to understand the object and database state by the end of the test. That can't really be fixed by reset-globs or manuel as I understand it. | 15:13 |
| danilos | intellectronica, +1, basic doctests (those which are usually large) should be basic class documentation; specific tests should be unit test | 15:13 |
| allenap | intellectronica: The limit is an incentive to split :) | 15:13 |
| allenap | intellectronica: But I agree. | 15:14 |
| danilos | allenap, I see the point, but artificial limit would have to be broken in so many valid cases | 15:14 |
| intellectronica | allenap: yes, i understand that, but somehow it feels a bit too artificial to me. are you concerned that without a hard limit developers/reviewers won't bother? | 15:14 |
| bac | allenap: it is a good point but i'd rather see us have smaller, more themed tests as a guide rather than adopting a limit | 15:14 |
| gary_poster | We do have a "800 line limit for reviews" that is a guideline but often breached | 15:14 |
| intellectronica | also, i'll play sinzui and ask: who's going to split all the existing test? ;) | 15:15 |
| danilos | how about instead spreading a rule "enforce corner-case testing through unit tests" among reviewers instead? those tend to make doctests harder to read | 15:15 |
| mars | instead of a hard limit, can we call it a "refactor point"? As in, over X lines, the reviewer should look for a possible refactoring | 15:15 |
| allenap | Okay, sounds good. Is there a way to measure or have more concrete guidelines, or is it at developer/reviewer discretion? | 15:15 |
| mars | it's ok if they do not find such a point | 15:15 |
| mars | kind of like review lint | 15:15 |
| intellectronica | allenap: there is a measure. variables shouldn't be recycled | 15:16 |
| danilos | mars, I still believe that those complex doctests should really be turned into unittests, and that's exactly where you can't reuse variables :) | 15:16 |
| allenap | intellectronica: I don't think we should split the existing tests... until it is no longer possible to not split them. | 15:16 |
| allenap | intellectronica: I like that :) | 15:16 |
| allenap | intellectronica: There's still a problem of db state. | 15:16 |
| mars | danilos, well, if they are over 800 lines, then you would recommend the refactoring, wouldn't you? :) | 15:16 |
| intellectronica | allenap: if we adopt this as a policy we will eventually have to split the tests, if only so that they don't serve as a negative example for new contributors | 15:17 |
| allenap | danilos: +1 | 15:17 |
| mars | danilos, or 400, or whatever | 15:17 |
| sinzui | intellectronica: One line that creates a file or email message requires that the test be run on a different layer. In many cases the test is actually moving to a new theme and I think it is easier to read the that aspect of the test separately. | 15:17 |
| allenap | intellectronica: Good point. | 15:17 |
| intellectronica | allenap: i guess it's the same for db state. don't rely on db state unless it's either set in the sample data or in the prologue to your test | 15:17 |
| danilos | mars, well, reviewers should always look for points of refactoring, so I just feel such "rule" wouldn't really make any difference, but sure :) | 15:18 |
| allenap | intellectronica: I agree, and I don't know of a way to make it less vague than that. I suppose that's what I was looking for. | 15:18 |
| gary_poster | I find the stories that doctests tell valuable. The stories are often long--and in fact, I often wish they were *better* connected, not less, from a narrative standpoint. That viewpoint would make me want to see manuel a preferred option, or perhaps Sphinx involved for tying together story chunks, but that is maybe herder. | 15:18 |
| intellectronica | sinzui: so what you're saying is that we should split tests if we're required to run the test in a lower layer because of a change in the middle of the test? | 15:18 |
| danilos | mars, even when the diff you are looking at is 10 lines, as a reviewer, you should wonder if the code is sitting in the right place | 15:18 |
| gary_poster | and means that it doesn't help for reading text files, only processed files | 15:19 |
| mars | danilos, well, I don't think of it as a rule, just a recommendation for reviewers. Kind of a warning, or code advice. | 15:19 |
| gary_poster | s/herder/harder/ | 15:19 |
| mars | danilos, right | 15:19 |
| danilos | anyway, it seems this is a hot topic | 15:19 |
| danilos | allenap, bac, shall we move it to the mailing list or try to come up with a conclusion here? | 15:20 |
| gary_poster | I think a lot of people talking about dividing things up are coming from the perspective of (1) expertise and (2) testing. | 15:20 |
| sinzui | intellectronica: yes. In the exampled I am thinking about, registiry object deletion, email notification, these themes are different from the core uses of the object. | 15:20 |
| allenap | danilos: Mailing list I think. There are many threads of conversation and this isn't going to end here anyway ;) | 15:20 |
| gary_poster | they are valuable, but these are narratives. | 15:21 |
| intellectronica | sinzui: yes, i think that's a good guideline | 15:21 |
| danilos | allenap, right, I think the general consensus is that we don't want 200 as the limit, so it's best to discuss other solutions to the problem you observe on list | 15:21 |
| bac | danilos: yes, let's move the discussion to the mailing list. allenap will you start the discussion there? | 15:21 |
| allenap | bac: Sure. | 15:21 |
| mars | gary_poster, good point | 15:21 |
| gary_poster | :-) thank you mars | 15:21 |
| bac | [action] allenap to start discussion on the ML about doctest size, refactoring, moving corner cases to unittests, etc | 15:21 |
| MootBot | ACTION received: allenap to start discussion on the ML about doctest size, refactoring, moving corner cases to unittests, etc | 15:21 |
| bac | [topic] Security checks: ``try:...except UnauthorizedError:...`` vs. canView [gary] | 15:22 |
| MootBot | New Topic: Security checks: ``try:...except UnauthorizedError:...`` vs. canView [gary] | 15:22 |
| gary_poster | OK I almost have the conversation pastable but not quite. | 15:22 |
| gary_poster | I was talking with EdwinGrubbs about a security check. He was checking whether a user had a specific permission on an object. He was using a utility in the LP tree that I forget. | 15:22 |
| gary_poster | I said that my top preference would be try:...except UnauthorizedError:... and my next preference would be canAccess and canWrite from zope.security.checker. | 15:22 |
| gary_poster | canView is nicer because you do not specify the permission, which is fragile. | 15:22 |
| gary_poster | The try/except approach uses the basic security machinery directly. It is of course the basic mechanism that Zope provides. | 15:22 |
| gary_poster | Edwin pointed out that this was not a pattern in the LP tree. He was also afraid it would be slower. | 15:22 |
| gary_poster | I'm pretty sure it will not be slower than calling a Python function, but can verify with a timeit test. | 15:22 |
| gary_poster | So, I suppose I have these thoughts: | 15:23 |
| gary_poster | 1) I'd like to make sure that we generally agree that we don't want to specify permissions unless we have to | 15:23 |
| gary_poster | (that is, my main suggestion from the story above) | 15:23 |
| gary_poster | 2) I'd like people to consider using the try/except approach, and for it to be a reasonable/approved one. If we want me to construct a timeit test to verify I can. | 15:24 |
| gary_poster | That's it. | 15:24 |
| gary_poster | s/conversation pastable/introduction pastable/ ;-) | 15:25 |
| * danilos reads | 15:25 | |
| intellectronica | gary_poster: i also think that using try/except is clearer | 15:25 |
| gary_poster | cool | 15:25 |
| bac | i think a lot of us have heard the "wisdom" that try/except is slower. i think a timeit test to disprove that would be useful. | 15:26 |
| danilos | I know we are using checkPermission for these things | 15:26 |
| gary_poster | EdwinGrubbs: are you around to say remind me what the utility was that you used at first? | 15:26 |
| gary_poster | Maybe it is checkPermission | 15:26 |
| EdwinGrubbs | yes, it was check_permission() | 15:26 |
| danilos | from canonical.launchpad.webapp.authorization import check_permission | 15:27 |
| gary_poster | ah ok, looking | 15:27 |
| danilos | bac, +1 | 15:27 |
| flacoste | it's a wrapper around checkPermission | 15:27 |
| intellectronica | bac: why is performance an issue here? usually you don't call these in loops | 15:27 |
| gary_poster | flacoste: oh ok | 15:27 |
| danilos | example: check_permission('launchpad.Edit', pofile) | 15:27 |
| flacoste | and i think actually the try: except: is what is used under the hood by check_permission anyway | 15:27 |
| EdwinGrubbs | you do call them in loops if you are displaying a list of objects | 15:27 |
| gary_poster | so generally, I don'y think we should use check_permission (or checkPermission) again, if at all possible. That's my #1, | 15:27 |
| gary_poster | canAccess and canWrite are less fragile | 15:28 |
| gary_poster | I'm happy to do the timeit check. I still think that try/except will be faster than a Python function, but I think that's interesting information, so I can do that and report back to the list. | 15:28 |
| henninge | gary_poster: Is this in view or in model code? | 15:28 |
| EdwinGrubbs | view code | 15:29 |
| bac | intellectronica: it may be in a loop. if some timing tests can show it is a myth then i think that's useful. | 15:29 |
| henninge | good | 15:29 |
| intellectronica | bac: knowing is definitely useful, but i think that even if we find that it is slower, we should still consider using try/except in cases where it doesn't affect performance _drastically_ | 15:30 |
| flacoste | hmm, right | 15:30 |
| flacoste | canAcess / canWrite uses try: except | 15:30 |
| flacoste | whereas checkPermission uses the checker information | 15:30 |
| bac | intellectronica: well, a lot of these cases are in vocabularies, IIRC. many of those do have performance issues | 15:30 |
| intellectronica | bac: yes, where we do it iteratively we definitely should optimize for performance | 15:31 |
| danilos | the reason I don't like try...except is that (especially with API), we will get a lot of cases where we'll have multiple conditions we are checking for | 15:32 |
| danilos | so, nested try..excepts sound ugly to me | 15:32 |
| gary_poster | If try:except is faster, as I suspect, then we do not have to have the theoretical discussion. suggested resolution for now: I'll do the timeit test and report back to the list. If there is a clear answer in favor of try/except for performance as well, then that's easy. ...except for danilo's case... | 15:32 |
| gary_poster | danilo, can you send me an example to look at? | 15:33 |
| danilos | gary_poster, I am not even sure it's a valid example, but there are cases where we do an OR of multiple permission checks | 15:33 |
| gary_poster | danilos: right, I'd like to see if it is valid; if it is not, we should know what a preferred spelling us | 15:34 |
| danilos | but that's probably just bad design where objects are not properly security-wrapped | 15:34 |
| bac | [action] gary to do timing tests for try/except, examine current usage of check_permission, and we'll discuss again next week. | 15:34 |
| gary_poster | is | 15:34 |
| MootBot | ACTION received: gary to do timing tests for try/except, examine current usage of check_permission, and we'll discuss again next week. | 15:34 |
| gary_poster | cool thanks | 15:34 |
| bac | thanks for bringing it up gary | 15:34 |
| bac | [topic] Cleaning up ImportFascist warnings [bac,sinzui] | 15:35 |
| MootBot | New Topic: Cleaning up ImportFascist warnings [bac,sinzui] | 15:35 |
| bac | jml's fix to the import fascist revealed lots of places where we have problems | 15:35 |
| bac | we need to drive those warnings back to zero or we'll continue to expand the problem | 15:36 |
| danilos | gary_poster, sure, why don't you ping me later and we can take a look (basically, lp.translations.browser.distroseries.DistroSeriesLanguagePackView has an example) | 15:36 |
| bac | sinzui has a branch that accomplishes a lot of it but has reported new code is introducing more problems | 15:36 |
| bac | sinzui where did you find the bulk of the offenders to be? | 15:36 |
| gary_poster | danilos: awesome thank you | 15:36 |
| sinzui | Most were explicit import from _schema_circular_imports which were not needed | 15:37 |
| sinzui | They were easy to remove | 15:37 |
| bac | sinzui: does your branch solve the problem for LP or just registry? | 15:37 |
| sinzui | LP, but there are two new ones in the last 12 hours | 15:37 |
| bac | ah, cool. so once this branch lands we can go back to zero tolerance as reviewers, no? | 15:38 |
| sinzui | BranchJobDerived BranchJobType are not in BranchJob.__all__, but are imported by translationtemplatesbuildjob and translationtemplatesbuildjob | 15:38 |
| gary_poster | should we make them errors? | 15:38 |
| gary_poster | I thought that was an option | 15:39 |
| sinzui | I could add these two classes to __all__, but I think the code team and translations team need to coordinate | 15:39 |
| danilos | sinzui, if it has happened during last 12h, it's something that's happening during a sprint in Wellington | 15:39 |
| sinzui | correct | 15:40 |
| danilos | sinzui, and I don't really overlap with them, so I'd appreciate if you mention it to jtv when he shows up and I am sure he'll be happy to fix the issue or find someone who will :) | 15:40 |
| sinzui | I will | 15:40 |
| danilos | sinzui, thanks | 15:40 |
| bac | so we're in agreement to get this cleaned up over the next week and then keep it under control? | 15:41 |
| danilos | sinzui, and thanks for spending the effort to fix it everywhere else as well, I hope we didn't have many issues in translations since we shouldn't be able to import stuff if we do (because we are not using global imports either) | 15:41 |
| danilos | bac, +1 | 15:41 |
| bac | [action] sinzui to land a import cleanup branch and then reviewers will enforce zero tolerance for introducing new issues. | 15:42 |
| MootBot | ACTION received: sinzui to land a import cleanup branch and then reviewers will enforce zero tolerance for introducing new issues. | 15:42 |
| bac | that's it for agenda items | 15:42 |
| danilos | sinzui, bac: just to confirm, this will also be part of lint checks? | 15:42 |
| bac | [topic] peanut gallery | 15:42 |
| MootBot | New Topic: peanut gallery | 15:42 |
| sinzui | landing is challenging since we are in phantom testfix | 15:42 |
| danilos | (i.e. easy to spot) | 15:42 |
| danilos | sinzui, js buildbot? | 15:43 |
| bac | danilos: i'm not sure. sinzuie? | 15:43 |
| bac | er, sinzui? | 15:43 |
| sinzui | danilos: importfacist has never been a part of lint since the testrunner has alwyas reported them | 15:43 |
| bac | thanks | 15:43 |
| sinzui | danilos: I thought I saw rockstar land a reversion to get us out of testfix | 15:43 |
| flacoste | sinzui: it's not phantom | 15:44 |
| flacoste | db_lp has three failures | 15:44 |
| bac | moving on, does anyone have an issue that is not on the agenda? | 15:44 |
| flacoste | related to code hosting | 15:44 |
| flacoste | i have | 15:44 |
| bac | flacoste: go | 15:44 |
| flacoste | but it's not related to reviewers :-) | 15:44 |
| bac | hmm | 15:44 |
| flacoste | but would like the opportunity that we have a lot of attention :-) | 15:44 |
| flacoste | we need a volunteer to fix the db_lp failures | 15:44 |
| bac | flacoste: ok, reminding you we're at 45 minutes | 15:44 |
| flacoste | that's all | 15:45 |
| flacoste | bac: you can cloase the meeting | 15:46 |
| bac | flacoste: let's try to resolve that outside the meeting | 15:46 |
| bac | #endmeeting | 15:46 |
| MootBot | Meeting finished at 09:46. | 15:46 |
| bac | thanks everyone for coming | 15:46 |
| gary_poster | thanks bac | 15:46 |
| mars | thanks bac | 15:47 |
| flacoste | these are the errors: | 15:47 |
| flacoste | Tests with errors: | 15:47 |
| flacoste | lp.code.model.tests.test_branchjob.TestRevisionsAddedJob.test_getMergedRevisionIDs | 15:47 |
| flacoste | lib/canonical/launchpad/ftests/../../../lp/bugs/doc/bug-set-status.txt | 15:47 |
| danilos | thanks bac | 15:47 |
| flacoste | lp.code.model.tests.test_codeimport.TestCodeImportDeletion.test_deleteIncludesJob | 15:47 |
| bac | flacoste: what about the other two? | 15:47 |
| sinzui | LayerIsolationError | 15:47 |
| bac | oh, nm | 15:47 |
| sinzui | for all of them | 15:47 |
| flacoste | indeed | 15:48 |
| sinzui | I suspect the test env, not the test | 15:48 |
| flacoste | well | 15:48 |
| flacoste | test_deleteIncludesJob | 15:48 |
| flacoste | is an Unauthorized error | 15:48 |
| sinzui | sorry, I did not see that | 15:49 |
| flacoste | well, no | 15:49 |
| flacoste | layerIsolationError are problems with the test | 15:49 |
| flacoste | it means that an object created by the test cannot be garbaged collected | 15:49 |
| sinzui | The bug test has not change in 6 months, and before that 2 years ago | 15:51 |
| flacoste | right | 15:53 |
| flacoste | the problem is that these layerIsolation might not be directly associated with the test | 15:53 |
| flacoste | but it sure comes from some kind of changes | 15:53 |
| flacoste | looking at the garbage left, it seems to be bzrlib objects | 15:54 |
| sinzui | I saw this happen when the order of tests changed | 15:54 |
| sinzui | I had to set the dirty db flag in some tests to fix the issue | 15:54 |
| flacoste | was the garbage related to bzr in that case? | 15:55 |
| flacoste | should we simply revert these revisions? | 15:56 |
| sinzui | No. two or three years ago I discovered that many unit tests were creating objects. I had to modify the tearDown(). The error was only observed by testing the layer | 15:56 |
| flacoste | is the Unauthorized error reproducible locally at least? | 15:56 |
| === deryck is now known as deryck[lunch] | ||
| sinzui | No it does not fail | 16:01 |
| === salgado-lunch is now known as salgado | ||
| flacoste | sinzui: should we simply trigger a rebuild? | 16:13 |
| sinzui | flacoste: I was tempted. | 16:15 |
| === deryck[lunch] is now known as deryck | ||
| sinzui | mars: do you have a script that scores new users yet? | 17:24 |
| === gary_poster is now known as gary-lunch | ||
| === danilos is now known as daniloff | ||
| === gary-lunch is now known as gary_poster | ||
| === salgado is now known as salgado-afk | ||
| sinzui | losas ping | 21:46 |
| jml | sinzui, what's your boggle? | 21:56 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!