-specific)? [15:02] leonardr: ah, that shouldn't be too hard; i just thought based on some other pages if it only applied to one page that's where we put it. i'll move it. [15:03] jcsackett: it won't apply to one page if you reuse it for the fieldset collapsing code? [15:03] if i'm wrong, then don't move it [15:03] leonardr: the fieldset already has styles set that aren't quite the same. [15:04] jcsackett: ok, don't bother, but add it to the bug so we can take another look later [15:04] leonardr: dig. [15:08] leonardr: the exception you showed me on the keyring branch yesterday also happens on launchpadlib trunk [15:08] benji: well, damn [15:08] i'll look into it after i finish this review [15:13] jcsackett: this question may expose my ignorance more than anything else, but... [15:13] lines 387-388 [15:14] client.click(link=u"Configuration links") triggers some javascript. does it then do anything to call waits.forPageLoad? surely the page has already loaded? [15:14] or does forPageLoad do something else? [15:17] maybe mars knows better than me -^ [15:19] jcsackett: i had a question while you were out [15:19] jcsackett: this question may expose my ignorance more than anything else, but... [15:19] lines 387-388 [15:19] client.click(link=u"Configuration links") triggers some javascript. does it then do anything to call waits.forPageLoad? surely the page has already loaded? [15:19] or does forPageLoad do something else? [15:20] also, do the windmill tests run in a deterministic order? can you be sure that test_configured_starts_collapsed runs after test_not_fully_configured_starts_shown? [15:21] for the first question: so, there's no generic "wait" thing in windmill that i could find, and the test would fail because the class wasn't actually removed (or added depending on test) when it then went to test it. [15:21] there's wait.sleep() but that's documented as pausing the browser, which isn't really appropriate. [15:22] i can add a comment explaining that it's a generic wait, and as i look at it i should decrease the timeout. [15:22] jcsackett: sounds good. i bet mars has the answer [15:23] for the second question: it doesn't matter (or it shouldn't) which order they run in; since the product is created in setup, each test should start with a product that's not configured. [15:25] jcsacket: if that's the case, it looks like both products are created with the same name ('hidden-configs'). how is that possible? [15:28] leonardr: in my experience all tests seem to have a built in teardown that destroys things created in setUp. [15:28] jcsackett: ok, as long as it works [15:28] it works in many other unittests; i'm not sure about windmill. [15:28] would mars be the person to check that with? [15:32] mars: i guess. my concern, now that i've dug through the code, is that the windmill testing layer doesn't inherit from FunctionalLayer, which is where i'd imagine such cleanup code to be [15:33] it's not in TestCaseWithFactory, unless the act of creating a LaunchpadObjectFactory() in setup obliterates any objects created by any other factory [15:33] jcsackett: sorry, misaddressed to mars -^ [15:34] leonardr: i can add a teardown method to be sure. [15:34] leonardr: old objects are cleaned up by the db reset at the end of the test [15:34] score. [15:34] so DatabaseLayer [15:35] jcsackett: and i've confirmed that setUp is run before each test [15:35] so you're fine [15:35] jcsackett: btw tearDown is (roughly speaking) deprecated [15:35] jcsackett: its generally much better to use addCleanup (but not always - explore them both, use your judgement) [15:36] lifless: dig. in my experience so far it's all just nicely taken care of. [15:36] lifeless, rather. ^ [15:36] jcsackett: this is the kind of thing you get when i review launchpad branches because most of the time i'm off in my own little world [15:37] leonardr: all good. it helps to have someone examining my assumptions. :) [15:49] jcsackett: r=me with minor changes [15:49] leonardr: cool, thanks! [15:49] i'll make those changes shortly. === leonardr changed the topic of #launchpad-reviews to: On call: leonardr || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [15:50] mars, do you want a review of https://code.edge.launchpad.net/~mars/launchpad/add-profiling-feature-flag/+merge/39179 or do you need to do some work to address lifeless's comment? === matsubara is now known as matsubara-lunch === Ursinha is now known as Ursinha-lunch === fjlacoste is now known as flacoste [17:04] leonardr: hi, I've got a small branch up for review that exposes /builders on the API if you can take it? === benji is now known as benji-lunch [17:05] bigjools, sure === leonardr changed the topic of #launchpad-reviews to: On call: leonardr || Reviewing: bigjools || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [17:07] leonardr: thank you === matsubara-lunch is now known as matsubara [17:22] bigjools: "(usually a member of the buildd-admins team)" that seems specific to the way we use launchpad rather than the code itself. is that okay? [17:34] leonardr: buildd-admins are a celebrity in LP [17:34] bigjools: ok, you're good then [17:34] i just have some minor wording suggestions [17:35] sure thing [17:42] bigjools: r=me with very minor changes, https://code.edge.launchpad.net/~julian-edwards/launchpad/api-expose-builders/+merge/39379 === leonardr changed the topic of #launchpad-reviews to: On call: leonardr || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [17:42] leonardr: great, thanks [17:43] leonardr: can you mark your review approved, I think you just landed a comment only. [17:44] oops [17:44] thanks for the suggestiond [17:47] leonardr: FWIW it looks like lp_attributes already sorts in a good way [17:47] bigjools: oh, i guess i used to be smarter than i am now [17:47] :) [17:47] it puts the boilerplate stuff first then the sorted attributes === benji-lunch is now known as benji === Ursinha-lunch is now known as Ursinha [18:58] rockstar, can i get a quick review from you? [18:58] https://code.edge.launchpad.net/~leonardr/launchpadlib/fix-token-authorization/+merge/39391 [18:58] leonardr, it may be lossy, as I'm at UDS. [18:59] rockstar: ok, if mars responds in the next 5 minutes and says he can do it never mind. otherwise, it doesn't have to be super quick [18:59] (or if someone else can take it) [18:59] leonardr, the code looks good, reading the description now. [18:59] leonardr, ? [19:00] mars, just seeing if you want to take a review, since i don't think you're at uds [19:00] leonardr, sure [19:00] I'm in and out, but I'll look - 38 lines, that's a good size :) [19:01] mars, I've got it. [19:02] (If it was any bigger, I would just get it now) [19:02] ok, rockstar has it [19:02] k [19:02] thanks rockstar [19:02] * rockstar high fives mars and yells "TEAMWORK!" [19:03] rockstar, btw, if you see Martin Pool, ask him if he has a moment to review my Feature Flags Test Fixture branch - people would love to see that land [19:03] mars, okay. I'll keep an eye out. [19:03] thank you [19:29] sounds good [20:56] leonardr: almost forgot: https://code.edge.launchpad.net/~benji/wadllib/missing-files/+merge/39400 [20:59] benji: r=me [20:59] leonardr: cool; shall I do a release? [20:59] benji: yes, please. let me know if you're missing anything from the process [20:59] k === fjlacoste is now known as flacoste === Ursinha is now known as Ursinha-brb === matsubara is now known as matsubara-afk === henninge is now known as henninge_ === leonardr 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 === henninge_ is now known as henninge [22:03] leonardr: I've responded to your keyring question/issue. [22:03] benji, thanks [22:04] benji: that makes a lot of sense, thanks [22:04] i'm eod but i'll review your changes in a bit [22:28] benji: how tough would it be to test the storage and retrieval of the same application name w/two different service roots? [22:29] leonardr: trivial; shall I add that? [22:29] benji: yes please [22:34] leonardr: test added [22:36] leonardr: oh, I need to be granted access to wadllib on PyPI to complete the release [22:40] benji: sure [22:49] benji, you are benji on pypi? [22:50] leonardr: yep [22:55] benji, the role is yours [22:55] thanks, the upload worked [22:59] benji, i can't parse "(this assertion is of the test mechanism, not a test assertion)." [23:00] this assertion is veriyging the test mechanism? [23:00] exactly; I'm verifying my assumption that there will only be two sets of credentials stored in the fake keyring [23:00] benji: let me run some additional assertions by you [23:00] 1. the fake keyring starts out empty [23:01] 2. the application key has a specific form incorporating the application name and the hostname (?) [23:02] i was hoping for #2 but i'm willing to be convinced it's not necessary/ not appropriate to test here [23:02] 1 seems reasonable [23:04] at first I thought 2 was overconstraining things, but we do want to be sure the keys remain reable between releases of launchpadlib, so it makes sense [23:04] I also want to add an assertion that the application name is include in the key [23:05] isn't that part of 2? [23:06] indirectly, but I think all three (app name, server name, overall structure) are important [23:06] and it's cheap to test them all, so... [23:07] ok, go for it [23:11] leonardr: done [23:14] * leonardr looks [23:20] benji: r=me, though i don't think we should merge this yet [23:21] leonardr: k === Ursinha-brb is now known as Ursinha