Added test for missing API params #107
Conversation
…rams # By Ian Booth # Via Ian Booth (1) and Juju bot (1) * 'master' of https://github.com/juju/juju: Tweak hpcloud boilerplate to turn floating-ip and remove az from example region
…rams # By Dave Cheney (6) and others # Via Juju bot (7) and others * 'master' of https://github.com/juju/juju: Add additional test Make tests easier to follow We shouldn't attempt to UTC a zero time. provider/azure: don't fail Destroy if AG/vnet can't be destroyed Add ResourceCatalog implementation Close the state connection, and run the tests. Handle constrained AZs fixups after merge from trunk update to latest juju/names Bug #1328716: Clean up CONTRIBUTING a bit. state/presence: fixing issues from review presencer: make the presencer interface official state/machine: check IsManager and only Sync presence watcher state/machine: StartSync in SetAgentPresence state/machine: use WaitAgentPresence, rename AgentAlive Final tweak Update for juju/names part 3 wip Introduce Parse{Unit,Service,Machine}
| c.Assert(err, gc.IsNil) | ||
|
|
||
| // Have to wait until root is registered. | ||
| <-s.ready |
There was a problem hiding this comment.
So this structure certainly makes me wonder if how the handler is defined is correct.
Looking at the code state/apiserver/apiserver.go I think the trick is that you have to call Serve() before you call Start(). That way there isn't anything responding on the websocket before you have a service that you are serving.
So probably you can remove the channel checks and just restructure your setup func as:
conn.Serve(&DispatchRoot(), nil)
conn.Start()
Certainly if you were actually using a real HTTP website, you would never have that channel to be able to indicate "ok, you've connected to my website, but give me 20ms to actually have the connection ready for you".
|
I think if we tweak the ordering of Start() and Serve() so that we don't have to have the separate channel to indicate you can actually make requests, then LGTM. |
…re need for a control channel
|
|
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Merging failed |
|
|
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
Added test for missing API params Test has been written to check for lp:1152393, but that has been an already fixed duplicate. We still add this little test to explicitely show that the API handles missing params well without closing the websocket.
Test has been written to check for lp:1152393, but that has been an already fixed duplicate. We still add this little test to explicitely show that the API handles missing params well without closing the websocket.