=== Ursinha is now known as Ursinha-afk === Ursinha-afk is now known as Ursinha === Ursinha is now known as Ursinha-afk === Ursinha-afk is now known as Ursinha === mrevell is now known as mrevell-lunch === Ursinha is now known as Ursinha-lunch === mrevell-lunch is now known as mrevell [15:00] #startmeeting [15:00] Meeting started at 08:59. The chair is bac. [15:00] Commands Available: [TOPIC], [IDEA], [ACTION], [AGREED], [LINK], [VOTE] [15:00] hi bac [15:00] who is here? [15:00] me [15:00] me [15:00] me [15:00] me [15:00] me [15:00] me [15:00] me [15:00] jtv is on vacation [15:00] me [15:00] me [15:00] er, unavailable [15:00] leonardr also unavailable [15:00] me [15:00] me [15:02] me [15:02] now we can start. [15:02] first, welcome mrevell to the reviewers meeting and to being a UI mentat! [15:02] Thanks :) [15:03] please, everyone, request UI reviews from mrevell and salgado first [15:03] me! [15:03] there are no new agenda items to discuss today [15:03] [topic] outstanding items [15:03] New Topic: outstanding items [15:03] * sinzui -- lint changes? [15:04] me [15:04] no, none today [15:04] sinzui: is this weekly reminder useful or should we drop if from the list and just be surprised when it happens? [15:05] bac: it should be a bug in lp-foundations I think [15:05] sinzui: if you'll file the bug i'll promise never to mention it here again. :) [15:05] I will do so [15:06] * bac to do stuff with maris regarding introduction of design metrics into reviews. [15:06] done [15:06] sinzui, please feel free to triage it as you feel appropriate, since you know the context/importance better than I [15:06] i sent out an email just a little while ago. [15:06] I will [15:06] i'm proposing a three week cycle where we concentrate on a new design metric in our reviews each cycle [15:06] first up, is test performance [15:07] the goal specified on the wiki is tests for a class should execute in under 2 seconds. the goal is clearly arbitrary. [15:08] but i think it is worthwhile to *pay attention* to new tests and the time they take as well as changes to timing for modified tests [15:09] if you haven't, please read the email on the list and follow up there, unless you have comments now. [15:10] any comments? [15:10] bac: So we're not actually enforcing the guideline, just checking that new tests execute in < 2 seconds? [15:10] bac: and modified ones don't increase by 2 seconds? [15:11] abentley: no, i don't think strict enforcement of that guideline is called for. [15:11] but i think reviewers and developers need to get into the habit of paying attention to test performance. [15:11] bac: you might be interested in the --slow-test option of the test runner [15:11] personally, i used to rarely look at the time new tests took [15:11] bac: Maybe we should change it to the metric we *are* checking, then? [15:11] benji: can you elaborate [15:12] if you use -c (colorized output) it will highlight tests that take longer than the specified number of seconds [15:12] Looks like someone will be deleting notfound-traversals soon [15:13] heh [15:13] yay [15:13] bac: is the "test for a class" mean a given test case class, or given coverage for, say, a model class? [15:13] oh, and you have to use -vvv [15:13] where do doctests fit into that timing? [15:13] jcsackett: good question [15:13] benji: +1 on making quick reply to bac's email with example :-) [15:13] a single doctest is a single test, IIRC [15:13] as you see the metric is vague and arbitrary [15:13] gary_poster: k [15:14] they'll all blow the 2 seconds then :) [15:14] but paying attention to test timing is what i'm proposing, regardless of where we draw the line [15:14] in the example case i gave in the email, i think the original test took something like 42 seconds [15:14] bac: so in other words, we can disregard the "2 seconds" thing and instead go with a "good lord the tests for this branch take a long time" [15:14] after tweaking i got it down to under 10 seconds [15:14] 10 [15:15] 10 > 2 but the exercise was worthwhile [15:15] jcsackett: yes, that sounds good [15:15] bac: cool. thanks. [15:15] bac: There's some inconsistency between what you said in the email and what you're saying here. In the email, you talked about incorporating the existing metric of all tests for a class. Now, you're talking about just new and modified tests. [15:16] abentley: i don't think so [15:16] "tests for a class" seems a bit hard to limit to me, fwiw [15:17] bac: Okay, could you explain why the inconsistency I perceive is an illusion? [15:17] abentley: from the email "To incorporate this metric into your reviews you'll obviously need to run the tests. If a test is brand new then you can just evaluate the timing. Modifications to existing tests should be compared with before and after timings. So reviewers, either download the code being reviewed and run the tests yourself or ask the developer for comparative timings. Start the conversation. [15:17] " [15:17] that paragraph mentions new tests and modified tests [15:18] Can we just say that the phrase "tests for a class" isn't super pertinent right now, and that what you just quoted is more important? [15:18] bac: okay, the inconsistency is within the email, too. I give up. [15:19] abentley: i quoted from the Architecture Guide and then went on to explain what i was proposing. [15:19] with the caveat that the guide is mutable [15:19] can we paraphrase this as "please review how long your tests are taking" [15:19] +1 :-) [15:20] bigjools: lovely, +1 [15:20] great. Next? [15:20] yes, any other topic to discuss? [15:21] thanks all. [15:21] #endmeeting [15:21] Meeting finished at 09:21. [15:21] thanks bac [15:21] thank you [15:21] bac: You suggested that we incorporate "Tests for a class should complete in under 2 seconds" by checking new and modified tests, and that's and inconsistency in my view. [15:21] thanka bac [15:21] bac: dammit, was typing. [15:21] bac: I have a topic. [15:22] abentley: can it be deferred? [15:22] bac: sure. [15:22] abentley: thanks, please add to the wiki for next week. === Ursinha-lunch is now known as Ursinha === benji is now known as benji-lunch === benji-lunch is now known as benji === Ursinha is now known as Ursinha-bbl === salgado is now known as salgado-afk === Ursinha-bbl is now known as Ursinha