From 59a7fb5ca978911ec2966b7dc3dd303fba50daf1 Mon Sep 17 00:00:00 2001
From: Daniel Nephin <dnephin@gmail.com>
Date: Sat, 27 Nov 2021 15:53:51 -0500
Subject: [PATCH] testing: use httptest with freeport

---
 command/snapshot/save/snapshot_save_test.go |  6 ++---
 lib/testing_httpserver.go                   | 30 +++++++++++----------
 sdk/testutil/server.go                      | 11 +++++---
 sdk/testutil/types.go                       |  1 +
 4 files changed, 28 insertions(+), 20 deletions(-)

diff --git a/command/snapshot/save/snapshot_save_test.go b/command/snapshot/save/snapshot_save_test.go
index b1b57289a..bc4bba216 100644
--- a/command/snapshot/save/snapshot_save_test.go
+++ b/command/snapshot/save/snapshot_save_test.go
@@ -138,7 +138,7 @@ func TestSnapshotSaveCommand_TruncatedStream(t *testing.T) {
 	var fakeResult atomic.Value
 
 	// Run a fake webserver to pretend to be the snapshot API.
-	fakeAddr := lib.StartTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
+	srv := lib.NewHTTPTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
 		if req.URL.Path != "/v1/snapshot" {
 			w.WriteHeader(http.StatusNotFound)
 			return
@@ -160,7 +160,7 @@ func TestSnapshotSaveCommand_TruncatedStream(t *testing.T) {
 
 	// Wait until the server is actually listening.
 	retry.Run(t, func(r *retry.R) {
-		resp, err := http.Get("http://" + fakeAddr + "/not-real")
+		resp, err := srv.Client().Get(srv.URL + "/not-real")
 		require.NoError(r, err)
 		require.Equal(r, http.StatusNotFound, resp.StatusCode)
 	})
@@ -179,7 +179,7 @@ func TestSnapshotSaveCommand_TruncatedStream(t *testing.T) {
 
 			file := filepath.Join(dir, "backup.tgz")
 			args := []string{
-				"-http-addr=" + fakeAddr, // point to the fake
+				"-http-addr=" + srv.Listener.Addr().String(), // point to the fake
 				file,
 			}
 
diff --git a/lib/testing_httpserver.go b/lib/testing_httpserver.go
index 86832ed20..a4ad79622 100644
--- a/lib/testing_httpserver.go
+++ b/lib/testing_httpserver.go
@@ -1,31 +1,33 @@
 package lib
 
 import (
+	"net"
 	"net/http"
+	"net/http/httptest"
 
 	"github.com/hashicorp/consul/ipaddr"
 	"github.com/hashicorp/consul/sdk/freeport"
-	"github.com/mitchellh/go-testing-interface"
 )
 
-// StartTestServer fires up a web server on a random unused port to serve the
-// given handler body. The address it is listening on is returned. When the
-// test case terminates the server will be stopped via cleanup functions.
+// NewHTTPTestServer starts and returns an httptest.Server that is listening
+// on a random port from freeport.Port. When the test case ends the server
+// will be stopped.
 //
 // We can't directly use httptest.Server here because that only thinks a port
 // is free if it's not bound. Consul tests frequently reserve ports via
 // `sdk/freeport` so you can have one part of the test try to use a port and
 // _know_ nothing is listening. If you simply assumed unbound ports were free
 // you'd end up with test cross-talk and weirdness.
-func StartTestServer(t testing.T, handler http.Handler) string {
+func NewHTTPTestServer(t freeport.TestingT, handler http.Handler) *httptest.Server {
+	srv := httptest.NewUnstartedServer(handler)
+
 	addr := ipaddr.FormatAddressPort("127.0.0.1", freeport.Port(t))
-
-	server := &http.Server{Addr: addr, Handler: handler}
-	t.Cleanup(func() {
-		server.Close()
-	})
-
-	go server.ListenAndServe()
-
-	return addr
+	listener, err := net.Listen("tcp", addr)
+	if err != nil {
+		t.Fatalf("failed to listen on %v", addr)
+	}
+	srv.Listener = listener
+	t.Cleanup(srv.Close)
+	srv.Start()
+	return srv
 }
diff --git a/sdk/testutil/server.go b/sdk/testutil/server.go
index 1ecde56a1..cd894a2ef 100644
--- a/sdk/testutil/server.go
+++ b/sdk/testutil/server.go
@@ -29,11 +29,12 @@ import (
 	"testing"
 	"time"
 
-	"github.com/hashicorp/consul/sdk/freeport"
-	"github.com/hashicorp/consul/sdk/testutil/retry"
 	"github.com/hashicorp/go-cleanhttp"
 	"github.com/hashicorp/go-uuid"
 	"github.com/pkg/errors"
+
+	"github.com/hashicorp/consul/sdk/freeport"
+	"github.com/hashicorp/consul/sdk/testutil/retry"
 )
 
 // TestPerformanceConfig configures the performance parameters.
@@ -142,7 +143,11 @@ func defaultServerConfig(t TestingTB) *TestServerConfig {
 		panic(err)
 	}
 
-	ports := freeport.MustTake(6)
+	ports, err := freeport.Take(6)
+	if err != nil {
+		t.Fatalf("failed to take ports: %v", err)
+	}
+
 	logBuffer := NewLogBuffer(t)
 
 	return &TestServerConfig{
diff --git a/sdk/testutil/types.go b/sdk/testutil/types.go
index ec04e45dc..d6b5841c5 100644
--- a/sdk/testutil/types.go
+++ b/sdk/testutil/types.go
@@ -8,4 +8,5 @@ type TestingTB interface {
 	Failed() bool
 	Logf(format string, args ...interface{})
 	Name() string
+	Fatalf(fmt string, args ...interface{})
 }