From d2098559f40c68f0938d6251884d0ffb58053527 Mon Sep 17 00:00:00 2001
From: Luther Monson <luther.monson@gmail.com>
Date: Sat, 21 Oct 2023 00:56:26 -0700
Subject: [PATCH] updating errors coverage to 100% (#400)

---
 errors.go      |  54 ++++++++-------
 errors_test.go | 184 +++++++++++++++++++++++++++++++++----------------
 2 files changed, 152 insertions(+), 86 deletions(-)

diff --git a/errors.go b/errors.go
index 702959049..a872eefe4 100644
--- a/errors.go
+++ b/errors.go
@@ -47,41 +47,45 @@ type APIError struct {
 
 func coupleAPIErrors(r *resty.Response, err error) (*resty.Response, error) {
 	if err != nil {
+		// an error was raised in go code, no need to check the resty Response
 		return nil, NewError(err)
 	}
 
-	if r.Error() != nil {
-		// Check that response is of the correct content-type before unmarshalling
-		expectedContentType := r.Request.Header.Get("Accept")
-		responseContentType := r.Header().Get("Content-Type")
+	if r.Error() == nil {
+		// no error in the resty Response
+		return r, nil
+	}
 
-		// If the upstream Linode API server being fronted fails to respond to the request,
-		// the http server will respond with a default "Bad Gateway" page with Content-Type
-		// "text/html".
-		if r.StatusCode() == http.StatusBadGateway && responseContentType == "text/html" {
-			return nil, Error{Code: http.StatusBadGateway, Message: http.StatusText(http.StatusBadGateway)}
-		}
+	// handle the resty Response errors
 
-		if responseContentType != expectedContentType {
-			msg := fmt.Sprintf(
-				"Unexpected Content-Type: Expected: %v, Received: %v\nResponse body: %s",
-				expectedContentType,
-				responseContentType,
-				string(r.Body()),
-			)
+	// Check that response is of the correct content-type before unmarshalling
+	expectedContentType := r.Request.Header.Get("Accept")
+	responseContentType := r.Header().Get("Content-Type")
 
-			return nil, Error{Code: r.StatusCode(), Message: msg}
-		}
+	// If the upstream Linode API server being fronted fails to respond to the request,
+	// the http server will respond with a default "Bad Gateway" page with Content-Type
+	// "text/html".
+	if r.StatusCode() == http.StatusBadGateway && responseContentType == "text/html" {
+		return nil, Error{Code: http.StatusBadGateway, Message: http.StatusText(http.StatusBadGateway)}
+	}
 
-		apiError, ok := r.Error().(*APIError)
-		if !ok || (ok && len(apiError.Errors) == 0) {
-			return r, nil
-		}
+	if responseContentType != expectedContentType {
+		msg := fmt.Sprintf(
+			"Unexpected Content-Type: Expected: %v, Received: %v\nResponse body: %s",
+			expectedContentType,
+			responseContentType,
+			string(r.Body()),
+		)
+
+		return nil, Error{Code: r.StatusCode(), Message: msg}
+	}
 
-		return nil, NewError(r)
+	apiError, ok := r.Error().(*APIError)
+	if !ok || (ok && len(apiError.Errors) == 0) {
+		return r, nil
 	}
 
-	return r, nil
+	return nil, NewError(r)
 }
 
 func (e APIError) Error() string {
diff --git a/errors_test.go b/errors_test.go
index 15510ec72..86576541d 100644
--- a/errors_test.go
+++ b/errors_test.go
@@ -13,12 +13,41 @@ import (
 	"github.com/google/go-cmp/cmp"
 )
 
-type tstringer string
+type testStringer string
 
-func (t tstringer) String() string {
+func (t testStringer) String() string {
 	return string(t)
 }
 
+type testError string
+
+func (e testError) Error() string {
+	return string(e)
+}
+
+func restyError(reason, field string) *resty.Response {
+	var reasons []APIErrorReason
+
+	// allow for an empty reasons
+	if reason != "" && field != "" {
+		reasons = append(reasons, APIErrorReason{
+			Reason: reason,
+			Field:  field,
+		})
+	}
+
+	return &resty.Response{
+		RawResponse: &http.Response{
+			StatusCode: 500,
+		},
+		Request: &resty.Request{
+			Error: &APIError{
+				Errors: reasons,
+			},
+		},
+	}
+}
+
 func TestNewError(t *testing.T) {
 	if NewError(nil) != nil {
 		t.Errorf("nil error should return nil")
@@ -26,33 +55,27 @@ func TestNewError(t *testing.T) {
 	if NewError(struct{}{}).Code != ErrorUnsupported {
 		t.Error("empty struct should return unsupported error type")
 	}
+
 	err := errors.New("test")
-	newErr := NewError(&err)
-	if newErr.Message == err.Error() && newErr.Code == ErrorFromError {
-		t.Error("nil error should return nil")
+	newErr := NewError(err)
+
+	if newErr.Message != err.Error() && newErr.Code != ErrorFromError {
+		t.Error("error should return ErrorFromError")
 	}
 
-	if err := NewError(&resty.Response{Request: &resty.Request{}}); err.Message != "Unexpected Resty Error Response, no error" {
-		t.Error("Unexpected Resty Error Response, no error")
+	if newErr.Error() != "[002] test" {
+		t.Error("Error should support Error() formatter with code")
 	}
 
-	rerr := &resty.Response{
-		RawResponse: &http.Response{
-			StatusCode: 500,
-		},
-		Request: &resty.Request{
-			Error: &APIError{
-				[]APIErrorReason{
-					{
-						Reason: "testreason",
-						Field:  "testfield",
-					},
-				},
-			},
-		},
+	if NewError(newErr) != newErr {
+		t.Error("Error should be itself")
+	}
+
+	if err := NewError(&resty.Response{Request: &resty.Request{}}); err.Message != "Unexpected Resty Error Response, no error" {
+		t.Error("Unexpected Resty Error Response, no error")
 	}
 
-	if err := NewError(rerr); err.Message != "[testfield] testreason" {
+	if err := NewError(restyError("testreason", "testfield")); err.Message != "[testfield] testreason" {
 		t.Error("rest response error should should be set")
 	}
 
@@ -60,8 +83,12 @@ func TestNewError(t *testing.T) {
 		t.Errorf("string error should be set")
 	}
 
-	if err := NewError(tstringer("teststringer")); err.Message != "teststringer" || err.Code != ErrorFromStringer {
-		t.Errorf("stringer error should be set")
+	if err := NewError(testStringer("teststringer")); err.Message != "teststringer" || err.Code != ErrorFromStringer {
+		t.Errorf("error should be set for a stringer interface")
+	}
+
+	if err := NewError(testError("testerror")); err.Message != "testerror" || err.Code != ErrorFromError {
+		t.Errorf("error should be set for an error interface")
 	}
 }
 
@@ -82,59 +109,94 @@ func createTestServer(method, route, contentType, body string, statusCode int) (
 	return ts, &client
 }
 
-func TestCoupleAPIErrors_genericHtmlError(t *testing.T) {
-	rawResponse := `<html>
+func TestCoupleAPIErrors(t *testing.T) {
+	t.Run("not nil error generates error", func(t *testing.T) {
+		err := errors.New("test")
+		if _, err := coupleAPIErrors(nil, err); !cmp.Equal(err, NewError(err)) {
+			t.Errorf("expect a not nil error to be returned as an Error")
+		}
+	})
+
+	t.Run("resty 500 response error with reasons", func(t *testing.T) {
+		if _, err := coupleAPIErrors(restyError("testreason", "testfield"), nil); err.Error() != "[500] [testfield] testreason" {
+			t.Error("resty error should return with proper format [code] [field] reason")
+		}
+	})
+
+	t.Run("resty 500 response error without reasons", func(t *testing.T) {
+		if _, err := coupleAPIErrors(restyError("", ""), nil); err != nil {
+			t.Error("resty error with no reasons should return no error")
+		}
+	})
+
+	t.Run("resty response with nil error", func(t *testing.T) {
+		emptyErr := &resty.Response{
+			RawResponse: &http.Response{
+				StatusCode: 500,
+			},
+			Request: &resty.Request{
+				Error: nil,
+			},
+		}
+		if _, err := coupleAPIErrors(emptyErr, nil); err != nil {
+			t.Error("resty error with no reasons should return no error")
+		}
+	})
+
+	t.Run("generic html error", func(t *testing.T) {
+		rawResponse := `<html>
 <head><title>500 Internal Server Error</title></head>
 <body bgcolor="white">
 <center><h1>500 Internal Server Error</h1></center>
 <hr><center>nginx</center>
 </body>
 </html>`
-	route := "/v4/linode/instances/123"
-	ts, client := createTestServer(http.MethodGet, route, "text/html", rawResponse, http.StatusInternalServerError)
-	client.SetDebug(true)
-	defer ts.Close()
-
-	expectedError := Error{
-		Code:    http.StatusInternalServerError,
-		Message: "Unexpected Content-Type: Expected: application/json, Received: text/html\nResponse body: " + rawResponse,
-	}
+		route := "/v4/linode/instances/123"
+		ts, client := createTestServer(http.MethodGet, route, "text/html", rawResponse, http.StatusInternalServerError)
+		// client.SetDebug(true)
+		defer ts.Close()
+
+		expectedError := Error{
+			Code:    http.StatusInternalServerError,
+			Message: "Unexpected Content-Type: Expected: application/json, Received: text/html\nResponse body: " + rawResponse,
+		}
 
-	_, err := coupleAPIErrors(client.R(context.Background()).SetResult(&Instance{}).Get(ts.URL + route))
-	if diff := cmp.Diff(expectedError, err); diff != "" {
-		t.Errorf("expected error to match but got diff:\n%s", diff)
-	}
-}
+		_, err := coupleAPIErrors(client.R(context.Background()).SetResult(&Instance{}).Get(ts.URL + route))
+		if diff := cmp.Diff(expectedError, err); diff != "" {
+			t.Errorf("expected error to match but got diff:\n%s", diff)
+		}
+	})
 
-func TestCoupleAPIErrors_badGatewayError(t *testing.T) {
-	rawResponse := []byte(`<html>
+	t.Run("bad gateway error", func(t *testing.T) {
+		rawResponse := []byte(`<html>
 <head><title>502 Bad Gateway</title></head>
 <body bgcolor="white">
 <center><h1>502 Bad Gateway</h1></center>
 <hr><center>nginx</center>
 </body>
 </html>`)
-	buf := io.NopCloser(bytes.NewBuffer(rawResponse))
+		buf := io.NopCloser(bytes.NewBuffer(rawResponse))
 
-	resp := &resty.Response{
-		Request: &resty.Request{
-			Error: errors.New("Bad Gateway"),
-		},
-		RawResponse: &http.Response{
-			Header: http.Header{
-				"Content-Type": []string{"text/html"},
+		resp := &resty.Response{
+			Request: &resty.Request{
+				Error: errors.New("Bad Gateway"),
 			},
-			StatusCode: http.StatusBadGateway,
-			Body:       buf,
-		},
-	}
+			RawResponse: &http.Response{
+				Header: http.Header{
+					"Content-Type": []string{"text/html"},
+				},
+				StatusCode: http.StatusBadGateway,
+				Body:       buf,
+			},
+		}
 
-	expectedError := Error{
-		Code:    http.StatusBadGateway,
-		Message: http.StatusText(http.StatusBadGateway),
-	}
+		expectedError := Error{
+			Code:    http.StatusBadGateway,
+			Message: http.StatusText(http.StatusBadGateway),
+		}
 
-	if _, err := coupleAPIErrors(resp, nil); !cmp.Equal(err, expectedError) {
-		t.Errorf("expected error %#v to match error %#v", err, expectedError)
-	}
+		if _, err := coupleAPIErrors(resp, nil); !cmp.Equal(err, expectedError) {
+			t.Errorf("expected error %#v to match error %#v", err, expectedError)
+		}
+	})
 }