From b9792d640c85db3af82505b50114c72460d7f36c Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 15 Jun 2021 19:17:39 +0200 Subject: [PATCH] refactor(mlablocatev2): use interfaces, remove unused fields, add docs This is a very light refactoring of the mlablocatev2 package where we do the following things: 1. use interfaces rather than depending on other pkgs where possible 2. add a missing test to the test suite 3. write more comprehensive docs (including todo-next comments) --- .../internal/mlablocate/mlablocate_test.go | 2 +- .../internal/mlablocatev2/mlablocatev2.go | 69 +++++++++++++++---- .../mlablocatev2/mlablocatev2_test.go | 47 +++++++++---- 3 files changed, 89 insertions(+), 29 deletions(-) diff --git a/internal/engine/internal/mlablocate/mlablocate_test.go b/internal/engine/internal/mlablocate/mlablocate_test.go index 7a431275fb..5ef30becbd 100644 --- a/internal/engine/internal/mlablocate/mlablocate_test.go +++ b/internal/engine/internal/mlablocate/mlablocate_test.go @@ -11,7 +11,7 @@ import ( "github.com/apex/log" ) -func TestWithoutProxy(t *testing.T) { +func TestSuccess(t *testing.T) { client := NewClient( http.DefaultClient, log.Log, diff --git a/internal/engine/internal/mlablocatev2/mlablocatev2.go b/internal/engine/internal/mlablocatev2/mlablocatev2.go index 2bad414a6f..690af85e31 100644 --- a/internal/engine/internal/mlablocatev2/mlablocatev2.go +++ b/internal/engine/internal/mlablocatev2/mlablocatev2.go @@ -1,4 +1,6 @@ -// Package mlablocatev2 implements m-lab locate services API v2. +// Package mlablocatev2 implements m-lab locate services API v2. This +// API currently only allows you to get servers for ndt7. Use the +// mlablocate package for all other m-lab tools. package mlablocatev2 import ( @@ -10,7 +12,6 @@ import ( "net/url" "regexp" - "github.com/ooni/probe-cli/v3/internal/engine/model" "github.com/ooni/probe-cli/v3/internal/iox" ) @@ -27,17 +28,40 @@ var ( ErrEmptyResponse = errors.New("mlablocatev2: empty response") ) -// Client is a client for v2 of the locate services. +// Logger is the logger expected by this package. +type Logger interface { + // Debugf formats and emits a debug message. + Debugf(format string, v ...interface{}) +} + +// HTTPClient is anything that looks like an http.Client. +type HTTPClient interface { + // Do behaves like http.Client.Do. + Do(req *http.Request) (*http.Response, error) +} + +// Client is a client for v2 of the locate services. Please use the +// NewClient factory to construct a new instance of client, otherwise +// you MUST fill all the fields marked as MANDATORY. type Client struct { - HTTPClient *http.Client - Hostname string - Logger model.Logger - Scheme string - UserAgent string + // HTTPClient is the MANDATORY http client to use + HTTPClient HTTPClient + + // Hostname is the MANDATORY hostname of the mlablocate API. + Hostname string + + // Logger is the MANDATORY logger to use. + Logger Logger + + // Scheme is the MANDATORY scheme to use (http or https). + Scheme string + + // UserAgent is the MANDATORY user-agent to use. + UserAgent string } // NewClient creates a client for v2 of the locate services. -func NewClient(httpClient *http.Client, logger model.Logger, userAgent string) Client { +func NewClient(httpClient HTTPClient, logger Logger, userAgent string) Client { return Client{ HTTPClient: httpClient, Hostname: "locate.measurementlab.net", @@ -49,8 +73,8 @@ func NewClient(httpClient *http.Client, logger model.Logger, userAgent string) C // entryRecord describes one of the boxes returned by v2 of // the locate service. It gives you the FQDN of the specific -// box along with URLs for each experiment phase. Use the -// URLs directly because they contain access tokens. +// box along with URLs for each experiment phase. You MUST +// use the URLs directly because they contain access tokens. type entryRecord struct { Machine string `json:"machine"` URLs map[string]string `json:"urls"` @@ -83,6 +107,8 @@ type resultRecord struct { // query performs a locate.measurementlab.net query // using v2 of the locate protocol. func (c Client) query(ctx context.Context, path string) (resultRecord, error) { + // TODO(bassosimone): this code should probably be + // refactored to use the httpx package. URL := &url.URL{ Scheme: c.Scheme, Host: c.Hostname, @@ -116,10 +142,22 @@ func (c Client) query(ctx context.Context, path string) (resultRecord, error) { // NDT7Result is the result of a v2 locate services query for ndt7. type NDT7Result struct { - Hostname string - Site string + // Hostname is an informative field containing the hostname + // to which you're connected. Because there are access tokens, + // you cannot use this field directly. + Hostname string + + // Site is an informative field containing the site + // to which the server belongs to. + Site string + + // WSSDownloadURL is the WebSocket URL to be used for + // performing a download over HTTPS. Note that the URL + // typically includes the required access token. WSSDownloadURL string - WSSUploadURL string + + // WSSUploadURL is like WSSDownloadURL but for the upload. + WSSUploadURL string } // QueryNDT7 performs a v2 locate services query for ndt7. @@ -137,6 +175,9 @@ func (c Client) QueryNDT7(ctx context.Context) ([]NDT7Result, error) { if r.WSSDownloadURL == "" || r.WSSUploadURL == "" { continue } + // Implementation note: we extract the hostname from the + // download URL, under the assumption that the download and + // the upload URLs have the same hostname. url, err := url.Parse(r.WSSDownloadURL) if err != nil { continue diff --git a/internal/engine/internal/mlablocatev2/mlablocatev2_test.go b/internal/engine/internal/mlablocatev2/mlablocatev2_test.go index 8650a26a95..dbb6a1aa21 100644 --- a/internal/engine/internal/mlablocatev2/mlablocatev2_test.go +++ b/internal/engine/internal/mlablocatev2/mlablocatev2_test.go @@ -13,9 +13,7 @@ import ( ) func TestSuccess(t *testing.T) { - if testing.Short() { - t.Skip("skip test in short mode") - } + // this test is ~0.5 s, so we can always run it client := NewClient(http.DefaultClient, log.Log, "miniooni/0.1.0-dev") result, err := client.QueryNDT7(context.Background()) if err != nil { @@ -26,7 +24,7 @@ func TestSuccess(t *testing.T) { } for _, entry := range result { if entry.Hostname == "" { - t.Fatal("expected non empty Machine here") + t.Fatal("expected non empty Hostname here") } if entry.Site == "" { t.Fatal("expected non=-empty Site here") @@ -47,9 +45,7 @@ func TestSuccess(t *testing.T) { } func Test404Response(t *testing.T) { - if testing.Short() { - t.Skip("skip test in short mode") - } + // this test is ~0.5 s, so we can always run it client := NewClient(http.DefaultClient, log.Log, "miniooni/0.1.0-dev") result, err := client.query(context.Background(), "nonexistent") if !errors.Is(err, ErrRequestFailed) { @@ -68,7 +64,7 @@ func TestNewRequestFailure(t *testing.T) { t.Fatal("not the error we expected") } if result.Results != nil { - t.Fatal("expected empty fqdn") + t.Fatal("expected nil results") } } @@ -83,7 +79,7 @@ func TestHTTPClientDoFailure(t *testing.T) { t.Fatal("not the error we expected") } if result.Results != nil { - t.Fatal("expected empty fqdn") + t.Fatal("expected nil results") } } @@ -105,7 +101,7 @@ func TestCannotReadBody(t *testing.T) { t.Fatal("not the error we expected") } if result.Results != nil { - t.Fatal("expected empty fqdn") + t.Fatal("expected nil results") } } @@ -127,7 +123,7 @@ func TestInvalidJSON(t *testing.T) { t.Fatal("not the error we expected") } if result.Results != nil { - t.Fatal("expected empty fqdn") + t.Fatal("expected nil results") } } @@ -149,7 +145,7 @@ func TestEmptyResponse(t *testing.T) { t.Fatal("not the error we expected") } if result != nil { - t.Fatal("expected empty fqdn") + t.Fatal("expected nil results") } } @@ -168,7 +164,7 @@ func TestNDT7QueryFails(t *testing.T) { t.Fatal("not the error we expected") } if result != nil { - t.Fatal("expected empty fqdn") + t.Fatal("expected nil results") } } @@ -191,7 +187,30 @@ func TestNDT7InvalidURLs(t *testing.T) { t.Fatal("not the error we expected") } if result != nil { - t.Fatal("expected empty fqdn") + t.Fatal("expected nil results") + } +} + +func TestNDT7EmptyURLs(t *testing.T) { + client := NewClient(http.DefaultClient, log.Log, "miniooni/0.1.0-dev") + client.HTTPClient = &http.Client{ + Transport: FakeTransport{ + Resp: &http.Response{ + StatusCode: 200, + Body: FakeBody{ + Data: []byte( + `{"results":[{"machine":"mlab3-mil04.mlab-oti.measurement-lab.org","urls":{"wss:///ndt/v7/download":"","wss:///ndt/v7/upload":""}}]}`), + Err: io.EOF, + }, + }, + }, + } + result, err := client.QueryNDT7(context.Background()) + if !errors.Is(err, ErrEmptyResponse) { + t.Fatal("not the error we expected") + } + if result != nil { + t.Fatal("expected nil results") } }