refactor(oohelperd): improve tests implementation (#835)

After this diff has landed, we have addressed all the points
originally published at ooni/probe#2134.

internal/cmd/oohelperd/handler_test.go

@@ -4,12 +4,14 @@ import (
 	"context"
 	"encoding/json"
 	"errors"
+	"io"
 	"net/http"
 	"net/http/httptest"
 	"strings"
 	"testing"
 
 	"github.com/ooni/probe-cli/v3/internal/model"
+	"github.com/ooni/probe-cli/v3/internal/model/mocks"
 	"github.com/ooni/probe-cli/v3/internal/netxlite"
 )
 
@@ -149,17 +151,30 @@ func TestWorkingAsIntended(t *testing.T) {
 func TestHandlerWithRequestBodyReadingError(t *testing.T) {
 	expected := errors.New("mocked error")
 	handler := handler{MaxAcceptableBody: 1 << 24}
-	rw := NewFakeResponseWriter()
+	var statusCode int
+	headers := http.Header{}
+	rw := &mocks.HTTPResponseWriter{
+		MockWriteHeader: func(code int) {
+			statusCode = code
+		},
+		MockHeader: func() http.Header {
+			return headers
+		},
+	}
 	req := &http.Request{
 		Method: "POST",
 		Header: map[string][]string{
 			"Content-Type":   {"application/json"},
 			"Content-Length": {"2048"},
 		},
-		Body: &FakeBody{Err: expected},
+		Body: io.NopCloser(&mocks.Reader{
+			MockRead: func(b []byte) (int, error) {
+				return 0, expected
+			},
+		}),
 	}
 	handler.ServeHTTP(rw, req)
-	if rw.StatusCode != 400 {
+	if statusCode != 400 {
 		t.Fatal("unexpected status code")
 	}
 }

internal/cmd/oohelperd/http_test.go

@@ -9,6 +9,7 @@ import (
 
 	"github.com/google/go-cmp/cmp"
 	"github.com/ooni/probe-cli/v3/internal/model"
+	"github.com/ooni/probe-cli/v3/internal/model/mocks"
 	"github.com/ooni/probe-cli/v3/internal/netxlite"
 )
 
@@ -46,8 +47,13 @@ func TestHTTPDoWithHTTPTransportFailure(t *testing.T) {
 		MaxAcceptableBody: 1 << 24,
 		NewClient: func() model.HTTPClient {
 			return &http.Client{
-				Transport: FakeTransport{
-					Err: expected,
+				Transport: &mocks.HTTPTransport{
+					MockRoundTrip: func(req *http.Request) (*http.Response, error) {
+						return nil, expected
+					},
+					MockCloseIdleConnections: func() {
+						// nothing
+					},
 				},
 			}
 		},

internal/cmd/oohelperd/main.go

@@ -4,26 +4,29 @@ package main
 import (
 	"context"
 	"flag"
+	"net"
 	"net/http"
 	"sync"
 	"time"
 
 	"github.com/apex/log"
 	"github.com/ooni/probe-cli/v3/internal/model"
 	"github.com/ooni/probe-cli/v3/internal/netxlite"
+	"github.com/ooni/probe-cli/v3/internal/runtimex"
 )
 
 const maxAcceptableBody = 1 << 24
 
 var (
 	endpoint  = flag.String("endpoint", ":8080", "Endpoint where to listen")
-	srvcancel context.CancelFunc
-	srvctx    context.Context
-	srvwg     = new(sync.WaitGroup)
+	srvAddr   = make(chan string, 1) // with buffer
+	srvCancel context.CancelFunc
+	srvCtx    context.Context
+	srvWg     = new(sync.WaitGroup)
 )
 
 func init() {
-	srvctx, srvcancel = context.WithCancel(context.Background())
+	srvCtx, srvCancel = context.WithCancel(context.Background())
 }
 
 func newResolver() model.Resolver {
@@ -48,10 +51,7 @@ func main() {
 	debug := flag.Bool("debug", false, "Toggle debug mode")
 	flag.Parse()
 	log.SetLevel(logmap[*debug])
-	testableMain()
-}
-
-func testableMain() {
+	defer srvCancel()
 	mux := http.NewServeMux()
 	mux.Handle("/", &handler{
 		MaxAcceptableBody: maxAcceptableBody,
@@ -64,9 +64,13 @@ func testableMain() {
 		NewResolver: newResolver,
 	})
 	srv := &http.Server{Addr: *endpoint, Handler: mux}
-	srvwg.Add(1)
-	go srv.ListenAndServe()
-	<-srvctx.Done()
+	listener, err := net.Listen("tcp", *endpoint)
+	runtimex.PanicOnError(err, "net.Listen failed")
+	srvAddr <- listener.Addr().String()
+	srvWg.Add(1)
+	go srv.Serve(listener)
+	<-srvCtx.Done()
 	shutdown(srv)
-	srvwg.Done()
+	listener.Close()
+	srvWg.Done()
 }

internal/cmd/oohelperd/main_test.go

@@ -1,15 +1,82 @@
 package main
 
 import (
+	"bytes"
+	"context"
+	"encoding/json"
+	"net/http"
+	"net/url"
+	"strings"
 	"testing"
+
+	"github.com/ooni/probe-cli/v3/internal/model"
+	"github.com/ooni/probe-cli/v3/internal/netxlite"
+	"github.com/ooni/probe-cli/v3/internal/runtimex"
 )
 
-func TestSmoke(t *testing.T) {
-	// Just check whether we can start and then tear down the server, so
-	// we have coverage of this code and when we see that some lines aren't
-	// covered we know these are genuine places where we're not testing
-	// the code rather than just places like this simple main.
-	go testableMain()
-	srvcancel()  // kills the listener
-	srvwg.Wait() // joined
+func TestWorkAsIntended(t *testing.T) {
+	// let the kernel pick a random free port
+	*endpoint = "127.0.0.1:0"
+
+	// run the main function in a background goroutine
+	go main()
+
+	// prepare the HTTP request body
+	jsonReq := ctrlRequest{
+		HTTPRequest: "https://dns.google",
+		HTTPRequestHeaders: map[string][]string{
+			"Accept":          {model.HTTPHeaderAccept},
+			"Accept-Language": {model.HTTPHeaderAcceptLanguage},
+			"User-Agent":      {model.HTTPHeaderUserAgent},
+		},
+		TCPConnect: []string{
+			"8.8.8.8:443",
+			"8.8.4.4:443",
+		},
+	}
+	data, err := json.Marshal(jsonReq)
+	runtimex.PanicOnError(err, "cannot marshal request")
+
+	// construct the test helper's URL
+	endpoint := <-srvAddr
+	URL := &url.URL{
+		Scheme: "http",
+		Host:   endpoint,
+		Path:   "/",
+	}
+	req, err := http.NewRequest("POST", URL.String(), bytes.NewReader(data))
+	runtimex.PanicOnError(err, "cannot create new HTTP request")
+
+	// issue the request and get the response
+	resp, err := http.DefaultClient.Do(req)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer resp.Body.Close()
+	if resp.StatusCode != 200 {
+		t.Fatal("unexpected status code", resp.StatusCode)
+	}
+
+	// read the response body
+	data, err = netxlite.ReadAllContext(context.Background(), resp.Body)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// parse the response
+	var jsonResp ctrlResponse
+	if err := json.Unmarshal(data, &jsonResp); err != nil {
+		t.Fatal(err)
+	}
+
+	// very simple correctness check
+	if !strings.Contains(jsonResp.HTTPRequest.Title, "Google") {
+		t.Fatal("expected the response title to contain the string Google")
+	}
+
+	// tear down the TH
+	srvCancel()
+
+	// wait for the background goroutine to join
+	srvWg.Wait()
 }

internal/model/mocks/http.go

@@ -40,3 +40,26 @@ func (txp *HTTPClient) Do(req *http.Request) (*http.Response, error) {
 func (txp *HTTPClient) CloseIdleConnections() {
 	txp.MockCloseIdleConnections()
 }
+
+// HTTPResponseWriter allows mocking http.ResponseWriter.
+type HTTPResponseWriter struct {
+	MockHeader func() http.Header
+
+	MockWrite func(b []byte) (int, error)
+
+	MockWriteHeader func(statusCode int)
+}
+
+var _ http.ResponseWriter = &HTTPResponseWriter{}
+
+func (w *HTTPResponseWriter) Header() http.Header {
+	return w.MockHeader()
+}
+
+func (w *HTTPResponseWriter) Write(b []byte) (int, error) {
+	return w.MockWrite(b)
+}
+
+func (w *HTTPResponseWriter) WriteHeader(statusCode int) {
+	w.MockWriteHeader(statusCode)
+}