refactor(mlablocatev2): use interfaces, add missing tests, add docs (#384)

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/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,

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
+	// HTTPClient is the MANDATORY http client to use
+	HTTPClient HTTPClient
+
+	// Hostname is the MANDATORY hostname of the mlablocate API.
 	Hostname string
-	Logger     model.Logger
+
+	// 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,9 +142,21 @@ 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 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 is like WSSDownloadURL but for the upload.
 	WSSUploadURL string
 }
 
@@ -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

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")
 	}
 }