local: fix port conflict in test

local: fix port conflict in test

Starting and stopping the listener during the test may
lead to port conflicts. Using two functions for getting
a new one and retrieving the actual one avoids this
failure.

https://code.launchpad.net/~themue/juju-core/015-fix-local-storage-port/+merge/151022

(do not edit description out of merge proposal)

[TheMue, 2013-02-28 13:50:27]

Please take a look.

[TheMue, 2013-02-28 14:26:10]

Please take a look.

[dimitern, 2013-02-28 15:10:42]

LGTM modulo the below.

https://codereview.appspot.com/7423045/diff/3001/environs/local/backend_test.go
File environs/local/backend_test.go (right):

https://codereview.appspot.com/7423045/diff/3001/environs/local/backend_test....
environs/local/backend_test.go:113: portNo := nextPortNo()
This code is repeated several times. Maybe add it to a helper method, returning
dataDir and use that?

https://codereview.appspot.com/7423045/diff/3001/environs/local/backend_test....
environs/local/backend_test.go:187: portNo := nextPortNo()
ditto

https://codereview.appspot.com/7423045/diff/3001/environs/local/backend_test....
environs/local/backend_test.go:237: portNo := nextPortNo()
ditto

https://codereview.appspot.com/7423045/diff/3001/environs/local/backend_test....
environs/local/backend_test.go:294: portNo := nextPortNo()
ditto

[TheMue, 2013-02-28 16:51:53]

Please take a look.

https://codereview.appspot.com/7423045/diff/3001/environs/local/backend_test.go
File environs/local/backend_test.go (right):

https://codereview.appspot.com/7423045/diff/3001/environs/local/backend_test....
environs/local/backend_test.go:113: portNo := nextPortNo()
On 2013/02/28 15:10:42, dimitern wrote:
> This code is repeated several times. Maybe add it to a helper method,
returning
> dataDir and use that?

Good idea, thanks.

https://codereview.appspot.com/7423045/diff/3001/environs/local/backend_test....
environs/local/backend_test.go:187: portNo := nextPortNo()
On 2013/02/28 15:10:42, dimitern wrote:
> ditto

Done.

https://codereview.appspot.com/7423045/diff/3001/environs/local/backend_test....
environs/local/backend_test.go:237: portNo := nextPortNo()
On 2013/02/28 15:10:42, dimitern wrote:
> ditto

Done.

https://codereview.appspot.com/7423045/diff/3001/environs/local/backend_test....
environs/local/backend_test.go:294: portNo := nextPortNo()
On 2013/02/28 15:10:42, dimitern wrote:
> ditto

Done.

[dimitern, 2013-02-28 16:54:03]

Thank you, LGTM.

https://codereview.appspot.com/7423045/diff/7001/environs/local/backend_test.go
File environs/local/backend_test.go (right):

https://codereview.appspot.com/7423045/diff/7001/environs/local/backend_test....
environs/local/backend_test.go:34: // nextTestSet returns a new port number,
listener and data directory.
Even better, thanks!

[gz, 2013-03-08 15:22:55]

This is an improvement if it makes the tests pass more, but seems like this is
still not quite as robust as it should be.

https://codereview.appspot.com/7423045/diff/7001/environs/local/backend_test.go
File environs/local/backend_test.go (right):

https://codereview.appspot.com/7423045/diff/7001/environs/local/backend_test....
environs/local/backend_test.go:31: currentPortNo int = 60005
Why are we using an explicit port number at all, rather than creating the
connection passing port 0 then using the randomly assigned port?

Using specific port number values in tests will always be fragile as they may
already be in use.

[TheMue, 2013-03-08 16:27:38]

Please take a look.

[gz, 2013-03-08 16:51:55]

LGTM with one minor tidy up.

https://codereview.appspot.com/7423045/diff/12001/environs/local/backend_test.go
File environs/local/backend_test.go (right):

https://codereview.appspot.com/7423045/diff/12001/environs/local/backend_test...
environs/local/backend_test.go:43: c.Assert(err, IsNil)
In testing/http.go this logic is handled by:

  port := l.Addr().(*net.TCPAddr).Port

That seems a little neater to me.

[TheMue, 2013-03-08 17:36:31]

Please take a look.

[TheMue, 2013-03-08 19:09:32]

*** Submitted:

local: fix port conflict in test

Starting and stopping the listener during the test may
lead to port conflicts. Using two functions for getting
a new one and retrieving the actual one avoids this
failure.

R=dimitern, gz
CC=
https://codereview.appspot.com/7423045

[dave_cheney.net, 2013-03-08 19:42:58]

I'd like to see all the string mangling replaced with net.Listener and net.Addr
types. Otherwise it looks pretty good too me.

https://codereview.appspot.com/7423045/diff/12001/environs/local/backend_test.go
File environs/local/backend_test.go (right):

https://codereview.appspot.com/7423045/diff/12001/environs/local/backend_test...
environs/local/backend_test.go:40: _, ports, err :=
net.SplitHostPort(listener.Addr().String())
if local.Listen returns a net.Listener which we know is a *net.TCPListener, then
you can make this simpler by saying

port := listener.Addr().(*net.TCPAddr).Port

https://codereview.appspot.com/7423045/diff/12001/environs/local/storage_test.go
File environs/local/storage_test.go (right):

https://codereview.appspot.com/7423045/diff/12001/environs/local/storage_test...
environs/local/storage_test.go:22: storage := local.NewStorage("127.0.0.1",
portNo)
why does NewStorage not take a net.Addr ?