[00:16] m_3: ec2 is having a less pissy day [00:16] precise-ec2-charm-alice-irc passed ! [00:21] davecheney: nice! [00:25] two for two [00:25] going to try with juju-core 1.9.12 [01:14] m_3: and now ec2 is having a sad period [01:14] 3 failures in a row [01:28] m_3: aww crap [01:28] status: error [01:28] status-info: 'hook failed: "install"' [01:28] + grep -v -q error [01:28] ^ not spotting the problem [01:37] * davecheney steps out for lunch [02:33] davecheney: ack [02:33] davecheney: gonna go get dinner [02:34] davecheney: see you on the flipside [02:57] davecheney: review done [02:58] davecheney: I think the version tests should move into the cmd package [02:58] that's about it [03:18] thumper: thanks mate [03:18] will take another crack now [04:10] * thumper is done for today [04:10] caio [05:18] davecheney: hi, i'm half way through fixing the raring tests and i noticed you have the card assigned to you. have you started on it yet? [05:19] nah, grab it [05:19] awesome thans [05:19] 4 [05:20] davecheney: it turns out the git issues can be fixed by setting env vars in setup test. but if we ever deploy on that later version, we will need to ensure the git config is correctly written [05:20] wallyworld_: i could not figure out what was wrong with tims' setup [05:20] also, the later version of git in raring has different error messages, and we are doing exact string matches in the tests [05:20] i tried in a fresh raring vm [05:21] and couldn't replicate the problem [05:21] wallyworld_: ahh, i have a fix for that [05:21] did you have a .gitconfig already? [05:21] * davecheney searches [05:21] not the .gitconfig one, i couldn't replicate the problem [05:21] i just changed the tests to use a regexp [05:21] that'll do [05:21] that is what I did too [05:21] i have never used git before, and i can replicate it [05:21] go with your version [05:21] mine didn't work :) [05:21] i think git has a few ways of knowing your email address [05:22] wallyworld_: thing is [05:22] and since tim nor i have any of those, the test sfail [05:22] in a fresh raring vm [05:22] ther ewas not problem [05:22] hmmm, ok [05:22] i cannot explain what thumper did to his machine [05:22] and so was unable to fix it [05:22] i did a dist upgrade from quantal [05:22] anyways, adding set envs in the test setup fixes it [05:23] brave man [05:23] i like to live on the edge [08:01] morning all [08:45] dimitern, mgz, jtv1: mornings [08:46] fwereade: hey [08:46] dimitern, thanks for the 1152717 review, I've loosened it up a little with a more detailed explanation; about to repropose, would be grateful if you would think of ways to break it ;p [08:46] Hi fwereade === jtv1 is now known as jtv [08:47] dimitern, how's the upgrade-charm stuff going? [08:47] fwereade: will do :) [08:47] * jtv keeps misreading that nick as "dim intern" :( [08:47] haha [08:48] fwereade: i'll finish the dreaded 010-upgade-charm-... branch today I hope - still fiddling with tests [08:48] jtv: lol we're the first definitely [08:48] jtv: s/we/you/ [08:48] I find that hard to believe! [08:48] Maybe I'm just the first who dared to say it. [08:48] :D [08:49] dimitern, cool -- if the morning goes well I might be free to pair after lunch, if that would be useful to you [08:49] * jtv may be going just a little bit dyslexic. [08:50] fwereade: that will be best and fastest actually [08:50] dimitern, ok, great [08:50] fwereade: although it's still a bit of a mess at home, so maybe I can come to yours? [08:51] dimitern, surely, cath and laura will be out most of the afternoon I think [08:52] fwereade: sweet, just give me a shout then - we can probably have lunch @cuba or something as well [08:53] ...seems like a long way to go for lunch... [08:53] dimitern, when's your standup again? [08:53] mgz, ;p [08:53] fwereade: 12:30 [08:53] dimitern, ok, lunch at 1:30 then? [08:53] mgz: oh, it's maltaspace you know - everything it's but a wormhole away :D [08:53] fwereade: sgtm [08:56] dimitern, reproposed https://codereview.appspot.com/7591044 [08:56] fwereade: already looking [08:56] <3 [08:57] jtv: sorry about my bitching about using lbox propose - i didn't realise you forked the maas provider and you're working on it separately [09:02] fwereade: so if we have the same situation s.doc.UnitCount ==1 in local (stale) state, then $gt will fail and we'll retry [09:02] dimitern, yep [09:02] fwereade: looks ok to me [09:02] dimitern, cool [09:15] mornin' all [09:19] so, does anyone know what writes "/var/lib/juju/agents/bootstrap/agent.conf" ? I'm playing with the Windows port, and the bootstrap node fails to find that file. [09:20] I'm guessing it is a bootstrap => cloud-init issue. [09:23] fwereade: as part of the "pass series into environ", is Arch on the table for poking at? As it seems to only use the juju client Arch (amd64/i386) but I'm not sure that actually makes sense. [09:23] jam: yes, the cloudinit code should write that file [09:23] (we only have 64-bit tools available, so why not start a 64-bit instance always) [09:23] jam, arch will be incorporated alongside constraints [09:23] fwereade: can i presume that https://codereview.appspot.com/7809043 was instigated by you? [09:24] rogpeppe, well, we kinda agreed the business value was insufficient to make it a focus now, but thumper only remembered that when he'd done it [09:25] fwereade: the problem is that it means that it's a right pain if you want to bootstrap from Go code now [09:25] fwereade: for instance it breaks builddb [09:25] rogpeppe, and I'm +1 on little steps that make environs clearer and less weirdly coupled, so I think it's still a good move [09:25] fwereade: that was the main consideration behind the current design [09:25] rogpeppe, hmm [09:26] rogpeppe, builddb can't create a cert independently? [09:26] fwereade: it *could*, but then it would be duplicating the code in cmd/juju/bootstrap [09:27] fwereade: if nothing else, the code to generate certs appropriately for an environment should be factored out into environs, so it's at least available to other Go code that wants to bootstrap. [09:28] fwereade: FWIW i hammered this out for ages with gustavo [09:29] rogpeppe, I agree the cert generation should not be inside cmd/juju, but I think it's the appropriate place to invoke it [09:30] rogpeppe: so the environs.agent code appears to use "path.Join()" to do these sorts of operations, which means it is trying to shell out to stuff like "bootstrap\agents.conf" rather than "bootstrap/agents.conf" [09:30] rogpeppe, so keeping the functionality in environs sgtm, but I remain -1 on keeping it *inside* environs.Bootstrap [09:30] fwereade: yeah, i'm not intrinsically opposed to needing two steps to bootstrap (i think that was my favoured choice originally) [09:30] so.... sigh, lots of code that would act differently on Windows for bad reasons. [09:30] jam: you mean filepath.Join ? [09:30] dimitern: I guessed. :) We don't think of it as forking, really, more as a feature branch. Eventually we'll submit the whole thing for review. (We'll probably chop it into smaller pieces for easier reviewing) [09:31] rogpeppe: well explicitly path.Join [09:31] maybe that one is ok? [09:31] rogpeppe, incidentally, do you recall why builddb lacks tests? [09:31] jam: path.Join should never produce \ [09:31] fwereade: because it's a quick hack command that gustavo wrote [09:31] jtv: :) I see [09:32] rogpeppe, that's an important part of our infrastructure?part of our [09:32] * fwereade sighs gently [09:32] jam: i was originally trying to be very careful about path hygiene (path vs filepath) but got told that it wasn't worth it - we'd need another pass later, so, yes, everything will break under windows. [09:32] fwereade: it is? [09:32] fwereade: it just compiles mongo. [09:32] fwereade: tbh, it could be a regular charm. [09:32] rogpeppe, doesn't that qualify? [09:33] rogpeppe, true [09:33] fwereade: the only real test is running it. there's no input and only one possible output. [09:34] jam: your best bet is to grep for all occurrences of (path|filepath)\.Join and inspect on a case-by-case basis. [09:34] rogpeppe, weeeell... that'd be a test for the charm, right? [09:34] rogpeppe: well as a start, I need to figure out if this is exactly why it is failing [09:35] fwereade: yup. but it's not in the charm store. when would that test ever have run? [09:35] jam: and it's quite an expensive test (n hours on an ec2 instance) [09:35] rogpeppe: I think you meant fwereade :) [09:35] jam: i did :- [09:35] ) [09:35] rogpeppe, surely there are plenty of things that could be tested without actually running the charm? eg that the code compiles, that the charm is actually a valid charm, etc? [09:36] fwereade: possibly. i think there are more important considerations. if that command fails, it can be dealt with elsewhere. [09:37] jam: what failure are you currently seeing? [09:38] rogpeppe, if it's not important enough to test it's probably not important enough to exist then? [09:38] fwereade: probably [09:39] fwereade: i think gustavo wrote it as more of a proof of concept than anything else. [09:39] rogpeppe: looks like agent.tools is ok, but trivial.ShQuote is not [09:40] > '\var\lib\juju\agents\machine-0\agent.conf' [09:40] not sure what file that will actually create :) [09:40] rogpeppe, if you're reviewing that branch, maybe tell thumper to correspond with davecheney and figure out if it's ok to drop it? [09:40] fwereade: ok [09:40] rogpeppe, ideally mongossl ends up in cloudarchive anyway [09:41] nm, not ShQuote, but c.Dir() [09:41] jam: that's not a ShQuote issue. [09:41] maybe?? [09:41] rogpeppe: that is using path.Join() as near as I can tell [09:44] jam: [09:44] func (c *Conf) File(name string) string { [09:44] return filepath.Join(c.Dir(), name) [09:44] } [09:45] jam: to be honest, i think if we changed all filepath imports to use path, everything would probably work [09:46] rogpeppe: I haven't found a 'filepath' yet [09:46] I can see that c.Dir() is returning '/var/lib'... [09:46] jam: in environs/agent/agent.go [09:46] but by the time it gets put together into WriteCommands it is \var... [09:47] right, I guess that code is mixing "where will I read" with "where will I write" and not paying attention to the fact that "where will I write" is running on another OS [09:48] but as you say, / works on Windows anyway, and for now, the 'where will I read' is always *nix anyway [09:48] well prob always Ubuntu even [09:49] jam: for the time being, yes [09:49] rogpeppe: so changing that looks good so far, I have to wait for the instance to start up [09:55] how can I get the caller of a function at run time? [09:55] rogpeppe: and step 2, mongo doesn't start because the upstart file that gets written was using 'filepath' as well. :) [09:56] jam: just change "filepath" to "path" throughout the code [09:57] rogpeppe: ^^ ? [09:57] yeah, that's what I'm thinking as well [09:57] jam: there will be one or two places where it uses filepath.Walk, and you'll have to fix those, but i think everything else should probably just work [09:57] dimitern: we used that in the Hooks code for goose [09:57] dimitern: ? [09:57] dimitern: http://golang.org/pkg/runtime/ [09:57] runtime.Caller() [09:57] jam: oh, cool 10x [09:57] ah [09:58] dimitern: i use a little helper function that gives me the source locations of all callers, all formatted on a single line [09:58] rogpeppe: great! can I have this pls? [09:58] dimitern: yeah, one mo [09:59] I'm getting an obscure panic and trying to see which call caused it [10:00] dimitern: import "code.google.com/p/rog-go/exp/runtime/debug" [10:00] // Callers returns the stack trace of the goroutine that called it, [10:00] // starting n entries above the caller of Callers, as a space-separated list [10:00] // of filename:line-number pairs with no new lines. [10:00] func Callers(n, max int) []byte { [10:00] rogpeppe: tyvm [10:01] dimitern: BTW doesn't the panic give you a stack trace? [10:01] rogpeppe: it does, but it's in another goroutine and it's not helpful [10:01] dimitern: ah yes [10:01] dimitern: don't you get *all* stack traces? [10:02] rogpeppe: I do but it's still confusing - there are like 80 goroutines [10:03] dimitern: what i tend to do in that case is search for functions i'm interested in [10:04] dimitern: in acme (you can probably do something similar in your editor) i do ,x/(.+\n)*/v/function-i'm-interested-in/d [10:04] dimitern: which removes all stack traces that don't mention the given function name [10:04] rogpeppe: aha, I can try this [10:06] rogpeppe: actually it's still weird - it's something to do with the watchers - wanna take a look? [10:06] dimitern: sure - paste away. is this against trunk? [10:07] rogpeppe: no, against the branch I'm working on, but I merge trunk regularly (just did this morning) - http://paste.ubuntu.com/5613199/ [10:08] dimitern: can you reproduce the problem in trunk? [10:08] rogpeppe: no [10:08] rogpeppe: but then again the tests in the uniter/filter will be different [10:09] anyway I still don't get why "watcher was stopped cleanly" is a panic? [10:10] dimitern: if you get eof from a watcher, it should be because it stopped because of an error. [10:10] rogpeppe: istm there's no way to stop a watcher without a panic [10:10] dimitern: the only time it there's no error is when the watcher was stopped deliberately [10:11] rogpeppe: that is, if you then call musterr [10:11] and I still don't get how this the the only place where err == nil is a panic [10:11] dimitern: musterr is used when we know that the watcher is never stopped. [10:12] dimitern: can you push your branch? [10:12] rogpeppe: ok [10:13] rogpeppe: lp:~dimitern/juju-core/010-uniter-handle-config-upgrades [10:14] dimitern: from the look of those stack traces, *loads* of watchers have been stopped unexpectedly. [10:14] dimitern: (look at all the calls to MustErr that are in progress) [10:14] rogpeppe: I saw that, but still no clue what i did wrong [10:14] dimitern: looks like you might not be cleaning up properly or something [10:15] dimitern: which test fails? [10:15] rogpeppe: not, sure let me run it with -vv [10:16] rogpeppe: START: context.go:0: FilterSuite.TearDownTest, just after that is the panic [10:16] dimitern: ah yes, i just worked that out [10:17] dimitern: ah, i see the problem [10:18] dimitern: you're not deferring f.Stop [10:18] rogpeppe: I can't see the difference, since it's near the end anyway [10:19] rogpeppe: no, what - which file/line? [10:19] dimitern: well, it fixes your panic [10:20] rogpeppe: filter_test.go:300 ? [10:20] dimitern: no :243 [10:20] dimitern: just after newFilter, in TestConfigEvents [10:21] rogpeppe: I'm not, but I'm calling f.Stop() explicitly later [10:21] dimitern: yes, but you're getting a test failure which means it never gets called [10:21] rogpeppe: right! [10:21] rogpeppe: I was thinking something like that [10:21] rogpeppe: anyway, 10x :) [10:22] dimitern: np. BTW, i'm not entirely sure why we get the error we do (i'd have thought that we'd get an error when the state is shut down underneath us) [10:23] rogpeppe: yeah, the fix is easy, but understanding the error is hard :) [10:24] dimitern: feel free to dig in and improve it. you'll get a better understanding of what's going on too if you do :-) [10:25] rogpeppe: once I get it I will :) [10:25] dimitern: perhaps try to write a little piece of code that reproduces the problem [10:27] rogpeppe: yeah, i have something in mind already [10:30] fwereade: can you take a look please - https://codereview.appspot.com/7425044 - the tests now pass, but I'm surely missing something? [10:31] dimitern, looking [10:54] dimitern, looking pretty good, sent some more comments -- should be able to polish it off today [10:54] fwereade: great to hear! [11:31] mgz: standup? [11:36] ta [12:05] system hung up on me [12:06] fwereade: any chance you could have a look at these CLs? https://codereview.appspot.com/7727044/ https://codereview.appspot.com/7727045/ [12:08] (or anyone else, for that matter (thanks already dimitern!)) [12:08] rogpeppe: if you stop the watcher, won't you get this when trying to read on the channel? [12:09] dimitern: get what? [12:09] rogpeppe: https://codereview.appspot.com/7727045/diff/5001/state/megawatcher.go#newcode105 [12:09] rogpeppe: detect the watcher was stopped [12:09] dimitern: sorry, i'm not sure i understand the question [12:10] rogpeppe: I mean having reply chan struct{} instead [12:10] rogpeppe, looking [12:11] rogpeppe: you mean having a bool instead gives you the possibility to send 2 distinct msgs: watcher closed and ? [12:11] dimitern: the allWatcher replies with either true (request accepted) or false (the StateWatcher has been stopped) [12:12] rogpeppe: ah, I see [12:12] dimitern: actually true means "request has been processed, and now contains the reply" [12:12] rogpeppe: thanks for clarifying this [12:12] dimitern: np. i'll try to clarify the comments appropriately. [12:12] fwereade: thanks [12:22] rogpeppe, sorry I didn't finish those yesterday -- both LGTM [12:22] fwereade: cool, thanks [12:23] * dimitern lunch [12:26] rogpeppe, if you have a mo I have https://codereview.appspot.com/7591044/ and https://codereview.appspot.com/7715046/ out for review [12:26] * fwereade lunch [12:26] fwereade: ok, thanks, will have a look in a bit === benji___ is now known as benji [12:54] When you guys get a chance I have a small branch up for review: https://codereview.appspot.com/7554046/ === wedgwood_away is now known as wedgwood [14:02] fwereade, mramm: hangout? [14:16] rogpeppe: sorry, did not wake up for the alarm [14:17] mramm, we're pretty much done actually [14:17] mramm: we're just about done, but still there if you want to join [14:21] seems like you finished before I got there... :) === teknico_ is now known as teknico [14:37] rogpeppe, https://codereview.appspot.com/7755045 might be trivial -- it's certainly small [14:39] fwereade: not trivial, i think - i'm having to think a bit [14:40] rogpeppe, ok, definitely not a priority then, I'll leve it lying around unreferenced in kanban and just drop it in a few days unless someone really likes it [14:41] fwereade: i *think* it's right, but there might have been a good reason i didn't do that before (rather than just not thinking of it) [14:42] rogpeppe, one thing that crosses my mind is that the highest-version behaviour in environs is incompatible -- the highest version (as deployed) will (surely?) be immediately downgraded to agent-version once an agent starts running [14:44] fwereade: i don't think so [14:44] fwereade: note line 177 [14:45] rogpeppe, not following -- `binary.Number = vers`? [14:45] fwereade: yeah [14:45] rogpeppe, I'm probably being thick, please explain [14:46] fwereade: ah, i might have been misreading your remark [14:46] rogpeppe, ISTM that environs gets the highest compatible tools and deploys with those [14:46] rogpeppe, but that the agent that then runs checks for the agent-version in env config [14:47] fwereade: i can't remember where agent-version gets set originally. [14:47] rogpeppe, and up/down/whatever-grades to that immediately [14:47] rogpeppe, hmm, good question [14:47] fwereade: in general, if agent version is set, we want to use that version. [14:48] rogpeppe, tools we bootstrp with, or defaults to version.Current [14:48] rogpeppe, I think :) [14:49] fwereade: agent-version is important - that's how we have any control over what version the agents are running [14:49] rogpeppe, indeed [14:50] rogpeppe, I think environs should be a bit more careful with it [14:50] fwereade: i'm thinking that the tools selection logic ... yeah [14:50] rogpeppe, but I think thumper's heading in that direction anyway [14:52] fwereade: AgentVersion defaults to the current version, BTW [14:53] rogpeppe, I think I said that [14:59] * rogpeppe continues to study the logic [15:06] fwereade: i *think* the provisioner should pass the current agent version into StartInstance [15:06] fwereade: (optionally) [15:07] rogpeppe, +-1, I'm not sure any of that stuff is the provisioner's business [15:07] rogpeppe, if anything should know the env config it's the env ;) [15:07] fwereade: hmm, good point, the environ should already *know* the agent version! [15:08] fwereade: but currently it can't tell if it's been set explicitly [15:09] fwereade: because i think that only if it's not set explicitly should the environ pass HighestVersion in the FindTools flags === teknico_ is now known as teknico [15:29] fwereade: last thing you saw? [15:30] rogpeppe, I said "although I'm now less inclined to take a hard line on that -- if the provisioner were to be running a locatoralike, that would probably be best" [15:30] fwereade: ah, last thing i saw was: [15:30] [15:07:15] rogpeppe, if anything should know the env config it's the env ;) [15:30] fwereade: last thing you saw from me? [15:30] rogpeppe, in between the two I said " just like the state/pi info we pass in" [15:31] rogpeppe, nothing from you since "(optionally)" [15:31] [15:07:50] fwereade: hmm, good point, the environ should already *know* the agent version! [15:31] [15:08:05] --> teknico_ has joined this channel (~quassel@93-42-34-107.ip84.fastwebnet.it). [15:31] [15:08:24] fwereade: but currently it can't tell if it's been set explicitly [15:31] [15:09:19] fwereade: because i think that only if it's not set explicitly should the environ pass HighestVersion in the FindTools flags [15:32] rogpeppe, hmm, yeah, maybe === teknico_ is now known as teknico [15:32] rogpeppe, I'm not totally wild about the "highest" behaviour because of the conflict with what the agent upgrader does [15:32] rogpeppe, "highest" STM like a good default for what to set it to when we do upgrde-juju [15:32] fwereade: i think highest is fine when deploying without a specified version [15:34] fwereade: if the agent version is not set, that is [15:34] rogpeppe, hmm, ok, if there's nothing in agent-version at *all*, that could make sense [15:34] rogpeppe, but is there ever such a situation? [15:34] fwereade: that is almost always the case currently [15:34] rogpeppe, I think we set one up at bootstrp time if none is set [15:34] fwereade: i think [15:35] rogpeppe, environs/config.go:179 [15:35] rogpeppe, seems strange not to aim to have everything on the same version in general [15:35] fwereade: yeah, i'm not sure about that. [15:35] fwereade: the main case for not doing so is when bootstrapping [15:36] rogpeppe, a random smorgasbord of versions, until one is set explicitly, seems more likely to confuse and upset than nything else [15:36] rogpeppe, I'd be fine with bootstrapping to the highest available when not otherwise set, I think [15:36] fwereade: i think if you bootstrap, you should get the latest version, regardless of the client [15:36] rogpeppe, ok, sgtm [15:37] fwereade: so this brings us back to Bootstrap needing to know if the agent version is explicitly set or not [15:39] fwereade: i *think* i'd be ok if config.Config never looked at version.Current. then all agent-version setting is done explicitly where necessary. [15:40] fwereade: and then perhaps upgrader would just do nothing if agent-version was not set [15:41] fwereade: although that should never happen in practice. [15:44] fwereade: gone again? [15:48] rogpeppe, sorry, only half looking t my own screen [15:49] fwereade: np [15:49] rogpeppe, yeah, I think I'm ok with that [15:49] rogpeppe, I agree that the upgrader should never see a missing agent-version [15:49] fwereade: BTW i'd like to know your thoughts on https://codereview.appspot.com/7554046/diff/1/state/apiserver/api_test.go#newcode404 [15:50] * fwereade looks [15:52] rogpeppe, heh, tricky [15:52] rogpeppe, no, wait, we have a state.State [15:52] rogpeppe, we can implement the test by adding via state and trying to remove via api [15:52] rogpeppe, yesno? [15:53] rogpeppe, I'm -1 on dropping those tests, I think they will become very meaningful again when we start doing the internal API [15:54] fwereade: i don't want to drop all those tests, just the Client ones. [15:54] fwereade: and if we ever had any other logic behind them (say different users could do different things), we'd need more again. [15:55] fwereade: i'd drop all the Client tests except one [15:57] rogpeppe, I think there's still value in checking that all the client methods work the same [15:58] fwereade: ok. but we really are just testing that single "if" statement in every one of them, because they're all gated through that method. [15:58] rogpeppe, even if it's obvious from the implementation that they do, that only applies today and not in the face of unknowable future changes [15:58] fwereade: if those changes come, we'll be needing to change the tests anyway. [15:58] rogpeppe, I have a great fondness for the refactor-with-axe, run-tests, see-what-failed approach [15:59] rogpeppe, if we keep those tests then when I do that I can at least see that the bits of implementation that are important enough to care about still act as they did before [16:02] fwereade: FWIW dropping all but one of those tests shaves 8s off the api test runtime [16:02] rogpeppe, that is to me an argument for making them faster, not for just dropping them ;p [16:02] fwereade: do you see any way we can make them faster? [16:03] fwereade: fundamentally we are pretty slow at mutating state [16:04] fwereade: perhaps there's a magic switch we haven't given to mongod [16:04] rogpeppe, tbh, no, nothing that isn't deeply hackish [16:04] rogpeppe, (magically poke everything into state at once!) [16:04] fwereade: i don't think that'll help here. [16:05] fwereade: we're not spending the time in setUpScenario [16:05] fwereade: (even if we could do that, of course) [16:05] rogpeppe, ah, ok, I thought that was where you'd had problems in the past [16:05] rogpeppe, I bet we *could*, but I'm petty sure it'd be a bad idea [16:05] fwereade: yeah, that's why i took it out of the loop and made functions return an "undo" function [16:06] fwereade: if we were doing setUpScenario, the tests would take ages and ages to run [16:06] rogpeppe, if the problem is the operations themselves then I don't see a way out then [16:07] fwereade: indeed. that's why i think that removing most of the tests which aren't actually testing any existing logic is a reasonable way forward. [16:08] rogpeppe, I dunno, I've been bitten before by changing stuff and assuming that the lack of test failures indicated smart tests and a good change [16:08] rogpeppe, when in fact it was just that nobody had written them in the first place [16:09] fwereade: to change this behaviour, we'd have to change the Client method - if we put a comment there, i think we'd be ok. [16:13] fwereade: BTW, if we were running setUpScenario each time, it would add about 1 [16:13] 3 minutes to the test time [16:13] (that's 3, not 13) [16:14] rogpeppe, ouch :/ [16:15] rogpeppe, still, if we can't test authorization behaviour directly I think we have no choice but to do so indirectly [16:16] rogpeppe, and suck up the costs [16:16] fwereade: actually, we *could* theoretically provide an entry point into rpc that allowed testing access to a particular rpc object without actually calling a method on that object. [16:17] fwereade: i'd much prefer it if we could make operations on the state faster though. [16:18] fwereade: so we wouldn't be worrying about this. [16:19] rogpeppe, +1 in at least theory, but I'm hoping to hear something about things that hurt real-world performance from dave before I fret too much in that direction [16:20] fwereade: i guess so. it might takes 15 minutes just to add 10000 units to state, it's probably not that huge a deal [16:20] s/takes/take/ [16:20] s/it's/but it's/ [16:21] rogpeppe, yeah, we shall see the actual numbers [16:21] fwereade: well, you can get a best-case scenario by simply calling AddUnit 10000 times on a local state server. [16:22] fwereade: i suspect it'll be about 10-15 minutes to do that [16:58] rogpeppe, I'm about to head off -- if you have a chance to take a look at https://codereview.appspot.com/7715046/ and/or https://codereview.appspot.com/7591044/ I would be most grateful [16:58] fwereade: ok, will do [16:59] rogpeppe, cheers [16:59] gn all [19:37] right, that's me for the day. good night. === _thumper_ is now known as thumper [21:10]