From fce90dc761b542c26e4af746fc1c29eeb12fb3e5 Mon Sep 17 00:00:00 2001 From: Kelsey Hightower Date: Sun, 20 Jul 2014 17:34:26 -0700 Subject: [PATCH] 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 | 1 - pkg/health/health.go | 4 +- pkg/health/health_check_test.go | 98 +++++++++++--------- pkg/registry/healthy_minion_registry_test.go | 17 +++- 4 files changed, 65 insertions(+), 55 deletions(-) diff --git a/README.md b/README.md index 45707ba0d0f..725335e4a18 100644 --- a/README.md +++ b/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 . ``` - diff --git a/pkg/health/health.go b/pkg/health/health.go index 7ea0faf1b25..2610482d2ac 100644 --- a/pkg/health/health.go +++ b/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 } diff --git a/pkg/health/health_check_test.go b/pkg/health/health_check_test.go index 4f4be7293c7..ba36249578c 100644 --- a/pkg/health/health_check_test.go +++ b/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. -type fakeHTTPClient struct { - req string - res http.Response - err error -} +const statusServerEarlyShutdown = -1 -func (f *fakeHTTPClient) Get(url string) (*http.Response, error) { - f.req = url - return &f.res, f.err -} - -func TestHttpHealth(t *testing.T) { - fakeClient := fakeHTTPClient{ - res: http.Response{ - StatusCode: http.StatusOK, - }, +func TestHealthChecker(t *testing.T) { + var healthCheckerTests = []struct { + status int + health Status + }{ + {http.StatusOK, Healthy}, + {statusServerEarlyShutdown, Unknown}, + {http.StatusBadRequest, Unhealthy}, + {http.StatusBadGateway, Unhealthy}, + {http.StatusInternalServerError, Unhealthy}, } - - check := HTTPHealthChecker{ - client: &fakeClient, - } - - container := api.Container{ - LivenessProbe: &api.LivenessProbe{ - HTTPGet: &api.HTTPGetProbe{ - Port: "8080", - Path: "/foo/bar", + for _, healthCheckerTest := range healthCheckerTests { + tt := healthCheckerTest + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(tt.status) + })) + u, err := url.Parse(ts.URL) + if err != nil { + t.Errorf("Unexpected error: %v", err) + } + 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", - }, - } - - ok, err := check.HealthCheck(container) - if ok != Healthy { - t.Error("Unexpected unhealthy") - } - if err != nil { - t.Errorf("Unexpected error: %#v", err) - } - if fakeClient.req != "http://localhost:8080/foo/bar" { - t.Errorf("Unexpected url: %s", fakeClient.req) + } + hc := NewHealthChecker() + health, err := hc.HealthCheck(container) + if err != nil && tt.health != Unknown { + t.Errorf("Unexpected error: %v", err) + } + if health != tt.health { + t.Errorf("Expected %v, got %v", tt.health, health) + } } } @@ -81,12 +89,10 @@ func TestFindPort(t *testing.T) { }, }, } - check := HTTPHealthChecker{} - validatePort(t, check.findPort(container, "foo"), 8080) -} - -func validatePort(t *testing.T, port int64, expectedPort int64) { - if port != expectedPort { - t.Errorf("Unexpected port: %d, expected: %d", port, expectedPort) + checker := HTTPHealthChecker{} + want := int64(8080) + got := checker.findPort(container, "foo") + if got != want { + t.Errorf("Expected %v, got %v", want, got) } } diff --git a/pkg/registry/healthy_minion_registry_test.go b/pkg/registry/healthy_minion_registry_test.go index 7ba29a6b6bc..73d17b4e0aa 100644 --- a/pkg/registry/healthy_minion_registry_test.go +++ b/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, - }, nil + StatusCode: status, + 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 } }