| teknico | is anyone around today? :-) I need a second review of https://codereview.appspot.com/8094045/ | 12:14 |
|---|---|---|
| dimitern | teknico: will take a look | 12:52 |
| fwereade | tkeLGTM | 12:52 |
| fwereade | teknico, LGTM | 12:53 |
| dimitern | :) | 12:53 |
| dimitern | sorry to be so late | 12:53 |
| fwereade | teknico, just one thing to think about and one test to add | 12:53 |
| teknico | fwereade, re: the args num removal, I removed it because of a request by rogpeppe, as you can see above in the code review | 12:54 |
| fwereade | teknico, I thought he suggested dropping it at the api layer | 12:54 |
| fwereade | teknico, I definitely agree with that | 12:54 |
| teknico | fwereade, oh, so I probably misunderstood his suggestion, I'll check | 12:55 |
| fwereade | teknico, it's just that the CLI can take a moment to connect, and failing fast there is nice for the user | 12:55 |
| teknico | fwereade, right | 12:55 |
| teknico | fwereade, and I'll add the test you mentioned, thanks for the review | 12:55 |
| fwereade | teknico, thanks for the code :) | 12:55 |
| fwereade | dimitern, a couple of things btw | 13:17 |
| dimitern | fwereade: yeah? | 13:17 |
| fwereade | dimitern, I have reviews up: https://codereview.appspot.com/8140044/ https://codereview.appspot.com/8154043/ | 13:17 |
| dimitern | fwereade: I'll take a look a bit later - i'm deeply into status tests refactoring right now | 13:17 |
| fwereade | dimitern, and, in kanban, there is a defect card that you *might* want topick up after what you're doing now because it leads into it nicely | 13:18 |
| dimitern | fwereade: which one? | 13:18 |
| fwereade | dimitern, it's the machine-id-in-status one: it's in core's TODO lane but feel free to grab it if you want it :) | 13:18 |
| dimitern | fwereade: but i'm not yet changing what status outputs | 13:19 |
| fwereade | dimitern, it really is only a suggestion | 13:19 |
| dimitern | fwereade: it could be a follow-up though, after the status refactoring | 13:20 |
| dimitern | fwereade: thanks | 13:20 |
| fwereade | dimitern, if say at nearly-eod you think to yourself "what tiny follow-up branch do I feel like doing before I go" | 13:20 |
| dimitern | fwereade: sure :) sgtm | 13:20 |
| fwereade | dimitern, I agree it's not the same as what you're doing now and should certainly not be rolled into the same CL :) | 13:20 |
| dimitern | fwereade: yep | 13:23 |
| dimitern | fwereade: can I use export_test to provide access to the internals to some other module? | 13:24 |
| fwereade | dimitern, only to the associated foo_test package | 13:25 |
| dimitern | fwereade: I'm writing the cleanup stuff for the status tests, and one step is destroying all machines created | 13:25 |
| fwereade | dimitern, not to everything | 13:25 |
| dimitern | fwereade: but I cannot destroy (normally) machines with JobManageEnviron | 13:25 |
| dimitern | fwereade: yeah, that sucks.. any thoughts how to work around it? | 13:25 |
| fwereade | dimitern, maybe just Reset the test suite? | 13:25 |
| fwereade | dimitern, UniterSuite has the same sort of problem | 13:26 |
| dimitern | fwereade: ah, let me try | 13:26 |
| fwereade | dimitern, check what we do there | 13:26 |
| dimitern | fwereade: cool, it worked like charm, 10x :) | 13:43 |
| fwereade | dimitern, sweet | 13:43 |
| === wedgwood_away is now known as wedgwood | ||
| dimitern | fwereade: nice CLs - LGTM on both | 15:16 |
| fwereade | dimitern, tyvm | 15:16 |
| dimitern | fwereade: and I'm ready with the status stuff - would you take a look? https://codereview.appspot.com/8133045 | 15:16 |
| fwereade | dimitern, soon I hope; thinking hard about something | 15:16 |
| dimitern | fwereade: sure, np. I'll go for a run and a short lunch afterwards | 15:17 |
| fwereade | dimitern, reviewed | 17:02 |
| fwereade | dimitern, not quite an LGTM but just because I want to know what you think about the change I suggest | 17:02 |
| dimitern | fwereade: cheers! I just made a small change to optimize test speed | 17:02 |
| fwereade | dimitern, if you're fine with it and clear about it then I'll LGTM it; let me know what you think | 17:02 |
| dimitern | fwereade: I'm running status --format {yaml,json} concurrently | 17:02 |
| dimitern | fwereade: i'll take a look | 17:03 |
| dimitern | fwereade: this change speeds up the tests about 50% (which is a bit better than they used to be, since changing them slowed them down - state is recreated each time) | 17:03 |
| fwereade | dimitern, excellent | 17:03 |
| fwereade | dimitern, very good idea | 17:04 |
| dimitern | fwereade: sweet! I'll implement your changes and repropose then | 17:06 |
| dimitern | fwereade: thinking about it, if I drop the output from the test case, the concurrency won't be even needed | 17:50 |
| fwereade | dimitern, ah, better still | 17:51 |
| dimitern | fwereade: and I can speed-up and simplify the tests even further by putting basically everything into one case with multiple expects | 17:51 |
| dimitern | fwereade: because the nature of the tests are incremental | 17:51 |
| fwereade | dimitern, I am a little bit suspicious here | 17:51 |
| dimitern | fwereade: but with the added benefit of independence between tests and multiple cases still possible | 17:52 |
| fwereade | dimitern, this sort of thing becomes a real hassle as the incremental chains grow | 17:52 |
| fwereade | dimitern, what's the speed/nastiness tradeoff? | 17:52 |
| dimitern | fwereade: yes, but my point is - you can still have small separate cases, well isolated | 17:52 |
| dimitern | fwereade: and have bigger aggregated cases which built incrementally and expect as they go | 17:53 |
| fwereade | dimitern, I agree that several of those test cases can be profitably combined into incremental ones | 17:53 |
| dimitern | fwereade: well, if most of the cases are incremental the speed will increase, because the state will be setup once | 17:53 |
| fwereade | dimitern, understood | 17:54 |
| dimitern | fwereade: but unlike before, you can easily add independent other cases with the same framework | 17:54 |
| fwereade | dimitern, but faster doesn't beat easier-to-modify, so ideally none of the cases should have more than a few expects | 17:55 |
| dimitern | fwereade: actually concurrency can still be there - but in expect{} only | 17:55 |
| fwereade | dimitern, yeah, that's probably worth it | 17:55 |
| dimitern | fwereade: ok, i'll try both and see how it looks | 17:55 |
| dimitern | fwereade: scratch that, having multiple expect is possible, but you still have a single summary, so it's not good to mix cases like that | 18:05 |
| fwereade | dimitern, comment field for the expects? :) | 18:05 |
| dimitern | fwereade: hmm.. why not | 18:06 |
| dimitern | fwereade: but then the summary is redundant - it'll be in expect only | 18:08 |
| === wedgwood is now known as wedgwood_away | ||
| fwereade | dimitern, that's probably fine, isn't it? | 18:08 |
| dimitern | fwereade: yeah, doesn't look so bad actually | 18:08 |
| fwereade | dimitern, but, well | 18:08 |
| dimitern | fwereade: will propose shortly | 18:08 |
| fwereade | dimitern, if you're gruping them yu probably ought to be able to explain why those tests need to go together | 18:08 |
| fwereade | dimitern, so maybe it's not so redundant :) | 18:09 |
| dimitern | fwereade: ah, good point | 18:09 |
| dimitern | fwereade: sweet! it improved the speed even further: now it's more than 100% better than the old tests (before refactoring) | 18:33 |
| * fwereade cheers | 18:34 | |
| dimitern | fwereade: ah, actually no - they're about as fast as before, just checked - but the added functionality/flexibility is much greater | 18:40 |
| fwereade | dimitern, sounds like a win regardless | 18:40 |
| fwereade | dimitern, they were way nicer even at the first propose | 18:40 |
| dimitern | fwereade: reproposed | 18:40 |
| dimitern | fwereade: oh yeah - a joy to look at, rather than a disgust :) | 18:41 |
| hazmat | anybody can offer insight into this error.. on bootstrap --upload-tools ... error: cannot upload tools: build command "go" failed: exit status 2; # launchpad.net/juju-core/cmd/jujud | 18:44 |
| hazmat | pack: cannot open $WORK/launchpad.net/juju-core/cmd/jujud/_obj/_go_.8 | 18:44 |
| hazmat | i'm using go tip.. and just did a recursive update via go get | 18:45 |
| dimitern | hazmat: can you try going inside cmd/jujud/ and running go install there ? | 18:47 |
| hazmat | dimitern, returns immediately.. juju version returns -> 1.9.13-quantal-i386 | 18:48 |
| hazmat | ditto for jujud | 18:48 |
| dimitern | hazmat: also, if you switched to go-tip and using the same $GOPATH as with go-1.x it might be the issue - rm -fr $GOPATH/pkg/ and try again | 18:48 |
| dimitern | hazmat: ah, you're on i386? | 18:48 |
| hazmat | dimitern, that's probably it, thanks.. indeed re 32b | 18:48 |
| fwereade | dimitern, I think I see another avenue for niceness, potentially | 18:49 |
| fwereade | dimitern, there are quite a lot of duplicated chunks of data in the tests still | 18:49 |
| dimitern | hazmat: not sure if it'll ever work on i386 for now | 18:49 |
| hazmat | i should probably do separate gopaths for go tip vs release | 18:49 |
| hazmat | dimitern, it used to | 18:49 |
| dimitern | hazmat: yeah, that's best | 18:49 |
| dimitern | fwereade: yeah? | 18:50 |
| dimitern | fwereade: in the expected output? | 18:50 |
| fwereade | dimitern, I'm thinking things like var exposedService = M{"exposed":true, "charm": "cs:foo/bar-1"} | 18:50 |
| dimitern | fwereade: the output was the only thing I didn't want to touch, even with duplicated parts, because it helps to see what's there | 18:51 |
| fwereade | dimitern, and var machine0 = M{"dns-name": "dummyenv-0.dns", "instance-id": "dummyenv-0"} | 18:51 |
| dimitern | fwereade: it has to be explicit somewhere | 18:52 |
| dimitern | at least | 18:52 |
| fwereade | dimitern, I *think* there's enough duplication that a list of <10 building blocks will collapse the rest a lot | 18:52 |
| dimitern | fwereade: even if in subsequent cases it's short-circuited | 18:52 |
| fwereade | dimitern, but I'll leave it to your judgment | 18:53 |
| dimitern | fwereade: ok, I'll think about it | 18:53 |
| dimitern | fwereade: otherwise? | 18:53 |
| fwereade | dimitern, otherwise LGTM, I think, just skimming the rest again | 18:54 |
| fwereade | dimitern, sent | 18:56 |
| dimitern | fwereade: cool! tyvm | 18:56 |
| dimitern | fwereade: I think I prefer having the output unchanged first time we're testing a scenario, and use shortcuts afterwards, sane? | 18:57 |
| fwereade | dimitern, I'm fine with that | 18:57 |
| fwereade | dimitern, we can decide next pass | 18:57 |
| dimitern | fwereade: great, I'll do it like that then | 18:58 |
| dimitern | fwereade: looks nicer now I think | 19:05 |
| dimitern | fwereade: which status-related card were you referring to earlier? I can't seem to see it. | 19:05 |
| === wedgwood_away is now known as wedgwood | ||
| fwereade | dimitern, the big red one in core's TODO | 19:24 |
| dimitern | fwereade: right! thanks | 19:24 |
| fwereade | dimitern, np | 19:25 |
| dimitern | fwereade: so what's the idea in bug 1121916 | 19:25 |
| _mup_ | Bug #1121916: status compatibility <cmdline> <juju-core:New> < https://launchpad.net/bugs/1121916 > | 19:25 |
| dimitern | fwereade: changing the machine id to int? | 19:26 |
| hazmat | latest failure trying to bootstrap with constraints, no image found.. http://pastebin.ubuntu.com/5659178/ | 19:45 |
| hazmat | if i already uploaded tools to the bucket, shouldn't it defer to those tools on subsequent bootstraps? | 19:46 |
| hazmat | dimitern, yeah.. that's the core | 19:47 |
| hazmat | perhaps constraints are working yet.. ? | 19:48 |
| hazmat | i though they worked for the bootstrap node | 19:48 |
| hazmat | hmm.. sans constraints, failed bootstrap it looks because it can't find/setup mongo http://juju-dist.s3.amazonaws.com/tools/mongo-2.2.0-quantal-i386.tgz | 20:00 |
| hazmat | 32bit client dictation | 20:00 |
| dimitern | hazmat: exactly, that was my point - there is no mongo 2.2 for i386 | 20:01 |
| hazmat | oh.. thats why it can't find the image either.. its client dictating 32bit for tools and cascading to images.. mongo. etc | 20:01 |
| dimitern | hazmat: yeah, it's viral | 20:02 |
| dimitern | hazmat: you could try without --upload-tools | 20:02 |
| dimitern | hazmat: but i'm not sure there are i386 tools in the dist bucket at all | 20:03 |
| hazmat | dimitern, no matching tools version without the --upload | 20:06 |
| dimitern | hazmat: you can probably build mongo from source + ssl and put at the same path in your private bucket from environ | 20:07 |
| hazmat | dimitern easier to fire up a 64bit vm | 20:07 |
| hazmat | but thanks for helping debug | 20:07 |
| dimitern | :) sure | 20:07 |
| fwereade | hazmat, sorry about that -- fixing up the tool-finding is in the backlog and I hope we will get there soon | 21:08 |
| === wedgwood is now known as wedgwood_away | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!