refactor(httpx): use mocks to implement tests (#650)

* refactor(httpx): use mocks to implement tests

While there, make sure no test depends on external services by
replacing such tests with httptest.

See https://github.com/ooni/probe/issues/1951.

* fix(httpx): ensure we honour the context

internal/engine/httpx/fake_test.go

@@ -1,45 +0,0 @@
-package httpx
-
-import (
-	"net/http"
-	"time"
-
-	"github.com/ooni/probe-cli/v3/internal/netxlite"
-)
-
-type FakeTransport struct {
-	Err  error
-	Func func(*http.Request) (*http.Response, error)
-	Resp *http.Response
-}
-
-func (txp FakeTransport) RoundTrip(req *http.Request) (*http.Response, error) {
-	time.Sleep(10 * time.Microsecond)
-	if txp.Func != nil {
-		return txp.Func(req)
-	}
-	if req.Body != nil {
-		netxlite.ReadAllContext(req.Context(), req.Body)
-		req.Body.Close()
-	}
-	if txp.Err != nil {
-		return nil, txp.Err
-	}
-	txp.Resp.Request = req // non thread safe but it doesn't matter
-	return txp.Resp, nil
-}
-
-func (txp FakeTransport) CloseIdleConnections() {}
-
-type FakeBody struct {
-	Err error
-}
-
-func (fb FakeBody) Read(p []byte) (int, error) {
-	time.Sleep(10 * time.Microsecond)
-	return 0, fb.Err
-}
-
-func (fb FakeBody) Close() error {
-	return nil
-}

internal/engine/httpx/httpx.go

@@ -137,7 +137,7 @@ func (c *apiClient) newRequest(ctx context.Context, method, resourcePath string,
 	if query != nil {
 		URL.RawQuery = query.Encode()
 	}
-	request, err := http.NewRequest(method, URL.String(), body)
+	request, err := http.NewRequestWithContext(ctx, method, URL.String(), body)
 	if err != nil {
 		return nil, err
 	}
@@ -149,7 +149,7 @@ func (c *apiClient) newRequest(ctx context.Context, method, resourcePath string,
 		request.Header.Set("Accept", c.Accept)
 	}
 	request.Header.Set("User-Agent", c.UserAgent)
-	return request.WithContext(ctx), nil
+	return request, nil
 }
 
 // ErrRequestFailed indicates that the server returned >= 400.

internal/engine/httpx/httpx_test.go

@@ -2,7 +2,9 @@ package httpx
 
 import (
 	"context"
+	"encoding/json"
 	"errors"
+	"fmt"
 	"io"
 	"net/http"
 	"net/http/httptest"
@@ -10,13 +12,16 @@ import (
 	"strings"
 	"testing"
 
-	"github.com/apex/log"
 	"github.com/google/go-cmp/cmp"
 	"github.com/ooni/probe-cli/v3/internal/fakefill"
 	"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"
+	"github.com/ooni/probe-cli/v3/internal/version"
 )
 
-const userAgent = "miniooni/0.1.0-dev"
+// userAgent is the user agent used by this test suite
+var userAgent = fmt.Sprintf("ooniprobe-cli/%s", version.Version)
 
 func TestAPIClientTemplate(t *testing.T) {
 	t.Run("normal constructor", func(t *testing.T) {
@@ -63,30 +68,41 @@ func TestAPIClientTemplate(t *testing.T) {
 	})
 }
 
-func newClient() *apiClient {
+// newAPIClient is an helper factory creating a client for testing.
+func newAPIClient() *apiClient {
 	return &apiClient{
-		BaseURL:    "https://httpbin.org",
+		BaseURL:    "https://example.com",
 		HTTPClient: http.DefaultClient,
-		Logger:     log.Log,
+		Logger:     model.DiscardLogger,
 		UserAgent:  userAgent,
 	}
 }
 
-func TestNewRequestWithJSONBodyJSONMarshalFailure(t *testing.T) {
+// fakeRequest is a fake request we serialize.
-	client := newClient()
+type fakeRequest struct {
+	Name       string
+	Age        int
+	Sleeping   bool
+	Attributes map[string][]string
+}
+
+func TestAPIClient(t *testing.T) {
+	t.Run("newRequestWithJSONBody", func(t *testing.T) {
+		t.Run("JSON marshal failure", func(t *testing.T) {
+			client := newAPIClient()
 			req, err := client.newRequestWithJSONBody(
 				context.Background(), "GET", "/", nil, make(chan interface{}),
 			)
 			if err == nil || !strings.HasPrefix(err.Error(), "json: unsupported type") {
-		t.Fatal("not the error we expected")
+				t.Fatal("not the error we expected", err)
 			}
 			if req != nil {
 				t.Fatal("expected nil request here")
 			}
-}
+		})
 
-func TestNewRequestWithJSONBodyNewRequestFailure(t *testing.T) {
+		t.Run("newRequest failure", func(t *testing.T) {
-	client := newClient()
+			client := newAPIClient()
 			client.BaseURL = "\t\t\t" // cause URL parse error
 			req, err := client.newRequestWithJSONBody(
 				context.Background(), "GET", "/", nil, nil,
@@ -97,10 +113,41 @@ func TestNewRequestWithJSONBodyNewRequestFailure(t *testing.T) {
 			if req != nil {
 				t.Fatal("expected nil request here")
 			}
-}
+		})
 
-func TestNewRequestWithQuery(t *testing.T) {
+		t.Run("sets the content-type properly", func(t *testing.T) {
-	client := newClient()
+			var jsonReq fakeRequest
+			ff := &fakefill.Filler{}
+			ff.Fill(&jsonReq)
+			client := newAPIClient()
+			req, err := client.newRequestWithJSONBody(
+				context.Background(), "GET", "/", nil, jsonReq,
+			)
+			if err != nil {
+				t.Fatal(err)
+			}
+			if req.Header.Get("Content-Type") != "application/json" {
+				t.Fatal("did not set content-type properly")
+			}
+		})
+	})
+
+	t.Run("newRequest", func(t *testing.T) {
+		t.Run("with invalid method", func(t *testing.T) {
+			client := newAPIClient()
+			req, err := client.newRequest(
+				context.Background(), "\t\t\t", "/", nil, nil,
+			)
+			if err == nil || !strings.HasPrefix(err.Error(), "net/http: invalid method") {
+				t.Fatal("not the error we expected")
+			}
+			if req != nil {
+				t.Fatal("expected nil request here")
+			}
+		})
+
+		t.Run("with query", func(t *testing.T) {
+			client := newAPIClient()
 			q := url.Values{}
 			q.Add("antani", "mascetti")
 			q.Add("melandri", "conte")
@@ -116,64 +163,10 @@ func TestNewRequestWithQuery(t *testing.T) {
 			if req.URL.Query().Get("melandri") != "conte" {
 				t.Fatal("expected different query string here")
 			}
-}
+		})
 
-func TestNewRequestNewRequestFailure(t *testing.T) {
+		t.Run("with authorization", func(t *testing.T) {
-	client := newClient()
+			client := newAPIClient()
-	req, err := client.newRequest(
-		context.Background(), "\t\t\t", "/", nil, nil,
-	)
-	if err == nil || !strings.HasPrefix(err.Error(), "net/http: invalid method") {
-		t.Fatal("not the error we expected")
-	}
-	if req != nil {
-		t.Fatal("expected nil request here")
-	}
-}
-
-func TestNewRequestCloudfronting(t *testing.T) {
-	client := newClient()
-	client.Host = "www.x.org"
-	req, err := client.newRequest(
-		context.Background(), "GET", "/", nil, nil,
-	)
-	if err != nil {
-		t.Fatal(err)
-	}
-	if req.Host != client.Host {
-		t.Fatal("expected different req.Host here")
-	}
-}
-
-func TestNewRequestAcceptIsSet(t *testing.T) {
-	client := newClient()
-	client.Accept = "application/xml"
-	req, err := client.newRequestWithJSONBody(
-		context.Background(), "GET", "/", nil, []string{},
-	)
-	if err != nil {
-		t.Fatal(err)
-	}
-	if req.Header.Get("Accept") != "application/xml" {
-		t.Fatal("expected different Accept here")
-	}
-}
-
-func TestNewRequestContentTypeIsSet(t *testing.T) {
-	client := newClient()
-	req, err := client.newRequestWithJSONBody(
-		context.Background(), "GET", "/", nil, []string{},
-	)
-	if err != nil {
-		t.Fatal(err)
-	}
-	if req.Header.Get("Content-Type") != "application/json" {
-		t.Fatal("expected different Content-Type here")
-	}
-}
-
-func TestNewRequestAuthorizationHeader(t *testing.T) {
-	client := newClient()
 			client.Authorization = "deadbeef"
 			req, err := client.newRequest(
 				context.Background(), "GET", "/", nil, nil,
@@ -184,10 +177,38 @@ func TestNewRequestAuthorizationHeader(t *testing.T) {
 			if req.Header.Get("Authorization") != client.Authorization {
 				t.Fatal("expected different Authorization here")
 			}
-}
+		})
 
-func TestNewRequestUserAgentIsSet(t *testing.T) {
+		t.Run("with accept", func(t *testing.T) {
-	client := newClient()
+			client := newAPIClient()
+			client.Accept = "application/xml"
+			req, err := client.newRequestWithJSONBody(
+				context.Background(), "GET", "/", nil, []string{},
+			)
+			if err != nil {
+				t.Fatal(err)
+			}
+			if req.Header.Get("Accept") != "application/xml" {
+				t.Fatal("expected different Accept here")
+			}
+		})
+
+		t.Run("with custom host header", func(t *testing.T) {
+			client := newAPIClient()
+			client.Host = "www.x.org"
+			req, err := client.newRequest(
+				context.Background(), "GET", "/", nil, nil,
+			)
+			if err != nil {
+				t.Fatal(err)
+			}
+			if req.Host != client.Host {
+				t.Fatal("expected different req.Host here")
+			}
+		})
+
+		t.Run("with user agent", func(t *testing.T) {
+			client := newAPIClient()
 			req, err := client.newRequest(
 				context.Background(), "GET", "/", nil, nil,
 			)
@@ -197,197 +218,204 @@ func TestNewRequestUserAgentIsSet(t *testing.T) {
 			if req.Header.Get("User-Agent") != userAgent {
 				t.Fatal("expected different User-Agent here")
 			}
-}
+		})
+	})
 
-func TestClientDoJSONClientDoFailure(t *testing.T) {
+	t.Run("doJSON", func(t *testing.T) {
+		t.Run("do failure", func(t *testing.T) {
 			expected := errors.New("mocked error")
-	client := newClient()
+			client := newAPIClient()
-	client.HTTPClient = &http.Client{Transport: FakeTransport{
+			client.HTTPClient = &mocks.HTTPClient{
-		Err: expected,
+				MockDo: func(req *http.Request) (*http.Response, error) {
-	}}
+					return nil, expected
+				},
+			}
 			err := client.doJSON(&http.Request{URL: &url.URL{Scheme: "https", Host: "x.org"}}, nil)
 			if !errors.Is(err, expected) {
 				t.Fatal("not the error we expected")
 			}
-}
+		})
 
-func TestClientDoJSONResponseNotSuccessful(t *testing.T) {
+		t.Run("response is not successful (i.e., >= 400)", func(t *testing.T) {
-	client := newClient()
+			client := newAPIClient()
-	client.HTTPClient = &http.Client{Transport: FakeTransport{
+			client.HTTPClient = &mocks.HTTPClient{
-		Resp: &http.Response{
+				MockDo: func(req *http.Request) (*http.Response, error) {
+					return &http.Response{
 						StatusCode: 401,
-			Body:       FakeBody{},
+						Body:       io.NopCloser(strings.NewReader("{}")),
+					}, nil
 				},
-	}}
+			}
 			err := client.doJSON(&http.Request{URL: &url.URL{Scheme: "https", Host: "x.org"}}, nil)
-	if err == nil || !strings.HasPrefix(err.Error(), "httpx: request failed") {
+			if !errors.Is(err, ErrRequestFailed) {
-		t.Fatal("not the error we expected")
+				t.Fatal("not the error we expected", err)
-	}
 			}
+		})
 
-func TestClientDoJSONResponseReadingBodyError(t *testing.T) {
+		t.Run("cannot read body", func(t *testing.T) {
 			expected := errors.New("mocked error")
-	client := newClient()
+			client := newAPIClient()
-	client.HTTPClient = &http.Client{Transport: FakeTransport{
+			client.HTTPClient = &mocks.HTTPClient{
-		Resp: &http.Response{
+				MockDo: func(req *http.Request) (*http.Response, error) {
+					return &http.Response{
 						StatusCode: 200,
-			Body: FakeBody{
+						Body: io.NopCloser(&mocks.Reader{
-				Err: expected,
+							MockRead: func(b []byte) (int, error) {
+								return 0, expected
 							},
+						}),
+					}, nil
 				},
-	}}
+			}
 			err := client.doJSON(&http.Request{URL: &url.URL{Scheme: "https", Host: "x.org"}}, nil)
 			if !errors.Is(err, expected) {
 				t.Fatal("not the error we expected")
 			}
-}
+		})
 
-func TestClientDoJSONResponseIsNotJSON(t *testing.T) {
+		t.Run("response is not JSON", func(t *testing.T) {
-	client := newClient()
+			client := newAPIClient()
-	client.HTTPClient = &http.Client{Transport: FakeTransport{
+			client.HTTPClient = &mocks.HTTPClient{
-		Resp: &http.Response{
+				MockDo: func(req *http.Request) (*http.Response, error) {
+					return &http.Response{
 						StatusCode: 200,
-			Body: FakeBody{
+						Body:       io.NopCloser(strings.NewReader("[")),
-				Err: io.EOF,
+					}, nil
 				},
-		},
+			}
-	}}
 			err := client.doJSON(&http.Request{URL: &url.URL{Scheme: "https", Host: "x.org"}}, nil)
 			if err == nil || err.Error() != "unexpected end of JSON input" {
 				t.Fatal("not the error we expected")
 			}
-}
+		})
+	})
 
-type httpbinheaders struct {
+	t.Run("GetJSON", func(t *testing.T) {
-	Headers map[string]string `json:"headers"`
+		t.Run("successful case", func(t *testing.T) {
-}
-
-func TestReadJSONSuccess(t *testing.T) {
-	var headers httpbinheaders
-	err := newClient().GetJSON(context.Background(), "/headers", &headers)
-	if err != nil {
-		t.Fatal(err)
-	}
-	if headers.Headers["Host"] != "httpbin.org" {
-		t.Fatal("unexpected Host header")
-	}
-	if headers.Headers["User-Agent"] != "miniooni/0.1.0-dev" {
-		t.Fatal("unexpected Host header")
-	}
-}
-
-type httpbinpost struct {
-	Data string `json:"data"`
-}
-
-func TestCreateJSONSuccess(t *testing.T) {
-	headers := httpbinheaders{
-		Headers: map[string]string{
-			"Foo": "bar",
-		},
-	}
-	var response httpbinpost
-	err := newClient().PostJSON(context.Background(), "/post", &headers, &response)
-	if err != nil {
-		t.Fatal(err)
-	}
-	if response.Data != `{"headers":{"Foo":"bar"}}` {
-		t.Fatal(response.Data)
-	}
-}
-
-func TestReadJSONFailure(t *testing.T) {
-	var headers httpbinheaders
-	client := newClient()
-	client.BaseURL = "\t\t\t\t"
-	err := client.GetJSON(context.Background(), "/headers", &headers)
-	if err == nil || !strings.HasSuffix(err.Error(), "invalid control character in URL") {
-		t.Fatal("not the error we expected")
-	}
-}
-
-func TestCreateJSONFailure(t *testing.T) {
-	var headers httpbinheaders
-	client := newClient()
-	client.BaseURL = "\t\t\t\t"
-	err := client.PostJSON(context.Background(), "/headers", &headers, &headers)
-	if err == nil || !strings.HasSuffix(err.Error(), "invalid control character in URL") {
-		t.Fatal("not the error we expected")
-	}
-}
-
-func TestFetchResourceIntegration(t *testing.T) {
-	log.SetLevel(log.DebugLevel)
-	ctx := context.Background()
-	data, err := (&apiClient{
-		BaseURL:    "http://facebook.com/",
-		HTTPClient: http.DefaultClient,
-		Logger:     log.Log,
-		UserAgent:  "ooniprobe-engine/0.1.0",
-	}).FetchResource(ctx, "/robots.txt")
-	if err != nil {
-		t.Fatal(err)
-	}
-	if len(data) <= 0 {
-		t.Fatal("Did not expect an empty resource")
-	}
-}
-
-func TestFetchResourceExpiredContext(t *testing.T) {
-	log.SetLevel(log.DebugLevel)
-	ctx, cancel := context.WithCancel(context.Background())
-	cancel()
-	data, err := (&apiClient{
-		BaseURL:    "http://facebook.com/",
-		HTTPClient: http.DefaultClient,
-		Logger:     log.Log,
-		UserAgent:  "ooniprobe-engine/0.1.0",
-	}).FetchResource(ctx, "/robots.txt")
-	if !errors.Is(err, context.Canceled) {
-		t.Fatal("not the error we expected")
-	}
-	if len(data) != 0 {
-		t.Fatal("expected an empty resource")
-	}
-}
-
-func TestFetchResourceInvalidURL(t *testing.T) {
-	log.SetLevel(log.DebugLevel)
-	ctx := context.Background()
-	data, err := (&apiClient{
-		BaseURL:    "http://\t/",
-		HTTPClient: http.DefaultClient,
-		Logger:     log.Log,
-		UserAgent:  "ooniprobe-engine/0.1.0",
-	}).FetchResource(ctx, "/robots.txt")
-	if err == nil || !strings.HasSuffix(err.Error(), "invalid control character in URL") {
-		t.Fatal("not the error we expected")
-	}
-	if len(data) != 0 {
-		t.Fatal("expected an empty resource")
-	}
-}
-
-func TestFetchResource400(t *testing.T) {
 			server := httptest.NewServer(http.HandlerFunc(
 				func(w http.ResponseWriter, r *http.Request) {
-			w.WriteHeader(400)
+					w.Write([]byte(`["foo", "bar"]`))
 				},
 			))
 			defer server.Close()
-	log.SetLevel(log.DebugLevel)
 			ctx := context.Background()
-	data, err := (&apiClient{
+			var result []string
-		Authorization: "foobar",
+			err := (&apiClient{
 				BaseURL:    server.URL,
 				HTTPClient: http.DefaultClient,
-		Logger:        log.Log,
+				Logger:     model.DiscardLogger,
-		UserAgent:     "ooniprobe-engine/0.1.0",
+			}).GetJSON(ctx, "/", &result)
-	}).FetchResource(ctx, "")
+			if err != nil {
-	if err == nil || !strings.HasSuffix(err.Error(), "400 Bad Request") {
+				t.Fatal(err)
+			}
+			if len(result) != 2 || result[0] != "foo" || result[1] != "bar" {
+				t.Fatal("invalid result", result)
+			}
+		})
+
+		t.Run("failure case", func(t *testing.T) {
+			var headers []string
+			client := newAPIClient()
+			client.BaseURL = "\t\t\t\t"
+			err := client.GetJSON(context.Background(), "/", &headers)
+			if err == nil || !strings.HasSuffix(err.Error(), "invalid control character in URL") {
 				t.Fatal("not the error we expected")
 			}
-	if len(data) != 0 {
+		})
-		t.Fatal("expected an empty resource")
+	})
+
+	t.Run("PostJSON", func(t *testing.T) {
+		t.Run("successful case", func(t *testing.T) {
+			server := httptest.NewServer(http.HandlerFunc(
+				func(w http.ResponseWriter, r *http.Request) {
+					var incoming []string
+					data, err := netxlite.ReadAllContext(r.Context(), r.Body)
+					if err != nil {
+						w.WriteHeader(500)
+						return
 					}
+					if err := json.Unmarshal(data, &incoming); err != nil {
+						w.WriteHeader(500)
+						return
+					}
+					w.Write(data)
+				},
+			))
+			defer server.Close()
+			ctx := context.Background()
+			incoming := []string{"foo", "bar"}
+			var result []string
+			err := (&apiClient{
+				BaseURL:    server.URL,
+				HTTPClient: http.DefaultClient,
+				Logger:     model.DiscardLogger,
+			}).PostJSON(ctx, "/", incoming, &result)
+			if err != nil {
+				t.Fatal(err)
+			}
+			if diff := cmp.Diff(incoming, result); diff != "" {
+				t.Fatal(diff)
+			}
+		})
+
+		t.Run("failure case", func(t *testing.T) {
+			incoming := []string{"foo", "bar"}
+			var result []string
+			client := newAPIClient()
+			client.BaseURL = "\t\t\t\t"
+			err := client.PostJSON(context.Background(), "/", incoming, &result)
+			if err == nil || !strings.HasSuffix(err.Error(), "invalid control character in URL") {
+				t.Fatal("not the error we expected")
+			}
+		})
+	})
+
+	t.Run("FetchResource", func(t *testing.T) {
+		t.Run("successful case", func(t *testing.T) {
+			server := httptest.NewServer(http.HandlerFunc(
+				func(w http.ResponseWriter, r *http.Request) {
+					w.Write([]byte("deadbeef"))
+				},
+			))
+			defer server.Close()
+			ctx := context.Background()
+			data, err := (&apiClient{
+				BaseURL:    server.URL,
+				HTTPClient: http.DefaultClient,
+				Logger:     model.DiscardLogger,
+			}).FetchResource(ctx, "/")
+			if err != nil {
+				t.Fatal(err)
+			}
+			if string(data) != "deadbeef" {
+				t.Fatal("invalid data")
+			}
+		})
+
+		t.Run("failure case", func(t *testing.T) {
+			client := newAPIClient()
+			client.BaseURL = "\t\t\t\t"
+			data, err := client.FetchResource(context.Background(), "/")
+			if err == nil || !strings.HasSuffix(err.Error(), "invalid control character in URL") {
+				t.Fatal("not the error we expected")
+			}
+			if data != nil {
+				t.Fatal("unexpected data")
+			}
+		})
+	})
+
+	t.Run("we honour context", func(t *testing.T) {
+		// It should suffice to check one of the public methods here
+		client := newAPIClient()
+		ctx, cancel := context.WithCancel(context.Background())
+		cancel() // test should fail
+		data, err := client.FetchResource(ctx, "/")
+		if !errors.Is(err, context.Canceled) {
+			t.Fatal("unexpected err", err)
+		}
+		if data != nil {
+			t.Fatal("unexpected data")
+		}
+	})
 }