Merge lp:~thumper/juju-core/client-test-no-fixed-port into lp:~go-bot/juju-core/trunk

[Tim Penhey]

Make the test not use a fixed port.

Simple change to avoid a fixed port in the test.

https://codereview.appspot.com/82940044/

Make the test not use a fixed port.

Simple change to avoid a fixed port in the test.

https://codereview.appspot.com/82940044/

[Tim Penhey (thumper)]

Reviewers: mp+213582_code.launchpad.net,

Message:
Please take a look.

Description:
Make the test not use a fixed port.

Simple change to avoid a fixed port in the test.

https://code.launchpad.net/~thumper/juju-core/client-test-no-fixed-port/+merge/213582

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/82940044/

Affected files (+9, -2 lines):
   A [revision details]
   M state/api/client_test.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20140331192817-ytfa2vb3oz21cj4e
+New revision: <email address hidden>

Index: state/api/client_test.go
=== modified file 'state/api/client_test.go'
--- state/api/client_test.go 2014-03-13 07:54:56 +0000
+++ state/api/client_test.go 2014-04-01 04:30:11 +0000
@@ -5,6 +5,7 @@

  import (
   "fmt"
+ "net"
   "net/http"

   jc "github.com/juju/testing/checkers"
@@ -65,17 +66,21 @@
   // Finally, try the NotImplementedError by mocking the server
   // address to a handler that returns 405 Method Not Allowed for
   // POST.
+ lis, err := net.Listen("tcp", ":0")
+ c.Assert(err, gc.IsNil)
+ port := lis.Addr().(*net.TCPAddr).Port
+ url := fmt.Sprintf("http://localhost:%d", port)
   http.HandleFunc("/charms", func(w http.ResponseWriter, r *http.Request) {
    if r.Method == "POST" {
     http.Error(w, "Method Not Allowed", http.StatusMethodNotAllowed)
    }
   })
   go func() {
- err = http.ListenAndServe(":8900", nil)
+ err = http.Serve(lis, nil)
    c.Assert(err, gc.IsNil)
   }()

- api.SetServerRoot(client, "http://localhost:8900")
+ api.SetServerRoot(client, url)
   _, err = client.AddLocalCharm(curl, charmArchive)
   c.Assert(err, jc.Satisfies, params.IsCodeNotImplemented)
  }

[Andrew Wilkins (axwalk)]

LGTM

https://codereview.appspot.com/82940044/diff/1/state/api/client_test.go
File state/api/client_test.go (right):

https://codereview.appspot.com/82940044/diff/1/state/api/client_test.go#newcode72
state/api/client_test.go:72: url := fmt.Sprintf("http://localhost:%d",
port)
This can be simplified to:
     fmt.Sprintf("http://%s", lis.Addr())

https://codereview.appspot.com/82940044/

[Andrew Wilkins (axwalk)]

https://codereview.appspot.com/82940044/diff/1/state/api/client_test.go
File state/api/client_test.go (right):

https://codereview.appspot.com/82940044/diff/1/state/api/client_test.go#newcode69
state/api/client_test.go:69: lis, err := net.Listen("tcp", ":0")
Sorry, just realised: please defer lis.Close() after checking the error.

https://codereview.appspot.com/82940044/

[Tim Penhey (thumper)]

Please take a look.

https://codereview.appspot.com/82940044/diff/1/state/api/client_test.go
File state/api/client_test.go (left):

https://codereview.appspot.com/82940044/diff/1/state/api/client_test.go#oldcode75
state/api/client_test.go:75: c.Assert(err, gc.IsNil)
Had to remove the assert as it now returned a "use of closed connection"
error, and we don't actually care about the error.

https://codereview.appspot.com/82940044/diff/1/state/api/client_test.go
File state/api/client_test.go (right):

https://codereview.appspot.com/82940044/diff/1/state/api/client_test.go#newcode69
state/api/client_test.go:69: lis, err := net.Listen("tcp", ":0")
On 2014/04/01 04:48:36, axw wrote:
> Sorry, just realised: please defer lis.Close() after checking the
error.

ok

https://codereview.appspot.com/82940044/diff/1/state/api/client_test.go#newcode72
state/api/client_test.go:72: url := fmt.Sprintf("http://localhost:%d",
port)
On 2014/04/01 04:47:19, axw wrote:
> This can be simplified to:
> fmt.Sprintf("http://%s", lis.Addr())

Done.

https://codereview.appspot.com/82940044/

[Go Bot (go-bot)]

The attempt to merge lp:~thumper/juju-core/client-test-no-fixed-port into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core 0.014s
ok launchpad.net/juju-core/agent 1.135s
ok launchpad.net/juju-core/agent/mongo 0.507s
ok launchpad.net/juju-core/agent/tools 0.198s
ok launchpad.net/juju-core/bzr 4.900s
ok launchpad.net/juju-core/cert 2.719s
ok launchpad.net/juju-core/charm 0.372s
? launchpad.net/juju-core/charm/hooks [no test files]
? launchpad.net/juju-core/charm/testing [no test files]
ok launchpad.net/juju-core/cloudinit 0.030s
ok launchpad.net/juju-core/cloudinit/sshinit 0.755s
ok launchpad.net/juju-core/cmd 0.182s
ok launchpad.net/juju-core/cmd/charm-admin 0.729s
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 201.516s
ok launchpad.net/juju-core/cmd/jujud 64.914s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 7.890s
? launchpad.net/juju-core/cmd/plugins/juju-restore [no test files]
ok launchpad.net/juju-core/cmd/plugins/local 0.168s
? launchpad.net/juju-core/cmd/plugins/local/juju-local [no test files]
ok launchpad.net/juju-core/constraints 0.023s
ok launchpad.net/juju-core/container 0.033s
ok launchpad.net/juju-core/container/factory 0.035s
ok launchpad.net/juju-core/container/kvm 0.237s
ok launchpad.net/juju-core/container/kvm/mock 0.035s
? launchpad.net/juju-core/container/kvm/testing [no test files]
ok launchpad.net/juju-core/container/lxc 4.280s
? launchpad.net/juju-core/container/lxc/mock [no test files]
? launchpad.net/juju-core/container/lxc/testing [no test files]
? launchpad.net/juju-core/container/testing [no test files]
ok launchpad.net/juju-core/downloader 5.215s
ok launchpad.net/juju-core/environs 2.272s
ok launchpad.net/juju-core/environs/bootstrap 10.280s
ok launchpad.net/juju-core/environs/cloudinit 0.475s
ok launchpad.net/juju-core/environs/config 3.269s
ok launchpad.net/juju-core/environs/configstore 0.029s
ok launchpad.net/juju-core/environs/filestorage 0.024s
ok launchpad.net/juju-core/environs/httpstorage 0.619s
ok launchpad.net/juju-core/environs/imagemetadata 0.453s
? launchpad.net/juju-core/environs/imagemetadata/testing [no test files]
ok launchpad.net/juju-core/environs/instances 0.036s
ok launchpad.net/juju-core/environs/jujutest 0.196s
ok launchpad.net/juju-core/environs/manual 14.422s
ok launchpad.net/juju-core/environs/simplestreams 0.327s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 0.998s
ok launchpad.net/juju-core/environs/storage 1.040s
ok launchpad.net/juju-core/environs/sync 43.084s
ok launchpad.net/juju-core/environs/testing 0.146s
ok launchpad.net/juju-core/environs/tools 4.775s
? launchpad.net/juju-core/environs/tools/testing [no test files]
ok launchpad.net/juju-core/errors 0.013s
ok launchpad.net/juju-core/instance 0.021s
? launchpad.net/juju-core/instance/testing [no test files]
ok launchpad.net/juju-core/juju 20.353s
ok launchpad.net/juju-core/juju/arch 0....

[Go Bot (go-bot)]

The attempt to merge lp:~thumper/juju-core/client-test-no-fixed-port into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core 0.014s
ok launchpad.net/juju-core/agent 1.089s
ok launchpad.net/juju-core/agent/mongo 0.523s
ok launchpad.net/juju-core/agent/tools 0.187s
ok launchpad.net/juju-core/bzr 5.388s
ok launchpad.net/juju-core/cert 2.988s
ok launchpad.net/juju-core/charm 0.433s
? launchpad.net/juju-core/charm/hooks [no test files]
? launchpad.net/juju-core/charm/testing [no test files]
ok launchpad.net/juju-core/cloudinit 0.031s
ok launchpad.net/juju-core/cloudinit/sshinit 0.861s
ok launchpad.net/juju-core/cmd 0.163s
ok launchpad.net/juju-core/cmd/charm-admin 0.743s
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]

warning: building out-of-date packages:
 launchpad.net/goamz/ec2/ec2test
 launchpad.net/goamz/s3/s3test
 github.com/joyent/gomanta/localservices/hook
 github.com/joyent/gomanta/localservices
 github.com/joyent/gomanta/localservices/manta
 github.com/joyent/gosdc/localservices/hook
 github.com/joyent/gosdc/localservices
 github.com/joyent/gosdc/localservices/cloudapi
 launchpad.net/goose/testservices/hook
 launchpad.net/goose/testservices/identityservice
 launchpad.net/goose/testservices
 launchpad.net/goose/testservices/novaservice
 launchpad.net/goose/testservices/swiftservice
 launchpad.net/goose/testservices/openstackservice
installing these packages with 'go test -i ./...' will speed future tests.

=== modified file 'state/api/client_test.go'
--- state/api/client_test.go	2014-03-13 07:54:56 +0000
+++ state/api/client_test.go	2014-04-01 05:39:15 +0000
@@ -5,6 +5,7 @@
 
 import (
 	"fmt"
+	"net"
 	"net/http"
 
 	jc "github.com/juju/testing/checkers"
@@ -65,17 +66,20 @@
 	// Finally, try the NotImplementedError by mocking the server
 	// address to a handler that returns 405 Method Not Allowed for
 	// POST.
+	lis, err := net.Listen("tcp", ":0")
+	c.Assert(err, gc.IsNil)
+	defer lis.Close()
+	url := fmt.Sprintf("http://%v", lis.Addr())
 	http.HandleFunc("/charms", func(w http.ResponseWriter, r *http.Request) {
 		if r.Method == "POST" {
 			http.Error(w, "Method Not Allowed", http.StatusMethodNotAllowed)
 		}
 	})
 	go func() {
-		err = http.ListenAndServe(":8900", nil)
-		c.Assert(err, gc.IsNil)
+		http.Serve(lis, nil)
 	}()
 
-	api.SetServerRoot(client, "http://localhost:8900")
+	api.SetServerRoot(client, url)
 	_, err = client.AddLocalCharm(curl, charmArchive)
 	c.Assert(err, jc.Satisfies, params.IsCodeNotImplemented)
 }