Web API test refactor: simpler, more readable and more complete tests.

api/doc.go

@@ -22,3 +22,13 @@ gag order, super-injunction (UK), National security letter (US) or similar
 mechanisms.
 */
 package api
+
+// Short is the type of responses provided by the Web API from a target.
+type Short struct {
+	Short string `json:"short"`
+}
+
+// Target is the type of requests accepted by the Web API for target submissions.
+type Target struct {
+	Target string `json:"target"`
+}

api/get_test.go

@@ -33,12 +33,29 @@ func TestHandleGetShortHappy(t *testing.T) {
 	res, err := c.Get(ts.URL + "/" + sampleShort)
 
 	if err != nil {
-		t.Error("Getting an existing short URL should not fail")
+		t.Log("Getting an existing short URL should not fail")
+		t.FailNow()
 	}
 	res.Body.Close()
 
 	if res.StatusCode != http.StatusTemporaryRedirect {
-		t.Errorf("Existing short URL returned %d, expected %d", res.StatusCode, http.StatusTemporaryRedirect)
+		t.Logf("Existing short URL returned %d, expected %d", res.StatusCode, http.StatusTemporaryRedirect)
+		t.FailNow()
+	}
+
+	location, err := res.Location()
+	if err == http.ErrNoLocation {
+		t.Log("Existing short URL returned a redirect without a Location header")
+		t.FailNow()
+	}
+	if err != nil {
+		t.Log(err)
+		t.FailNow()
+	}
+	target := location.String()
+	if target != sampleTarget {
+		t.Logf("Existing short URL redirected to %s instead of expected location", target)
+		t.FailNow()
 	}
 }
 
@@ -59,6 +76,6 @@ func TestHandleGetShortSadNotFound(t *testing.T) {
 	res.Body.Close()
 
 	if res.StatusCode != http.StatusNotFound {
-		t.Errorf("Existing short URL returned %d, expected %d", res.StatusCode, http.StatusNotFound)
+		t.Errorf("Undefined short URL returned %d, expected %d", res.StatusCode, http.StatusNotFound)
 	}
 }

api/post.go

@@ -11,7 +11,7 @@ import (
 const postContentType = "application/json"
 
 func jsonFromString(s string) []byte {
-	j, err := json.Marshal([]byte(s))
+	j, err := json.Marshal(Target{})
 	if err != nil {
 		panic(err)
 	}
@@ -26,15 +26,15 @@ func HandlePostTarget(w http.ResponseWriter, r *http.Request) {
 		w.Write(jsonFromString(`{ error: "Incomplete request body"}`))
 		return
 	}
-	wrapper := struct{ Target string }{}
-	err = json.Unmarshal(payload, &wrapper)
+	var target Target
+	err = json.Unmarshal(payload, &target)
 	if err != nil {
 		w.WriteHeader(http.StatusBadRequest)
 		w.Write(jsonFromString(`{ error: "Invalid JSON request"}`))
 		return
 	}
 
-	su, isNew, err := domain.GetShortURL(wrapper.Target)
+	shortString, isNew, err := domain.GetShortURL(target.Target)
 
 	w.Header().Set("Content-type", postContentType)
 
@@ -57,7 +57,8 @@ func HandlePostTarget(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	payload, err = json.Marshal(su)
+	short := Short{Short: shortString}
+	payload, err = json.Marshal(short)
 	if err != nil {
 		w.WriteHeader(http.StatusInternalServerError)
 		w.Write(jsonFromString(`{ error: "Short URL serialization error"}`))

api/post_test.go

@@ -3,7 +3,7 @@ package api
 import (
 	"bytes"
 	"encoding/json"
-	"fmt"
+	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
 	"testing"
@@ -11,7 +11,20 @@ import (
 	"code.osinet.fr/fgm/kurz/domain"
 )
 
-func setupPost(seed bool) (*httptest.Server, *http.Client) {
+func jsonBody(t *testing.T, res *http.Response) Short {
+	body, err := ioutil.ReadAll(res.Body)
+	var actual Short
+	err = json.Unmarshal(body, &actual)
+	if err != nil {
+		t.Logf("Response body is not valid JSON: %s", body)
+		t.FailNow()
+	}
+	return actual
+}
+
+// subTest performs the main test work: settings up repositories, querying the
+// web API, and ensuring response status and format.
+func subTest(t *testing.T, seed bool, targetURL string, expectedStatus int, errorMessage string) *http.Response {
 	tr := domain.MakeMockTargetRepo(!seed)
 	if seed {
 		tr.Data[domain.TargetURL{URL: domain.URL(sampleTarget)}] = domain.ShortURL{URL: domain.URL(sampleShort)}
@@ -19,72 +32,66 @@ func setupPost(seed bool) (*httptest.Server, *http.Client) {
 	domain.RegisterRepositories(domain.MockShortRepo{}, tr)
 
 	ts := httptest.NewServer(http.HandlerFunc(HandlePostTarget))
+	defer ts.Close()
 
 	c := ts.Client()
 	c.CheckRedirect = doNotFollowRedirects
 
-	return ts, c
-}
-
-func TestHandlePostTargetHappy(t *testing.T) {
-	ts, c := setupPost(false)
-	defer ts.Close()
-
-	target, err := json.Marshal(map[string]string{"target": sampleTarget})
+	target, err := json.Marshal(Target{Target: targetURL})
 	if err != nil {
-		t.Log(err)
-		t.FailNow()
+		panic(err)
 	}
 
-	// Submitting a new valid target should succeed with 201 and return new short
 	res, err := c.Post(ts.URL, postContentType, bytes.NewReader(target))
 	if err != nil {
 		t.Log(err)
 		t.FailNow()
 	}
-	if res.StatusCode != http.StatusCreated {
-		t.Log("Creation of new short for valid target should succeed")
-		t.FailNow()
-	}
 
-	// Submitting an existing target should fail with 409 and return existing short
-	res, err = c.Post(ts.URL, postContentType, bytes.NewReader(target))
-	if err != nil {
-		t.Log(err)
+	if res.StatusCode != expectedStatus {
+		t.Log(errorMessage)
 		t.FailNow()
 	}
-	if res.StatusCode != http.StatusConflict {
-		t.Error("Re-creation of existing short should conflict")
-	}
+	return res
 }
 
-func TestHandlePostTargetSad(t *testing.T) {
-	ts, c := setupPost(true)
-	defer ts.Close()
+// TestHandleGetShortHappyNew ensures that submitting a new valid target succeeds
+// with 201 and returns a new short.
+func TestHandleGetShortHappyNew(t *testing.T) {
+	res := subTest(t, false, sampleTarget,
+		http.StatusCreated, "Creation of new short for valid target should succeed")
 
-	target, err := json.Marshal(map[string]string{"target": sampleTarget + "bis"})
-	if err != nil {
-		fmt.Println(err)
+	// Mock API creates short URLs equal to the target.
+	actual := jsonBody(t, res)
+	if actual.Short != sampleTarget {
+		t.Logf("Response short URL is %s, expected %s", actual.Short, sampleTarget)
 		t.FailNow()
 	}
+}
 
-	// Submitting an invalid target should fail with 400
-	res, err := c.Post(ts.URL, postContentType, nil)
-	if err != nil {
-		fmt.Println(err)
-		t.FailNow()
-	}
-	if res.StatusCode != http.StatusBadRequest {
-		t.Error("Creation of short for empty target should be a bad request")
-	}
+// TestHandleGetShortHappyOld ensures that submitting an existing target fails
+// with 409 and returns an existing short.
+func TestHandleGetShortHappyOld(t *testing.T) {
+	res := subTest(t, true, sampleTarget,
+		http.StatusConflict, "Re-creation of existing short should conflict")
 
-	// Submitting a new valid target should fail with 50x since mock repo is set to no creation.
-	res, err = c.Post(ts.URL, postContentType, bytes.NewReader(target))
-	if err != nil {
-		fmt.Println(err)
+	// It was created that way in the previous Post() call.
+	actual := jsonBody(t, res)
+	if actual.Short != sampleShort {
+		t.Logf("Response short URL is %s, expected %s", actual.Short, sampleShort)
 		t.FailNow()
 	}
-	if res.StatusCode != http.StatusInternalServerError {
-		t.Error("Creation of new short for valid target should fail since repository cannot create")
-	}
+}
+
+// TestHandlePostTargetSadEmpty ensures that submitting an invalid target fails with 400.
+func TestHandlePostTargetSadEmpty(t *testing.T) {
+	subTest(t, true, "",
+		http.StatusBadRequest, "Creation of short for empty target should be a bad request")
+}
+
+// TestHandlePostTargetSadUncreated ensures that submitting a new valid target
+// fails with 50x when the repository cannot create new short URLs.
+func TestHandlePostTargetSadUncreated(t *testing.T) {
+	subTest(t, true, sampleTarget+"bis",
+		http.StatusInternalServerError, "Creation of new short for valid target should fail since repository cannot create")
 }

domain/domain_api_testing.go

@@ -21,7 +21,7 @@ func (tr MockTargetRepo) GetShort(tu TargetURL) (su ShortURL, isNew bool, err er
 		return
 	}
 	if tr.create {
-		su = ShortURL{URL: tu.URL}
+		su = ShortURL{URL: tu.URL, target: tu}
 		tr.Data[tu] = su
 		isNew = true
 	} else {

domain/short_url_test.go

@@ -13,7 +13,7 @@ func TestNewShortURLHappy(t *testing.T) {
 		t.FailNow()
 	}
 
-	if su.URL.IsEmpty() || su.target.IsEmpty() || !su.target.MayRedirect() {
+	if su.URL.IsEmpty() || su.Target().IsEmpty() || !su.Target().MayRedirect() {
 		t.FailNow()
 	}
 }

domain/target_url.go

@@ -4,8 +4,8 @@ type TargetURL struct {
 	URL
 }
 
-func (tu *TargetURL) IsEmpty() bool {
-	return tu == nil || tu.URL.IsEmpty()
+func (tu TargetURL) IsEmpty() bool {
+	return &tu == nil || tu.URL.IsEmpty()
 }
 
 /*
@@ -15,6 +15,6 @@ In the current version, any non empty RedirectURL may be used. In future version
 restrictions like blocked URLs or even whole domains may limit the availability
 of a target RedirectURL.
 */
-func (tu *TargetURL) MayRedirect() bool {
+func (tu TargetURL) MayRedirect() bool {
 	return !tu.IsEmpty()
 }