From cfa092f4f045d004b4c03d5b69af8bab7ed5fc1b Mon Sep 17 00:00:00 2001
From: Sergey Ponomarev <me@sergey-ponomarev.ru>
Date: Tue, 16 Oct 2018 03:48:41 +0300
Subject: [PATCH] Fix LoadHTML* tests (#1559)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Digging into the test code base I've found out that some of the tests for `LoadHTML*` methods are not reliable and efficient. They use timeouts to be sure that goroutine with the server has started. And even more, in old implementation, the server started only once – all the new instances silently failed due to the occupied network port.

Here is a short overview of the proposed changes:
- it's not necessary to rely on timeouts, the server starts listening synchronously and returns control when it is ready
- once the server is run, it's stopped after a test passes
- dry out http server setup
- magic with empty closure return is eliminated
- preserve router.RunTLS coverage with integration tests
---
 gin_integration_test.go |  26 ++++-
 gin_test.go             | 252 ++++++++++++++++++++++------------------
 2 files changed, 165 insertions(+), 113 deletions(-)

diff --git a/gin_integration_test.go b/gin_integration_test.go
index 12a943b0..038c8b7c 100644
--- a/gin_integration_test.go
+++ b/gin_integration_test.go
@@ -6,6 +6,7 @@ package gin
 
 import (
 	"bufio"
+	"crypto/tls"
 	"fmt"
 	"io/ioutil"
 	"net"
@@ -20,7 +21,14 @@ import (
 )
 
 func testRequest(t *testing.T, url string) {
-	resp, err := http.Get(url)
+	tr := &http.Transport{
+		TLSClientConfig: &tls.Config{
+			InsecureSkipVerify: true,
+		},
+	}
+	client := &http.Client{Transport: tr}
+
+	resp, err := client.Get(url)
 	assert.NoError(t, err)
 	defer resp.Body.Close()
 
@@ -45,6 +53,22 @@ func TestRunEmpty(t *testing.T) {
 	testRequest(t, "http://localhost:8080/example")
 }
 
+func TestRunTLS(t *testing.T) {
+	router := New()
+	go func() {
+		router.GET("/example", func(c *Context) { c.String(http.StatusOK, "it worked") })
+
+		assert.NoError(t, router.RunTLS(":8443", "./testdata/certificate/cert.pem", "./testdata/certificate/key.pem"))
+	}()
+
+	// have to wait for the goroutine to start and run the server
+	// otherwise the main thread will complete
+	time.Sleep(5 * time.Millisecond)
+
+	assert.Error(t, router.RunTLS(":8443", "./testdata/certificate/cert.pem", "./testdata/certificate/key.pem"))
+	testRequest(t, "https://localhost:8443/example")
+}
+
 func TestRunEmptyWithEnv(t *testing.T) {
 	os.Setenv("PORT", "3123")
 	router := New()
diff --git a/gin_test.go b/gin_test.go
index 95b9cc16..353c9be1 100644
--- a/gin_test.go
+++ b/gin_test.go
@@ -10,6 +10,7 @@ import (
 	"html/template"
 	"io/ioutil"
 	"net/http"
+	"net/http/httptest"
 	"reflect"
 	"testing"
 	"time"
@@ -22,105 +23,105 @@ func formatAsDate(t time.Time) string {
 	return fmt.Sprintf("%d/%02d/%02d", year, month, day)
 }
 
-func setupHTMLFiles(t *testing.T, mode string, tls bool) func() {
-	go func() {
-		SetMode(mode)
-		router := New()
-		router.Delims("{[{", "}]}")
-		router.SetFuncMap(template.FuncMap{
-			"formatAsDate": formatAsDate,
+func setupHTMLFiles(t *testing.T, mode string, tls bool, loadMethod func(*Engine)) *httptest.Server {
+	SetMode(mode)
+	router := New()
+	router.Delims("{[{", "}]}")
+	router.SetFuncMap(template.FuncMap{
+		"formatAsDate": formatAsDate,
+	})
+	loadMethod(router)
+	router.GET("/test", func(c *Context) {
+		c.HTML(http.StatusOK, "hello.tmpl", map[string]string{"name": "world"})
+	})
+	router.GET("/raw", func(c *Context) {
+		c.HTML(http.StatusOK, "raw.tmpl", map[string]interface{}{
+			"now": time.Date(2017, 07, 01, 0, 0, 0, 0, time.UTC),
 		})
-		router.LoadHTMLFiles("./testdata/template/hello.tmpl", "./testdata/template/raw.tmpl")
-		router.GET("/test", func(c *Context) {
-			c.HTML(http.StatusOK, "hello.tmpl", map[string]string{"name": "world"})
-		})
-		router.GET("/raw", func(c *Context) {
-			c.HTML(http.StatusOK, "raw.tmpl", map[string]interface{}{
-				"now": time.Date(2017, 07, 01, 0, 0, 0, 0, time.UTC),
-			})
-		})
-		if tls {
-			// these files generated by `go run $GOROOT/src/crypto/tls/generate_cert.go --host 127.0.0.1`
-			router.RunTLS(":9999", "./testdata/certificate/cert.pem", "./testdata/certificate/key.pem")
-		} else {
-			router.Run(":8888")
-		}
-	}()
-	t.Log("waiting 1 second for server startup")
-	time.Sleep(1 * time.Second)
-	return func() {}
+	})
+
+	var ts *httptest.Server
+
+	if tls {
+		ts = httptest.NewTLSServer(router)
+	} else {
+		ts = httptest.NewServer(router)
+	}
+
+	return ts
 }
 
-func setupHTMLGlob(t *testing.T, mode string, tls bool) func() {
-	go func() {
-		SetMode(mode)
-		router := New()
-		router.Delims("{[{", "}]}")
-		router.SetFuncMap(template.FuncMap{
-			"formatAsDate": formatAsDate,
-		})
-		router.LoadHTMLGlob("./testdata/template/*")
-		router.GET("/test", func(c *Context) {
-			c.HTML(http.StatusOK, "hello.tmpl", map[string]string{"name": "world"})
-		})
-		router.GET("/raw", func(c *Context) {
-			c.HTML(http.StatusOK, "raw.tmpl", map[string]interface{}{
-				"now": time.Date(2017, 07, 01, 0, 0, 0, 0, time.UTC),
-			})
-		})
-		if tls {
-			// these files generated by `go run $GOROOT/src/crypto/tls/generate_cert.go --host 127.0.0.1`
-			router.RunTLS(":9999", "./testdata/certificate/cert.pem", "./testdata/certificate/key.pem")
-		} else {
-			router.Run(":8888")
-		}
-	}()
-	t.Log("waiting 1 second for server startup")
-	time.Sleep(1 * time.Second)
-	return func() {}
-}
+func TestLoadHTMLGlobDebugMode(t *testing.T) {
+	ts := setupHTMLFiles(
+		t,
+		DebugMode,
+		false,
+		func(router *Engine) {
+			router.LoadHTMLGlob("./testdata/template/*")
+		},
+	)
+	defer ts.Close()
 
-func TestLoadHTMLGlob(t *testing.T) {
-	td := setupHTMLGlob(t, DebugMode, false)
-	res, err := http.Get("http://127.0.0.1:8888/test")
+	res, err := http.Get(fmt.Sprintf("%s/test", ts.URL))
 	if err != nil {
 		fmt.Println(err)
 	}
 
 	resp, _ := ioutil.ReadAll(res.Body)
 	assert.Equal(t, "<h1>Hello world</h1>", string(resp))
-
-	td()
 }
 
-func TestLoadHTMLGlob2(t *testing.T) {
-	td := setupHTMLGlob(t, TestMode, false)
-	res, err := http.Get("http://127.0.0.1:8888/test")
+func TestLoadHTMLGlobTestMode(t *testing.T) {
+	ts := setupHTMLFiles(
+		t,
+		TestMode,
+		false,
+		func(router *Engine) {
+			router.LoadHTMLGlob("./testdata/template/*")
+		},
+	)
+	defer ts.Close()
+
+	res, err := http.Get(fmt.Sprintf("%s/test", ts.URL))
 	if err != nil {
 		fmt.Println(err)
 	}
 
 	resp, _ := ioutil.ReadAll(res.Body)
 	assert.Equal(t, "<h1>Hello world</h1>", string(resp))
-
-	td()
 }
 
-func TestLoadHTMLGlob3(t *testing.T) {
-	td := setupHTMLGlob(t, ReleaseMode, false)
-	res, err := http.Get("http://127.0.0.1:8888/test")
+func TestLoadHTMLGlobReleaseMode(t *testing.T) {
+	ts := setupHTMLFiles(
+		t,
+		ReleaseMode,
+		false,
+		func(router *Engine) {
+			router.LoadHTMLGlob("./testdata/template/*")
+		},
+	)
+	defer ts.Close()
+
+	res, err := http.Get(fmt.Sprintf("%s/test", ts.URL))
 	if err != nil {
 		fmt.Println(err)
 	}
 
 	resp, _ := ioutil.ReadAll(res.Body)
 	assert.Equal(t, "<h1>Hello world</h1>", string(resp))
-
-	td()
 }
 
 func TestLoadHTMLGlobUsingTLS(t *testing.T) {
-	td := setupHTMLGlob(t, DebugMode, true)
+	ts := setupHTMLFiles(
+		t,
+		DebugMode,
+		true,
+		func(router *Engine) {
+			router.LoadHTMLGlob("./testdata/template/*")
+		},
+	)
+	defer ts.Close()
+
 	// Use InsecureSkipVerify for avoiding `x509: certificate signed by unknown authority` error
 	tr := &http.Transport{
 		TLSClientConfig: &tls.Config{
@@ -128,29 +129,33 @@ func TestLoadHTMLGlobUsingTLS(t *testing.T) {
 		},
 	}
 	client := &http.Client{Transport: tr}
-	res, err := client.Get("https://127.0.0.1:9999/test")
+	res, err := client.Get(fmt.Sprintf("%s/test", ts.URL))
 	if err != nil {
 		fmt.Println(err)
 	}
 
 	resp, _ := ioutil.ReadAll(res.Body)
 	assert.Equal(t, "<h1>Hello world</h1>", string(resp))
-
-	td()
 }
 
 func TestLoadHTMLGlobFromFuncMap(t *testing.T) {
-	time.Now()
-	td := setupHTMLGlob(t, DebugMode, false)
-	res, err := http.Get("http://127.0.0.1:8888/raw")
+	ts := setupHTMLFiles(
+		t,
+		DebugMode,
+		false,
+		func(router *Engine) {
+			router.LoadHTMLGlob("./testdata/template/*")
+		},
+	)
+	defer ts.Close()
+
+	res, err := http.Get(fmt.Sprintf("%s/raw", ts.URL))
 	if err != nil {
 		fmt.Println(err)
 	}
 
 	resp, _ := ioutil.ReadAll(res.Body)
 	assert.Equal(t, "Date: 2017/07/01\n", string(resp))
-
-	td()
 }
 
 func init() {
@@ -164,59 +169,77 @@ func TestCreateEngine(t *testing.T) {
 	assert.Empty(t, router.Handlers)
 }
 
-// func TestLoadHTMLDebugMode(t *testing.T) {
-// 	router := New()
-// 	SetMode(DebugMode)
-// 	router.LoadHTMLGlob("*.testtmpl")
-// 	r := router.HTMLRender.(render.HTMLDebug)
-// 	assert.Empty(t, r.Files)
-// 	assert.Equal(t, "*.testtmpl", r.Glob)
-//
-// 	router.LoadHTMLFiles("index.html.testtmpl", "login.html.testtmpl")
-// 	r = router.HTMLRender.(render.HTMLDebug)
-// 	assert.Empty(t, r.Glob)
-// 	assert.Equal(t, []string{"index.html", "login.html"}, r.Files)
-// 	SetMode(TestMode)
-// }
+func TestLoadHTMLFilesTestMode(t *testing.T) {
+	ts := setupHTMLFiles(
+		t,
+		TestMode,
+		false,
+		func(router *Engine) {
+			router.LoadHTMLFiles("./testdata/template/hello.tmpl", "./testdata/template/raw.tmpl")
+		},
+	)
+	defer ts.Close()
 
-func TestLoadHTMLFiles(t *testing.T) {
-	td := setupHTMLFiles(t, TestMode, false)
-	res, err := http.Get("http://127.0.0.1:8888/test")
+	res, err := http.Get(fmt.Sprintf("%s/test", ts.URL))
 	if err != nil {
 		fmt.Println(err)
 	}
 
 	resp, _ := ioutil.ReadAll(res.Body)
 	assert.Equal(t, "<h1>Hello world</h1>", string(resp))
-	td()
 }
 
-func TestLoadHTMLFiles2(t *testing.T) {
-	td := setupHTMLFiles(t, DebugMode, false)
-	res, err := http.Get("http://127.0.0.1:8888/test")
+func TestLoadHTMLFilesDebugMode(t *testing.T) {
+	ts := setupHTMLFiles(
+		t,
+		DebugMode,
+		false,
+		func(router *Engine) {
+			router.LoadHTMLFiles("./testdata/template/hello.tmpl", "./testdata/template/raw.tmpl")
+		},
+	)
+	defer ts.Close()
+
+	res, err := http.Get(fmt.Sprintf("%s/test", ts.URL))
 	if err != nil {
 		fmt.Println(err)
 	}
 
 	resp, _ := ioutil.ReadAll(res.Body)
 	assert.Equal(t, "<h1>Hello world</h1>", string(resp))
-	td()
 }
 
-func TestLoadHTMLFiles3(t *testing.T) {
-	td := setupHTMLFiles(t, ReleaseMode, false)
-	res, err := http.Get("http://127.0.0.1:8888/test")
+func TestLoadHTMLFilesReleaseMode(t *testing.T) {
+	ts := setupHTMLFiles(
+		t,
+		ReleaseMode,
+		false,
+		func(router *Engine) {
+			router.LoadHTMLFiles("./testdata/template/hello.tmpl", "./testdata/template/raw.tmpl")
+		},
+	)
+	defer ts.Close()
+
+	res, err := http.Get(fmt.Sprintf("%s/test", ts.URL))
 	if err != nil {
 		fmt.Println(err)
 	}
 
 	resp, _ := ioutil.ReadAll(res.Body)
 	assert.Equal(t, "<h1>Hello world</h1>", string(resp))
-	td()
 }
 
 func TestLoadHTMLFilesUsingTLS(t *testing.T) {
-	td := setupHTMLFiles(t, TestMode, true)
+	ts := setupHTMLFiles(
+		t,
+		TestMode,
+		true,
+		func(router *Engine) {
+			router.LoadHTMLFiles("./testdata/template/hello.tmpl", "./testdata/template/raw.tmpl")
+		},
+	)
+	defer ts.Close()
+
 	// Use InsecureSkipVerify for avoiding `x509: certificate signed by unknown authority` error
 	tr := &http.Transport{
 		TLSClientConfig: &tls.Config{
@@ -224,28 +247,33 @@ func TestLoadHTMLFilesUsingTLS(t *testing.T) {
 		},
 	}
 	client := &http.Client{Transport: tr}
-	res, err := client.Get("https://127.0.0.1:9999/test")
+	res, err := client.Get(fmt.Sprintf("%s/test", ts.URL))
 	if err != nil {
 		fmt.Println(err)
 	}
 
 	resp, _ := ioutil.ReadAll(res.Body)
 	assert.Equal(t, "<h1>Hello world</h1>", string(resp))
-	td()
 }
 
 func TestLoadHTMLFilesFuncMap(t *testing.T) {
-	time.Now()
-	td := setupHTMLFiles(t, TestMode, false)
-	res, err := http.Get("http://127.0.0.1:8888/raw")
+	ts := setupHTMLFiles(
+		t,
+		TestMode,
+		false,
+		func(router *Engine) {
+			router.LoadHTMLFiles("./testdata/template/hello.tmpl", "./testdata/template/raw.tmpl")
+		},
+	)
+	defer ts.Close()
+
+	res, err := http.Get(fmt.Sprintf("%s/raw", ts.URL))
 	if err != nil {
 		fmt.Println(err)
 	}
 
 	resp, _ := ioutil.ReadAll(res.Body)
 	assert.Equal(t, "Date: 2017/07/01\n", string(resp))
-
-	td()
 }
 
 func TestAddRoute(t *testing.T) {