From ee0aa1861692639811fb55112db65e5b1a9a8798 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 5 Jan 2022 16:13:42 +0100 Subject: [PATCH] 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 | 45 -- internal/engine/httpx/httpx.go | 4 +- internal/engine/httpx/httpx_test.go | 632 +++++++++++++++------------- 3 files changed, 332 insertions(+), 349 deletions(-) delete mode 100644 internal/engine/httpx/fake_test.go diff --git a/internal/engine/httpx/fake_test.go b/internal/engine/httpx/fake_test.go deleted file mode 100644 index dfc9fc0..0000000 --- a/internal/engine/httpx/fake_test.go +++ /dev/null @@ -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 -} diff --git a/internal/engine/httpx/httpx.go b/internal/engine/httpx/httpx.go index e58b53e..1f08a89 100644 --- a/internal/engine/httpx/httpx.go +++ b/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. diff --git a/internal/engine/httpx/httpx_test.go b/internal/engine/httpx/httpx_test.go index 7d4a1e9..dc32470 100644 --- a/internal/engine/httpx/httpx_test.go +++ b/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,331 +68,354 @@ 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) { - client := newClient() - 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") - } - if req != nil { - t.Fatal("expected nil request here") - } +// fakeRequest is a fake request we serialize. +type fakeRequest struct { + Name string + Age int + Sleeping bool + Attributes map[string][]string } -func TestNewRequestWithJSONBodyNewRequestFailure(t *testing.T) { - client := newClient() - client.BaseURL = "\t\t\t" // cause URL parse error - req, err := client.newRequestWithJSONBody( - context.Background(), "GET", "/", nil, nil, - ) - if err == nil || !strings.HasSuffix(err.Error(), "invalid control character in URL") { - t.Fatal("not the error we expected") - } - if req != nil { - t.Fatal("expected nil request here") - } -} +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", err) + } + if req != nil { + t.Fatal("expected nil request here") + } + }) -func TestNewRequestWithQuery(t *testing.T) { - client := newClient() - q := url.Values{} - q.Add("antani", "mascetti") - q.Add("melandri", "conte") - req, err := client.newRequest( - context.Background(), "GET", "/", q, nil, - ) - if err != nil { - t.Fatal(err) - } - if req.URL.Query().Get("antani") != "mascetti" { - t.Fatal("expected different query string here") - } - if req.URL.Query().Get("melandri") != "conte" { - t.Fatal("expected different query string here") - } -} + t.Run("newRequest failure", func(t *testing.T) { + client := newAPIClient() + client.BaseURL = "\t\t\t" // cause URL parse error + req, err := client.newRequestWithJSONBody( + context.Background(), "GET", "/", nil, nil, + ) + if err == nil || !strings.HasSuffix(err.Error(), "invalid control character in URL") { + t.Fatal("not the error we expected") + } + if req != nil { + t.Fatal("expected nil request here") + } + }) -func TestNewRequestNewRequestFailure(t *testing.T) { - client := newClient() - 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("sets the content-type properly", func(t *testing.T) { + 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") + } + }) + }) -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") - } -} + 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") + } + }) -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") - } -} + t.Run("with query", func(t *testing.T) { + client := newAPIClient() + q := url.Values{} + q.Add("antani", "mascetti") + q.Add("melandri", "conte") + req, err := client.newRequest( + context.Background(), "GET", "/", q, nil, + ) + if err != nil { + t.Fatal(err) + } + if req.URL.Query().Get("antani") != "mascetti" { + t.Fatal("expected different query string here") + } + if req.URL.Query().Get("melandri") != "conte" { + t.Fatal("expected different query string 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") - } -} + t.Run("with authorization", func(t *testing.T) { + client := newAPIClient() + client.Authorization = "deadbeef" + req, err := client.newRequest( + context.Background(), "GET", "/", nil, nil, + ) + if err != nil { + t.Fatal(err) + } + if req.Header.Get("Authorization") != client.Authorization { + t.Fatal("expected different Authorization here") + } + }) -func TestNewRequestAuthorizationHeader(t *testing.T) { - client := newClient() - client.Authorization = "deadbeef" - req, err := client.newRequest( - context.Background(), "GET", "/", nil, nil, - ) - if err != nil { - t.Fatal(err) - } - if req.Header.Get("Authorization") != client.Authorization { - t.Fatal("expected different Authorization here") - } -} + t.Run("with accept", func(t *testing.T) { + 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") + } + }) -func TestNewRequestUserAgentIsSet(t *testing.T) { - client := newClient() - req, err := client.newRequest( - context.Background(), "GET", "/", nil, nil, - ) - if err != nil { - t.Fatal(err) - } - if req.Header.Get("User-Agent") != userAgent { - t.Fatal("expected different User-Agent 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") + } + }) -func TestClientDoJSONClientDoFailure(t *testing.T) { - expected := errors.New("mocked error") - client := newClient() - client.HTTPClient = &http.Client{Transport: FakeTransport{ - Err: 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") - } -} + t.Run("with user agent", func(t *testing.T) { + client := newAPIClient() + req, err := client.newRequest( + context.Background(), "GET", "/", nil, nil, + ) + if err != nil { + t.Fatal(err) + } + if req.Header.Get("User-Agent") != userAgent { + t.Fatal("expected different User-Agent here") + } + }) + }) -func TestClientDoJSONResponseNotSuccessful(t *testing.T) { - client := newClient() - client.HTTPClient = &http.Client{Transport: FakeTransport{ - Resp: &http.Response{ - StatusCode: 401, - Body: FakeBody{}, - }, - }} - err := client.doJSON(&http.Request{URL: &url.URL{Scheme: "https", Host: "x.org"}}, nil) - if err == nil || !strings.HasPrefix(err.Error(), "httpx: request failed") { - t.Fatal("not the error we expected") - } -} + t.Run("doJSON", func(t *testing.T) { + t.Run("do failure", func(t *testing.T) { + expected := errors.New("mocked error") + client := newAPIClient() + client.HTTPClient = &mocks.HTTPClient{ + 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 TestClientDoJSONResponseReadingBodyError(t *testing.T) { - expected := errors.New("mocked error") - client := newClient() - client.HTTPClient = &http.Client{Transport: FakeTransport{ - Resp: &http.Response{ - StatusCode: 200, - Body: FakeBody{ - Err: 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") - } -} + t.Run("response is not successful (i.e., >= 400)", func(t *testing.T) { + client := newAPIClient() + client.HTTPClient = &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: 401, + Body: io.NopCloser(strings.NewReader("{}")), + }, nil + }, + } + err := client.doJSON(&http.Request{URL: &url.URL{Scheme: "https", Host: "x.org"}}, nil) + if !errors.Is(err, ErrRequestFailed) { + t.Fatal("not the error we expected", err) + } + }) -func TestClientDoJSONResponseIsNotJSON(t *testing.T) { - client := newClient() - client.HTTPClient = &http.Client{Transport: FakeTransport{ - Resp: &http.Response{ - StatusCode: 200, - Body: FakeBody{ - Err: io.EOF, - }, - }, - }} - 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") - } -} + t.Run("cannot read body", func(t *testing.T) { + expected := errors.New("mocked error") + client := newAPIClient() + client.HTTPClient = &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: 200, + Body: io.NopCloser(&mocks.Reader{ + 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") + } + }) -type httpbinheaders struct { - Headers map[string]string `json:"headers"` -} + t.Run("response is not JSON", func(t *testing.T) { + client := newAPIClient() + client.HTTPClient = &mocks.HTTPClient{ + MockDo: func(req *http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: 200, + Body: io.NopCloser(strings.NewReader("[")), + }, 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") + } + }) + }) -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") - } -} + t.Run("GetJSON", 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(`["foo", "bar"]`)) + }, + )) + defer server.Close() + ctx := context.Background() + var result []string + err := (&apiClient{ + BaseURL: server.URL, + HTTPClient: http.DefaultClient, + Logger: model.DiscardLogger, + }).GetJSON(ctx, "/", &result) + if err != nil { + t.Fatal(err) + } + if len(result) != 2 || result[0] != "foo" || result[1] != "bar" { + t.Fatal("invalid result", result) + } + }) -type httpbinpost struct { - Data string `json:"data"` -} + 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") + } + }) + }) -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) - } -} + 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) + } + }) -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") - } -} + 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") + } + }) + }) -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") - } -} + 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") + } + }) -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") - } -} + 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") + } + }) + }) -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) - }, - )) - defer server.Close() - log.SetLevel(log.DebugLevel) - ctx := context.Background() - data, err := (&apiClient{ - Authorization: "foobar", - BaseURL: server.URL, - HTTPClient: http.DefaultClient, - Logger: log.Log, - UserAgent: "ooniprobe-engine/0.1.0", - }).FetchResource(ctx, "") - if err == nil || !strings.HasSuffix(err.Error(), "400 Bad Request") { - t.Fatal("not the error we expected") - } - if len(data) != 0 { - t.Fatal("expected an empty resource") - } + 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") + } + }) }