address review

sdg0919 · Aug 18, 2012 · 1df2267 · 1df2267
1 parent 27a0833
commit 1df2267
Show file tree

Hide file tree

Showing 12 changed files with 112 additions and 181 deletions.
diff --git a/cmd/jujud/upgrade.go b/cmd/jujud/upgrade.go
@@ -121,7 +121,7 @@ func (u *Upgrader) run() error {
 				// The tools have already been downloaded, so use them.
 				return &UpgradedError{tools}
 			}
-			download = downloader.New(tools.URL)
+			download = downloader.New(tools.URL, "")
 			downloadTools = tools
 			downloadDone = download.Done()
 		case status := <-downloadDone:

diff --git a/downloader/downloader.go b/downloader/downloader.go
@@ -10,10 +10,6 @@ import (
 	"os"
 )
 
-// TempDir holds the temporary directory used to
-// write the URL download.
-var TempDir = os.TempDir()
-
 // Status represents the status of a completed download.
 type Status struct {
 	// File holds the downloaded data on success.
@@ -29,12 +25,12 @@ type Download struct {
 }
 
 // New returns a new Download instance downloading
-// from the given URL.
-func New(url string) *Download {
+// from the given URL to the given directory.
+func New(url, dir string) *Download {
 	d := &Download{
 		done: make(chan Status),
 	}
-	go d.run(url)
+	go d.run(url, dir)
 	return d
 }
 
@@ -51,9 +47,9 @@ func (d *Download) Done() <-chan Status {
 	return d.done
 }
 
-func (d *Download) run(url string) {
+func (d *Download) run(url, dir string) {
 	defer d.tomb.Done()
-	file, err := download(url)
+	file, err := download(url, dir)
 	if err != nil {
 		err = fmt.Errorf("cannot download %q: %v", url, err)
 	}
@@ -68,8 +64,11 @@ func (d *Download) run(url string) {
 	}
 }
 
-func download(url string) (file *os.File, err error) {
-	tempFile, err := ioutil.TempFile(TempDir, "inprogress-")
+func download(url, dir string) (file *os.File, err error) {
+	if dir == "" {
+		dir = os.TempDir()
+	}
+	tempFile, err := ioutil.TempFile(dir, "inprogress-")
 	if err != nil {
 		return nil, err
 	}

diff --git a/downloader/downloader_test.go b/downloader/downloader_test.go
@@ -12,6 +12,7 @@ import (
 )
 
 type suite struct {
+	testing.HTTPSuite
 	testing.LoggingSuite
 }
 
@@ -21,56 +22,39 @@ func Test(t *stdtesting.T) {
 	TestingT(t)
 }
 
-func (s *suite) SetUpTest(c *C) {
-	downloader.TempDir = c.MkDir()
-}
-
-func (s *suite) TearDownTest(c *C) {
-	downloader.TempDir = os.TempDir()
-}
-
 func (s *suite) TestDownload(c *C) {
-	l := testing.NewServer()
-	defer l.Close()
-
-	url := l.AddContent("/archive.tgz", []byte("archive"))
-	d := downloader.New(url)
+	tmp := c.MkDir()
+	testing.Server.Response(200, nil, []byte("archive"))
+	d := downloader.New(s.URL("/archive.tgz"), tmp)
 	status := <-d.Done()
 	c.Assert(status.Err, IsNil)
 	c.Assert(status.File, NotNil)
 	defer os.Remove(status.File.Name())
 	defer status.File.Close()
 
 	dir, _ := filepath.Split(status.File.Name())
-	c.Assert(filepath.Clean(dir), Equals, downloader.TempDir)
+	c.Assert(filepath.Clean(dir), Equals, tmp)
 	assertFileContents(c, status.File, "archive")
 }
 
 func (s *suite) TestDownloadError(c *C) {
-	l := testing.NewServer()
-	defer l.Close()
-	// Add some content, then delete it - we should
-	// get a 404 response.
-	url := l.AddContent("/archive.tgz", nil)
-	l.RemoveContent("/archive.tgz")
-	d := downloader.New(url)
+	testing.Server.Response(404, nil, nil)
+	d := downloader.New(s.URL("/archive.tgz"), c.MkDir())
 	status := <-d.Done()
 	c.Assert(status.File, IsNil)
 	c.Assert(status.Err, ErrorMatches, `cannot download ".*": bad http response: 404 Not Found`)
 }
 
 func (s *suite) TestStopDownload(c *C) {
-	l := testing.NewServer()
-	defer l.Close()
-	url := l.AddContent("/x.tgz", []byte("content"))
-	d := downloader.New(url)
+	tmp := c.MkDir()
+	d := downloader.New(s.URL("/x.tgz"), tmp)
 	d.Stop()
 	select {
 	case status := <-d.Done():
 		c.Fatalf("received status %#v after stop", status)
 	case <-time.After(100 * time.Millisecond):
 	}
-	infos, err := ioutil.ReadDir(downloader.TempDir)
+	infos, err := ioutil.ReadDir(tmp)
 	c.Assert(err, IsNil)
 	c.Assert(infos, HasLen, 0)
 }

diff --git a/store/lpad_test.go b/store/lpad_test.go
@@ -5,11 +5,10 @@ import (
 	. "launchpad.net/gocheck"
 	"launchpad.net/juju-core/charm"
 	"launchpad.net/juju-core/store"
+	"launchpad.net/juju-core/testing"
 	"launchpad.net/lpad"
 )
 
-var apiBase = lpad.APIBase(testServer.URL)
-
 var jsonType = map[string]string{
 	"Content-Type": "application/json",
 }
@@ -18,7 +17,7 @@ func (s *StoreSuite) TestPublishCharmDistro(c *C) {
 	branch := s.dummyBranch(c, "~joe/charms/oneiric/dummy/trunk")
 
 	// The Distro call will look for bare /charms, first.
-	testServer.Response(200, jsonType, "{}")
+	testing.Server.Response(200, jsonType, []byte("{}"))
 
 	// And then it picks up the tips.
 	data := fmt.Sprintf(`[`+
@@ -28,8 +27,9 @@ func (s *StoreSuite) TestPublishCharmDistro(c *C) {
 		`["file:///non-existent/~jeff/charms/precise/bad/skip-me", "rev3", []]`+
 		`]`,
 		branch.path(), branch.path(), branch.digest())
-	testServer.Response(200, jsonType, data)
+	testing.Server.Response(200, jsonType, []byte(data))
 
+	apiBase := lpad.APIBase(testing.Server.URL)
 	err := store.PublishCharmsDistro(s.store, apiBase)
 
 	// Should have a single failure from the trunk branch that doesn't
@@ -52,12 +52,12 @@ func (s *StoreSuite) TestPublishCharmDistro(c *C) {
 	c.Assert(err, Equals, store.ErrNotFound)
 
 	// bare /charms lookup
-	req := testServer.WaitRequest()
+	req := testing.Server.WaitRequest()
 	c.Assert(req.Method, Equals, "GET")
 	c.Assert(req.URL.Path, Equals, "/charms")
 
 	// tips request
-	req = testServer.WaitRequest()
+	req = testing.Server.WaitRequest()
 	c.Assert(req.Method, Equals, "GET")
 	c.Assert(req.URL.Path, Equals, "/charms")
 	c.Assert(req.Form["ws.op"], DeepEquals, []string{"getBranchTips"})

diff --git a/store/store_test.go b/store/store_test.go
@@ -24,7 +24,7 @@ var _ = Suite(&TrivialSuite{})
 
 type StoreSuite struct {
 	MgoSuite
-	HTTPSuite
+	testing.HTTPSuite
 	store *store.Store
 }
 

diff --git a/store/http_test.go → testing/http.go b/store/http_test.go → testing/http.go
@@ -1,4 +1,4 @@
-package store_test
+package testing
 
 import (
 	"bytes"
@@ -13,17 +13,21 @@ import (
 
 type HTTPSuite struct{}
 
-var testServer = NewTestHTTPServer("http://localhost:4444", 5*time.Second)
+var Server = NewHTTPServer("http://localhost:4444", 5*time.Second)
 
 func (s *HTTPSuite) SetUpSuite(c *C) {
-	testServer.Start()
+	Server.Start()
 }
 
 func (s *HTTPSuite) TearDownTest(c *C) {
-	testServer.Flush()
+	Server.Flush()
 }
 
-type TestHTTPServer struct {
+func (s *HTTPSuite) URL(path string) string {
+	return Server.URL + path
+}
+
+type HTTPServer struct {
 	URL      string
 	Timeout  time.Duration
 	started  bool
@@ -32,19 +36,19 @@ type TestHTTPServer struct {
 	pending  chan bool
 }
 
-func NewTestHTTPServer(url_ string, timeout time.Duration) *TestHTTPServer {
-	return &TestHTTPServer{URL: url_, Timeout: timeout}
+func NewHTTPServer(url_ string, timeout time.Duration) *HTTPServer {
+	return &HTTPServer{URL: url_, Timeout: timeout}
 }
 
 type Response struct {
 	Status  int
 	Headers map[string]string
-	Body    string
+	Body    []byte
 }
 
 type ResponseFunc func(path string) Response
 
-func (s *TestHTTPServer) Start() {
+func (s *HTTPServer) Start() {
 	if s.started {
 		return
 	}
@@ -57,7 +61,7 @@ func (s *TestHTTPServer) Start() {
 	url_, _ := url.Parse(s.URL)
 	go http.ListenAndServe(url_.Host, s)
 
-	s.Response(203, nil, "")
+	s.Response(203, nil, nil)
 	for {
 		// Wait for it to be up.
 		resp, err := http.Get(s.URL)
@@ -70,7 +74,7 @@ func (s *TestHTTPServer) Start() {
 }
 
 // FlushRequests discards requests which were not yet consumed by WaitRequest.
-func (s *TestHTTPServer) Flush() {
+func (s *HTTPServer) Flush() {
 	for {
 		select {
 		case <-s.request:
@@ -89,7 +93,7 @@ func body(req *http.Request) string {
 	return string(data)
 }
 
-func (s *TestHTTPServer) ServeHTTP(w http.ResponseWriter, req *http.Request) {
+func (s *HTTPServer) ServeHTTP(w http.ResponseWriter, req *http.Request) {
 	req.ParseMultipartForm(1e6)
 	data, err := ioutil.ReadAll(req.Body)
 	if err != nil {
@@ -104,7 +108,7 @@ func (s *TestHTTPServer) ServeHTTP(w http.ResponseWriter, req *http.Request) {
 	case <-time.After(s.Timeout):
 		const msg = "ERROR: Timeout waiting for test to prepare a response\n"
 		fmt.Fprintf(os.Stderr, msg)
-		resp = Response{500, nil, msg}
+		resp = Response{500, nil, []byte(msg)}
 	}
 	if resp.Headers != nil {
 		h := w.Header()
@@ -115,13 +119,13 @@ func (s *TestHTTPServer) ServeHTTP(w http.ResponseWriter, req *http.Request) {
 	if resp.Status != 0 {
 		w.WriteHeader(resp.Status)
 	}
-	w.Write([]byte(resp.Body))
+	w.Write(resp.Body)
 }
 
 // WaitRequests returns the next n requests made to the http server from
 // the queue. If not enough requests were previously made, it waits until
 // the timeout value for them to be made.
-func (s *TestHTTPServer) WaitRequests(n int) []*http.Request {
+func (s *HTTPServer) WaitRequests(n int) []*http.Request {
 	reqs := make([]*http.Request, 0, n)
 	for i := 0; i < n; i++ {
 		select {
@@ -137,13 +141,13 @@ func (s *TestHTTPServer) WaitRequests(n int) []*http.Request {
 // WaitRequest returns the next request made to the http server from
 // the queue. If no requests were previously made, it waits until the
 // timeout value for one to be made.
-func (s *TestHTTPServer) WaitRequest() *http.Request {
+func (s *HTTPServer) WaitRequest() *http.Request {
 	return s.WaitRequests(1)[0]
 }
 
 // ResponseFunc prepares the test server to respond the following n
 // requests using f to build each response.
-func (s *TestHTTPServer) ResponseFunc(n int, f ResponseFunc) {
+func (s *HTTPServer) ResponseFunc(n int, f ResponseFunc) {
 	for i := 0; i < n; i++ {
 		s.response <- f
 	}
@@ -154,21 +158,22 @@ type ResponseMap map[string]Response
 
 // ResponseMap prepares the test server to respond the following n
 // requests using the m to obtain the responses.
-func (s *TestHTTPServer) ResponseMap(n int, m ResponseMap) {
+func (s *HTTPServer) ResponseMap(n int, m ResponseMap) {
 	f := func(path string) Response {
 		for rpath, resp := range m {
 			if rpath == path {
 				return resp
 			}
 		}
-		return Response{Status: 500, Body: "Path not found in response map: " + path}
+		body := []byte("Path not found in response map: " + path)
+		return Response{Status: 500, Body: body}
 	}
 	s.ResponseFunc(n, f)
 }
 
 // Responses prepares the test server to respond the following n requests
 // using the provided response parameters.
-func (s *TestHTTPServer) Responses(n int, status int, headers map[string]string, body string) {
+func (s *HTTPServer) Responses(n int, status int, headers map[string]string, body []byte) {
 	f := func(path string) Response {
 		return Response{status, headers, body}
 	}
@@ -177,6 +182,6 @@ func (s *TestHTTPServer) Responses(n int, status int, headers map[string]string,
 
 // Response prepares the test server to respond the following request
 // using the provided response parameters.
-func (s *TestHTTPServer) Response(status int, headers map[string]string, body string) {
+func (s *HTTPServer) Response(status int, headers map[string]string, body []byte) {
 	s.Responses(1, status, headers, body)
 }