Improve test coverage for the health package and remove mocks

The current tests for the health package utilize a fake HTTP client
for testing health checks and only cover a limited set of test cases.

This patch removes the need for mocks by using the httptest package
from the standard library. After removing the fake HTTP client a bug
was found in the health.Check function; it incorrectly assumes that
a http.Response is always non-nil. Fix the issue by moving the defer
that closes the http.Response.Body after error handling.

Related tests in the registry package have be refactored to work with
the changes made to the health.Check function. All methods that implement
the health.HTTPGetInterface interface now return a http.Response with
with a noop http.Response.Body.

This patch does not introduce any changes in behavior.

README.md

@@ -204,4 +204,3 @@ sudo docker build -t kubernetes/raml2html .
 sudo docker run --name="docgen" kubernetes/raml2html
 sudo docker cp docgen:/data/kubernetes.html .
 ```
-

pkg/health/health.go

@@ -42,12 +42,10 @@ type HTTPGetInterface interface {
 // It returns Unknown and err if the HTTP communication itself fails.
 func Check(url string, client HTTPGetInterface) (Status, error) {
 	res, err := client.Get(url)
-	if res.Body != nil {
-		defer res.Body.Close()
-	}
 	if err != nil {
 		return Unknown, err
 	}
+	defer res.Body.Close()
 	if res.StatusCode >= http.StatusOK && res.StatusCode < http.StatusBadRequest {
 		return Healthy, nil
 	}

pkg/health/health_check_test.go

@@ -17,54 +17,62 @@ limitations under the License.
 package health
 
 import (
+	"net"
 	"net/http"
+	"net/http/httptest"
+	"net/url"
 	"testing"
 
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
 )
 
-// fakeHTTPClient is a fake implementation of HTTPGetInterface.
+const statusServerEarlyShutdown = -1
-type fakeHTTPClient struct {
-	req string
-	res http.Response
-	err error
-}
 
-func (f *fakeHTTPClient) Get(url string) (*http.Response, error) {
+func TestHealthChecker(t *testing.T) {
-	f.req = url
+	var healthCheckerTests = []struct {
-	return &f.res, f.err
+		status int
-}
+		health Status
-
+	}{
-func TestHttpHealth(t *testing.T) {
+		{http.StatusOK, Healthy},
-	fakeClient := fakeHTTPClient{
+		{statusServerEarlyShutdown, Unknown},
-		res: http.Response{
+		{http.StatusBadRequest, Unhealthy},
-			StatusCode: http.StatusOK,
+		{http.StatusBadGateway, Unhealthy},
-		},
+		{http.StatusInternalServerError, Unhealthy},
 	}
-
+	for _, healthCheckerTest := range healthCheckerTests {
-	check := HTTPHealthChecker{
+		tt := healthCheckerTest
-		client: &fakeClient,
+		ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-	}
+			w.WriteHeader(tt.status)
-
+		}))
-	container := api.Container{
+		u, err := url.Parse(ts.URL)
-		LivenessProbe: &api.LivenessProbe{
+		if err != nil {
-			HTTPGet: &api.HTTPGetProbe{
+			t.Errorf("Unexpected error: %v", err)
-				Port: "8080",
+		}
-				Path: "/foo/bar",
+		host, port, err := net.SplitHostPort(u.Host)
+		if err != nil {
+			t.Errorf("Unexpected error: %v", err)
+		}
+		if tt.status == statusServerEarlyShutdown {
+			ts.Close()
+		}
+		container := api.Container{
+			LivenessProbe: &api.LivenessProbe{
+				HTTPGet: &api.HTTPGetProbe{
+					Port: port,
+					Path: "/foo/bar",
+					Host: host,
+				},
+				Type: "http",
 			},
-			Type: "http",
+		}
-		},
+		hc := NewHealthChecker()
-	}
+		health, err := hc.HealthCheck(container)
-
+		if err != nil && tt.health != Unknown {
-	ok, err := check.HealthCheck(container)
+			t.Errorf("Unexpected error: %v", err)
-	if ok != Healthy {
+		}
-		t.Error("Unexpected unhealthy")
+		if health != tt.health {
-	}
+			t.Errorf("Expected %v, got %v", tt.health, health)
-	if err != nil {
+		}
-		t.Errorf("Unexpected error: %#v", err)
-	}
-	if fakeClient.req != "http://localhost:8080/foo/bar" {
-		t.Errorf("Unexpected url: %s", fakeClient.req)
 	}
 }
 
@@ -81,12 +89,10 @@ func TestFindPort(t *testing.T) {
 			},
 		},
 	}
-	check := HTTPHealthChecker{}
+	checker := HTTPHealthChecker{}
-	validatePort(t, check.findPort(container, "foo"), 8080)
+	want := int64(8080)
-}
+	got := checker.findPort(container, "foo")
-
+	if got != want {
-func validatePort(t *testing.T, port int64, expectedPort int64) {
+		t.Errorf("Expected %v, got %v", want, got)
-	if port != expectedPort {
-		t.Errorf("Unexpected port: %d, expected: %d", port, expectedPort)
 	}
 }

pkg/registry/healthy_minion_registry_test.go

@@ -17,6 +17,8 @@ limitations under the License.
 package registry
 
 import (
+	"bytes"
+	"io/ioutil"
 	"net/http"
 	"reflect"
 	"testing"
@@ -24,10 +26,15 @@ import (
 
 type alwaysYes struct{}
 
-func (alwaysYes) Get(url string) (*http.Response, error) {
+func fakeHTTPResponse(status int) *http.Response {
 	return &http.Response{
-		StatusCode: http.StatusOK,
+		StatusCode: status,
-	}, nil
+		Body:       ioutil.NopCloser(&bytes.Buffer{}),
+	}
+}
+
+func (alwaysYes) Get(url string) (*http.Response, error) {
+	return fakeHTTPResponse(http.StatusOK), nil
 }
 
 func TestBasicDelegation(t *testing.T) {
@@ -66,9 +73,9 @@ type notMinion struct {
 
 func (n *notMinion) Get(url string) (*http.Response, error) {
 	if url != "http://"+n.minion+":10250/healthz" {
-		return &http.Response{StatusCode: http.StatusOK}, nil
+		return fakeHTTPResponse(http.StatusOK), nil
 	} else {
-		return &http.Response{StatusCode: http.StatusInternalServerError}, nil
+		return fakeHTTPResponse(http.StatusInternalServerError), nil
 	}
 }