doc(oonirun): document v2.go and v2_test.go (#1565)

Closes https://github.com/ooni/probe/issues/2716

internal/enginelocate/cloudflare.go

@@ -2,7 +2,7 @@ package enginelocate
 
 import (
 	"context"
-	"net/http"
+	"net"
 	"regexp"
 	"strings"
 
@@ -12,22 +12,34 @@ import (
 
 func cloudflareIPLookup(
 	ctx context.Context,
-	httpClient *http.Client,
+	httpClient model.HTTPClient,
 	logger model.Logger,
 	userAgent string,
 	resolver model.Resolver,
 ) (string, error) {
+	// get the raw response body
 	data, err := (&httpx.APIClientTemplate{
 		BaseURL:    "https://www.cloudflare.com",
 		HTTPClient: httpClient,
 		Logger:     logger,
 		UserAgent:  model.HTTPHeaderUserAgent,
 	}).WithBodyLogging().Build().FetchResource(ctx, "/cdn-cgi/trace")
+
+	// handle the error case
 	if err != nil {
 		return model.DefaultProbeIP, err
 	}
+
+	// find the IP addr
 	r := regexp.MustCompile("(?:ip)=(.*)")
 	ip := strings.Trim(string(r.Find(data)), "ip=")
 	logger.Debugf("cloudflare: body: %s", ip)
+
+	// make sure the IP addr is valid
+	if net.ParseIP(ip) == nil {
+		return model.DefaultProbeIP, ErrInvalidIPAddress
+	}
+
+	// done!
 	return ip, nil
 }

internal/enginelocate/cloudflare_test.go

@@ -2,32 +2,234 @@ package enginelocate
 
 import (
 	"context"
+	"errors"
 	"net"
 	"net/http"
+	"net/url"
 	"testing"
 
 	"github.com/apex/log"
+	"github.com/ooni/probe-cli/v3/internal/mocks"
 	"github.com/ooni/probe-cli/v3/internal/model"
 	"github.com/ooni/probe-cli/v3/internal/netxlite"
+	"github.com/ooni/probe-cli/v3/internal/runtimex"
+	"github.com/ooni/probe-cli/v3/internal/testingx"
 )
 
-func TestIPLookupWorksUsingcloudlflare(t *testing.T) {
-	if testing.Short() {
-		t.Skip("skip test in short mode")
-	}
+// cloudflareRealisticresponse is a realistic response returned by cloudflare
+// with the IP address modified to belong to a public institution.
+var cloudflareRealisticResponse = []byte(`
+fl=270f47
+h=www.cloudflare.com
+ip=130.192.91.211
+ts=1713946961.154
+visit_scheme=https
+uag=Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:125.0) Gecko/20100101 Firefox/125.0
+colo=MXP
+sliver=none
+http=http/3
+loc=IT
+tls=TLSv1.3
+sni=plaintext
+warp=off
+gateway=off
+rbi=off
+kex=X25519
+`)
 
-	netx := &netxlite.Netx{}
-	ip, err := cloudflareIPLookup(
-		context.Background(),
-		http.DefaultClient,
-		log.Log,
-		model.HTTPHeaderUserAgent,
-		netx.NewStdlibResolver(model.DiscardLogger),
-	)
-	if err != nil {
-		t.Fatal(err)
-	}
-	if net.ParseIP(ip) == nil {
-		t.Fatalf("not an IP address: '%s'", ip)
-	}
+func TestIPLookupWorksUsingcloudlflare(t *testing.T) {
+
+	// We want to make sure the real server gives us an IP address.
+	t.Run("is working as intended when using the real server", func(t *testing.T) {
+		if testing.Short() {
+			t.Skip("skip test in short mode")
+		}
+
+		// figure out the IP address using cloudflare
+		netx := &netxlite.Netx{}
+		ip, err := cloudflareIPLookup(
+			context.Background(),
+			http.DefaultClient,
+			log.Log,
+			model.HTTPHeaderUserAgent,
+			netx.NewStdlibResolver(model.DiscardLogger),
+		)
+
+		// we expect this call to succeed
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		// we expect to get back a valid IPv4/IPv6 address
+		if net.ParseIP(ip) == nil {
+			t.Fatalf("not an IP address: '%s'", ip)
+		}
+	})
+
+	// But we also want to make sure everything is working as intended when using
+	// a local HTTP server, as well as that we can handle errors, so that we can run
+	// tests in short mode. This is done with the tests below.
+
+	t.Run("is working as intended when using a fake server", func(t *testing.T) {
+		// create a fake server returning an hardcoded IP address.
+		srv := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			w.Write(cloudflareRealisticResponse)
+		}))
+		defer srv.Close()
+
+		// create an HTTP client that uses the fake server.
+		client := &mocks.HTTPClient{
+			MockDo: func(req *http.Request) (*http.Response, error) {
+				// rewrite the request URL to be the one of the fake server
+				req.URL = runtimex.Try1(url.Parse(srv.URL))
+				return http.DefaultClient.Do(req)
+			},
+			MockCloseIdleConnections: func() {
+				http.DefaultClient.CloseIdleConnections()
+			},
+		}
+
+		// figure out the IP address using cloudflare
+		netx := &netxlite.Netx{}
+		ip, err := cloudflareIPLookup(
+			context.Background(),
+			client,
+			log.Log,
+			model.HTTPHeaderUserAgent,
+			netx.NewStdlibResolver(model.DiscardLogger),
+		)
+
+		// we expect this call to succeed
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		// we expect to get back a valid IPv4/IPv6 address
+		if net.ParseIP(ip) == nil {
+			t.Fatalf("not an IP address: '%s'", ip)
+		}
+
+		// we expect to see exactly the IP address that we want to see
+		if ip != "130.192.91.211" {
+			t.Fatal("unexpected IP address", ip)
+		}
+	})
+
+	t.Run("correctly handles network errors", func(t *testing.T) {
+		// create a fake server resetting the connection for the client.
+		srv := testingx.MustNewHTTPServer(testingx.HTTPHandlerReset())
+		defer srv.Close()
+
+		// create an HTTP client that uses the fake server.
+		client := &mocks.HTTPClient{
+			MockDo: func(req *http.Request) (*http.Response, error) {
+				// rewrite the request URL to be the one of the fake server
+				req.URL = runtimex.Try1(url.Parse(srv.URL))
+				return http.DefaultClient.Do(req)
+			},
+			MockCloseIdleConnections: func() {
+				http.DefaultClient.CloseIdleConnections()
+			},
+		}
+
+		// figure out the IP address using cloudflare
+		netx := &netxlite.Netx{}
+		ip, err := cloudflareIPLookup(
+			context.Background(),
+			client,
+			log.Log,
+			model.HTTPHeaderUserAgent,
+			netx.NewStdlibResolver(model.DiscardLogger),
+		)
+
+		// we expect to see ECONNRESET here
+		if !errors.Is(err, netxlite.ECONNRESET) {
+			t.Fatal("unexpected error", err)
+		}
+
+		// the returned IP address should be the default one
+		if ip != model.DefaultProbeIP {
+			t.Fatal("unexpected IP address", ip)
+		}
+	})
+
+	t.Run("correctly handles parsing errors", func(t *testing.T) {
+		// create a fake server returnning different keys
+		srv := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			w.Write([]byte(`ipx=130.192.91.211`)) // note: different key name
+		}))
+		defer srv.Close()
+
+		// create an HTTP client that uses the fake server.
+		client := &mocks.HTTPClient{
+			MockDo: func(req *http.Request) (*http.Response, error) {
+				// rewrite the request URL to be the one of the fake server
+				req.URL = runtimex.Try1(url.Parse(srv.URL))
+				return http.DefaultClient.Do(req)
+			},
+			MockCloseIdleConnections: func() {
+				http.DefaultClient.CloseIdleConnections()
+			},
+		}
+
+		// figure out the IP address using cloudflare
+		netx := &netxlite.Netx{}
+		ip, err := cloudflareIPLookup(
+			context.Background(),
+			client,
+			log.Log,
+			model.HTTPHeaderUserAgent,
+			netx.NewStdlibResolver(model.DiscardLogger),
+		)
+
+		// we expect to see an error indicating there's no IP address in the response
+		if !errors.Is(err, ErrInvalidIPAddress) {
+			t.Fatal("unexpected error", err)
+		}
+
+		// the returned IP address should be the default one
+		if ip != model.DefaultProbeIP {
+			t.Fatal("unexpected IP address", ip)
+		}
+	})
+
+	t.Run("correctly handles the case where the IP address is invalid", func(t *testing.T) {
+		// create a fake server returnning different keys
+		srv := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			w.Write([]byte(`ip=foobarbaz`)) // note: invalid IP address
+		}))
+		defer srv.Close()
+
+		// create an HTTP client that uses the fake server.
+		client := &mocks.HTTPClient{
+			MockDo: func(req *http.Request) (*http.Response, error) {
+				// rewrite the request URL to be the one of the fake server
+				req.URL = runtimex.Try1(url.Parse(srv.URL))
+				return http.DefaultClient.Do(req)
+			},
+			MockCloseIdleConnections: func() {
+				http.DefaultClient.CloseIdleConnections()
+			},
+		}
+
+		// figure out the IP address using cloudflare
+		netx := &netxlite.Netx{}
+		ip, err := cloudflareIPLookup(
+			context.Background(),
+			client,
+			log.Log,
+			model.HTTPHeaderUserAgent,
+			netx.NewStdlibResolver(model.DiscardLogger),
+		)
+
+		// we expect to see an error indicating there's no IP address in the response
+		if !errors.Is(err, ErrInvalidIPAddress) {
+			t.Fatal("unexpected error", err)
+		}
+
+		// the returned IP address should be the default one
+		if ip != model.DefaultProbeIP {
+			t.Fatal("unexpected IP address", ip)
+		}
+	})
 }

internal/enginelocate/fake_test.go

@@ -1,28 +0,0 @@
-package enginelocate
-
-import (
-	"net/http"
-	"time"
-
-	"github.com/ooni/probe-cli/v3/internal/netxlite"
-)
-
-type FakeTransport struct {
-	Err  error
-	Resp *http.Response
-}
-
-func (txp FakeTransport) RoundTrip(req *http.Request) (*http.Response, error) {
-	time.Sleep(10 * time.Microsecond)
-	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() {}

internal/enginelocate/invalid_test.go

@@ -2,14 +2,13 @@ package enginelocate
 
 import (
 	"context"
-	"net/http"
 
 	"github.com/ooni/probe-cli/v3/internal/model"
 )
 
 func invalidIPLookup(
 	ctx context.Context,
-	httpClient *http.Client,
+	httpClient model.HTTPClient,
 	logger model.Logger,
 	userAgent string,
 	resolver model.Resolver,

internal/enginelocate/iplookup.go

@@ -25,7 +25,7 @@ var (
 )
 
 type lookupFunc func(
-	ctx context.Context, client *http.Client,
+	ctx context.Context, client model.HTTPClient,
 	logger model.Logger, userAgent string,
 	resolver model.Resolver,
 ) (string, error)

internal/enginelocate/stun.go

@@ -3,7 +3,6 @@ package enginelocate
 import (
 	"context"
 	"net"
-	"net/http"
 
 	"github.com/ooni/probe-cli/v3/internal/model"
 	"github.com/ooni/probe-cli/v3/internal/netxlite"
@@ -86,7 +85,7 @@ func stunIPLookup(ctx context.Context, config stunConfig) (string, error) {
 
 func stunEkigaIPLookup(
 	ctx context.Context,
-	httpClient *http.Client,
+	httpClient model.HTTPClient,
 	logger model.Logger,
 	userAgent string,
 	resolver model.Resolver,
@@ -100,7 +99,7 @@ func stunEkigaIPLookup(
 
 func stunGoogleIPLookup(
 	ctx context.Context,
-	httpClient *http.Client,
+	httpClient model.HTTPClient,
 	logger model.Logger,
 	userAgent string,
 	resolver model.Resolver,

internal/enginelocate/ubuntu.go

@@ -3,7 +3,7 @@ package enginelocate
 import (
 	"context"
 	"encoding/xml"
-	"net/http"
+	"net"
 
 	"github.com/ooni/probe-cli/v3/internal/httpx"
 	"github.com/ooni/probe-cli/v3/internal/model"
@@ -16,25 +16,39 @@ type ubuntuResponse struct {
 
 func ubuntuIPLookup(
 	ctx context.Context,
-	httpClient *http.Client,
+	httpClient model.HTTPClient,
 	logger model.Logger,
 	userAgent string,
 	resolver model.Resolver,
 ) (string, error) {
+	// read the HTTP response body
 	data, err := (&httpx.APIClientTemplate{
 		BaseURL:    "https://geoip.ubuntu.com/",
 		HTTPClient: httpClient,
 		Logger:     logger,
 		UserAgent:  userAgent,
 	}).WithBodyLogging().Build().FetchResource(ctx, "/lookup")
+
+	// handle the error case
 	if err != nil {
 		return model.DefaultProbeIP, err
 	}
+
+	// parse the XML
 	logger.Debugf("ubuntu: body: %s", string(data))
 	var v ubuntuResponse
 	err = xml.Unmarshal(data, &v)
+
+	// handle the error case
 	if err != nil {
 		return model.DefaultProbeIP, err
 	}
+
+	// make sure the IP addr is valid
+	if net.ParseIP(v.IP) == nil {
+		return model.DefaultProbeIP, ErrInvalidIPAddress
+	}
+
+	// handle the success case
 	return v.IP, nil
 }

internal/enginelocate/ubuntu_test.go

@@ -2,56 +2,268 @@ package enginelocate
 
 import (
 	"context"
-	"io"
+	"errors"
 	"net"
 	"net/http"
+	"net/url"
 	"strings"
 	"testing"
 
 	"github.com/apex/log"
+	"github.com/ooni/probe-cli/v3/internal/mocks"
 	"github.com/ooni/probe-cli/v3/internal/model"
 	"github.com/ooni/probe-cli/v3/internal/netxlite"
+	"github.com/ooni/probe-cli/v3/internal/runtimex"
+	"github.com/ooni/probe-cli/v3/internal/testingx"
 )
 
-func TestUbuntuParseError(t *testing.T) {
-	netx := &netxlite.Netx{}
-	ip, err := ubuntuIPLookup(
-		context.Background(),
-		&http.Client{Transport: FakeTransport{
-			Resp: &http.Response{
-				StatusCode: 200,
-				Body:       io.NopCloser(strings.NewReader("<")),
-			},
-		}},
-		log.Log,
-		model.HTTPHeaderUserAgent,
-		netx.NewStdlibResolver(model.DiscardLogger),
-	)
-	if err == nil || !strings.HasPrefix(err.Error(), "XML syntax error") {
-		t.Fatalf("not the error we expected: %+v", err)
-	}
-	if ip != model.DefaultProbeIP {
-		t.Fatalf("not the expected IP address: %s", ip)
-	}
-}
+// ubuntuRealisticresponse is a realistic response returned by cloudflare
+// with the IP address modified to belong to a public institution.
+var ubuntuRealisticresponse = []byte(`
+<Response>
+<Ip>130.192.91.211</Ip>
+<Status>OK</Status>
+<CountryCode>IT</CountryCode>
+<CountryCode3>ITA</CountryCode3>
+<CountryName>Italy</CountryName>
+<RegionCode>09</RegionCode>
+<RegionName>Lombardia</RegionName>
+<City>Sesto San Giovanni</City>
+<ZipPostalCode>20099</ZipPostalCode>
+<Latitude>45.5349</Latitude>
+<Longitude>9.2295</Longitude>
+<AreaCode>0</AreaCode>
+<TimeZone>Europe/Rome</TimeZone>
+</Response>
+`)
 
 func TestIPLookupWorksUsingUbuntu(t *testing.T) {
-	if testing.Short() {
-		t.Skip("skip test in short mode")
-	}
 
-	netx := &netxlite.Netx{}
-	ip, err := ubuntuIPLookup(
-		context.Background(),
-		http.DefaultClient,
-		log.Log,
-		model.HTTPHeaderUserAgent,
-		netx.NewStdlibResolver(model.DiscardLogger),
-	)
-	if err != nil {
-		t.Fatal(err)
-	}
-	if net.ParseIP(ip) == nil {
-		t.Fatalf("not an IP address: '%s'", ip)
-	}
+	// We want to make sure the real server gives us an IP address.
+	t.Run("is working as intended when using the real server", func(t *testing.T) {
+		if testing.Short() {
+			t.Skip("skip test in short mode")
+		}
+
+		netx := &netxlite.Netx{}
+		ip, err := ubuntuIPLookup(
+			context.Background(),
+			http.DefaultClient,
+			log.Log,
+			model.HTTPHeaderUserAgent,
+			netx.NewStdlibResolver(model.DiscardLogger),
+		)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if net.ParseIP(ip) == nil {
+			t.Fatalf("not an IP address: '%s'", ip)
+		}
+	})
+
+	// But we also want to make sure everything is working as intended when using
+	// a local HTTP server, as well as that we can handle errors, so that we can run
+	// tests in short mode. This is done with the tests below.
+
+	t.Run("is working as intended when using a fake server", func(t *testing.T) {
+		// create a fake server returning an hardcoded IP address.
+		srv := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			w.Write(ubuntuRealisticresponse)
+		}))
+		defer srv.Close()
+
+		// create an HTTP client that uses the fake server.
+		client := &mocks.HTTPClient{
+			MockDo: func(req *http.Request) (*http.Response, error) {
+				// rewrite the request URL to be the one of the fake server
+				req.URL = runtimex.Try1(url.Parse(srv.URL))
+				return http.DefaultClient.Do(req)
+			},
+			MockCloseIdleConnections: func() {
+				http.DefaultClient.CloseIdleConnections()
+			},
+		}
+
+		// figure out the IP address using ubuntu
+		netx := &netxlite.Netx{}
+		ip, err := ubuntuIPLookup(
+			context.Background(),
+			client,
+			log.Log,
+			model.HTTPHeaderUserAgent,
+			netx.NewStdlibResolver(model.DiscardLogger),
+		)
+
+		// we expect this call to succeed
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		// we expect to get back a valid IPv4/IPv6 address
+		if net.ParseIP(ip) == nil {
+			t.Fatalf("not an IP address: '%s'", ip)
+		}
+
+		// we expect to see exactly the IP address that we want to see
+		if ip != "130.192.91.211" {
+			t.Fatal("unexpected IP address", ip)
+		}
+	})
+
+	t.Run("correctly handles network errors", func(t *testing.T) {
+		srv := testingx.MustNewHTTPServer(testingx.HTTPHandlerReset())
+		defer srv.Close()
+
+		// create an HTTP client that uses the fake server.
+		client := &mocks.HTTPClient{
+			MockDo: func(req *http.Request) (*http.Response, error) {
+				// rewrite the request URL to be the one of the fake server
+				req.URL = runtimex.Try1(url.Parse(srv.URL))
+				return http.DefaultClient.Do(req)
+			},
+			MockCloseIdleConnections: func() {
+				http.DefaultClient.CloseIdleConnections()
+			},
+		}
+
+		// figure out the IP address using ubuntu
+		netx := &netxlite.Netx{}
+		ip, err := ubuntuIPLookup(
+			context.Background(),
+			client,
+			log.Log,
+			model.HTTPHeaderUserAgent,
+			netx.NewStdlibResolver(model.DiscardLogger),
+		)
+
+		// we expect to see ECONNRESET here
+		if !errors.Is(err, netxlite.ECONNRESET) {
+			t.Fatal("unexpected error", err)
+		}
+
+		// the returned IP address should be the default one
+		if ip != model.DefaultProbeIP {
+			t.Fatal("unexpected IP address", ip)
+		}
+	})
+
+	t.Run("correctly handles parsing errors", func(t *testing.T) {
+		// create a fake server returnning different keys
+		srv := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			w.Write([]byte(`<`)) // note: invalid XML
+		}))
+		defer srv.Close()
+
+		// create an HTTP client that uses the fake server.
+		client := &mocks.HTTPClient{
+			MockDo: func(req *http.Request) (*http.Response, error) {
+				// rewrite the request URL to be the one of the fake server
+				req.URL = runtimex.Try1(url.Parse(srv.URL))
+				return http.DefaultClient.Do(req)
+			},
+			MockCloseIdleConnections: func() {
+				http.DefaultClient.CloseIdleConnections()
+			},
+		}
+
+		// figure out the IP address using ubuntu
+		netx := &netxlite.Netx{}
+		ip, err := ubuntuIPLookup(
+			context.Background(),
+			client,
+			log.Log,
+			model.HTTPHeaderUserAgent,
+			netx.NewStdlibResolver(model.DiscardLogger),
+		)
+
+		// we expect to see an XML parsing error here
+		if err == nil || !strings.HasPrefix(err.Error(), "XML syntax error") {
+			t.Fatalf("not the error we expected: %+v", err)
+		}
+
+		// the returned IP address should be the default one
+		if ip != model.DefaultProbeIP {
+			t.Fatal("unexpected IP address", ip)
+		}
+	})
+
+	t.Run("correctly handles missing IP address in a valid XML document", func(t *testing.T) {
+		// create a fake server returnning different keys
+		srv := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			w.Write([]byte(`<Response></Response>`)) // note: missing IP address
+		}))
+		defer srv.Close()
+
+		// create an HTTP client that uses the fake server.
+		client := &mocks.HTTPClient{
+			MockDo: func(req *http.Request) (*http.Response, error) {
+				// rewrite the request URL to be the one of the fake server
+				req.URL = runtimex.Try1(url.Parse(srv.URL))
+				return http.DefaultClient.Do(req)
+			},
+			MockCloseIdleConnections: func() {
+				http.DefaultClient.CloseIdleConnections()
+			},
+		}
+
+		// figure out the IP address using ubuntu
+		netx := &netxlite.Netx{}
+		ip, err := ubuntuIPLookup(
+			context.Background(),
+			client,
+			log.Log,
+			model.HTTPHeaderUserAgent,
+			netx.NewStdlibResolver(model.DiscardLogger),
+		)
+
+		// we expect to see an error indicating there's no IP address in the response
+		if !errors.Is(err, ErrInvalidIPAddress) {
+			t.Fatal("unexpected error", err)
+		}
+
+		// the returned IP address should be the default one
+		if ip != model.DefaultProbeIP {
+			t.Fatal("unexpected IP address", ip)
+		}
+	})
+
+	t.Run("correctly handles the case where the IP address is invalid", func(t *testing.T) {
+		// create a fake server returnning different keys
+		srv := testingx.MustNewHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			w.Write([]byte(`<Response><Ip>foobarbaz</Ip></Response>`)) // note: not an IP address
+		}))
+		defer srv.Close()
+
+		// create an HTTP client that uses the fake server.
+		client := &mocks.HTTPClient{
+			MockDo: func(req *http.Request) (*http.Response, error) {
+				// rewrite the request URL to be the one of the fake server
+				req.URL = runtimex.Try1(url.Parse(srv.URL))
+				return http.DefaultClient.Do(req)
+			},
+			MockCloseIdleConnections: func() {
+				http.DefaultClient.CloseIdleConnections()
+			},
+		}
+
+		// figure out the IP address using ubuntu
+		netx := &netxlite.Netx{}
+		ip, err := ubuntuIPLookup(
+			context.Background(),
+			client,
+			log.Log,
+			model.HTTPHeaderUserAgent,
+			netx.NewStdlibResolver(model.DiscardLogger),
+		)
+
+		// we expect to see an error indicating there's no IP address in the response
+		if !errors.Is(err, ErrInvalidIPAddress) {
+			t.Fatal("unexpected error", err)
+		}
+
+		// the returned IP address should be the default one
+		if ip != model.DefaultProbeIP {
+			t.Fatal("unexpected IP address", ip)
+		}
+	})
 }

internal/oonirun/v2.go

@@ -60,9 +60,6 @@ type V2Nettest struct {
 	TestName string `json:"test_name"`
 }
 
-// ErrHTTPRequestFailed indicates that an HTTP request failed.
-var ErrHTTPRequestFailed = errors.New("oonirun: HTTP request failed")
-
 // getV2DescriptorFromHTTPSURL GETs a v2Descriptor instance from
 // a static URL (e.g., from a GitHub repo or from a Gist).
 func getV2DescriptorFromHTTPSURL(ctx context.Context, client model.HTTPClient,
@@ -96,7 +93,11 @@ const v2DescriptorCacheKey = "oonirun-v2.state"
 
 // v2DescriptorCacheLoad loads the v2DescriptorCache.
 func v2DescriptorCacheLoad(fsstore model.KeyValueStore) (*v2DescriptorCache, error) {
+	// attempt to access the cache
 	data, err := fsstore.Get(v2DescriptorCacheKey)
+
+	// if there's a miss either create a new descriptor or return the
+	// error if it's something I/O related
 	if err != nil {
 		if errors.Is(err, kvstore.ErrNoSuchKey) {
 			cache := &v2DescriptorCache{
@@ -106,13 +107,19 @@ func v2DescriptorCacheLoad(fsstore model.KeyValueStore) (*v2DescriptorCache, err
 		}
 		return nil, err
 	}
+
+	// transform the raw descriptor into a struct
 	var cache v2DescriptorCache
 	if err := json.Unmarshal(data, &cache); err != nil {
 		return nil, err
 	}
+
+	// handle the case where there are no entries inside the on-disk cache
+	// by properly initializing to a non-nil map
 	if cache.Entries == nil {
 		cache.Entries = make(map[string]*V2Descriptor)
 	}
+
 	return &cache, nil
 }
 
@@ -168,13 +175,18 @@ func V2MeasureDescriptor(ctx context.Context, config *LinkConfig, desc *V2Descri
 		// more robust in terms of the implementation.
 		return ErrNilDescriptor
 	}
+
 	logger := config.Session.Logger()
+
 	for _, nettest := range desc.Nettests {
+		// early handling of the case where the test name is empty
 		if nettest.TestName == "" {
 			logger.Warn("oonirun: nettest name cannot be empty")
 			v2CountEmptyNettestNames.Add(1)
 			continue
 		}
+
+		// construct an experiment from the current nettest
 		exp := &Experiment{
 			Annotations:            config.Annotations,
 			ExtraOptions:           nettest.Options,
@@ -193,12 +205,15 @@ func V2MeasureDescriptor(ctx context.Context, config *LinkConfig, desc *V2Descri
 			newSaverFn:             nil,
 			newInputProcessorFn:    nil,
 		}
+
+		// actually run the experiment
 		if err := exp.Run(ctx); err != nil {
 			logger.Warnf("cannot run experiment: %s", err.Error())
 			v2CountFailedExperiments.Add(1)
 			continue
 		}
 	}
+
 	return nil
 }
 
@@ -209,14 +224,25 @@ var ErrNeedToAcceptChanges = errors.New("oonirun: need to accept changes")
 
 // v2DescriptorDiff shows what changed between the old and the new descriptors.
 func v2DescriptorDiff(oldValue, newValue *V2Descriptor, URL string) string {
+	// JSON serialize old descriptor
 	oldData, err := json.MarshalIndent(oldValue, "", "  ")
 	runtimex.PanicOnError(err, "json.MarshalIndent failed unexpectedly")
+
+	// JSON serialize new descriptor
 	newData, err := json.MarshalIndent(newValue, "", "  ")
 	runtimex.PanicOnError(err, "json.MarshalIndent failed unexpectedly")
+
+	// make sure the serializations are newline-terminated
 	oldString, newString := string(oldData)+"\n", string(newData)+"\n"
+
+	// generate names for the final diff
 	oldFile := "OLD " + URL
 	newFile := "NEW " + URL
+
+	// compute the edits to update from the old to the new descriptor
 	edits := myers.ComputeEdits(span.URIFromPath(oldFile), oldString, newString)
+
+	// transform the edits and obtain an unified diff
 	return fmt.Sprint(gotextdiff.ToUnified(oldFile, newFile, oldString, edits))
 }
 
@@ -233,25 +259,39 @@ func v2DescriptorDiff(oldValue, newValue *V2Descriptor, URL string) string {
 func v2MeasureHTTPS(ctx context.Context, config *LinkConfig, URL string) error {
 	logger := config.Session.Logger()
 	logger.Infof("oonirun/v2: running %s", URL)
+
+	// load the descriptor from the cache
 	cache, err := v2DescriptorCacheLoad(config.KVStore)
 	if err != nil {
 		return err
 	}
+
+	// pull a possibly new descriptor without updating the old descriptor
 	clnt := config.Session.DefaultHTTPClient()
 	oldValue, newValue, err := cache.PullChangesWithoutSideEffects(ctx, clnt, logger, URL)
 	if err != nil {
 		return err
 	}
+
+	// compare the new descriptor to the old descriptor
 	diff := v2DescriptorDiff(oldValue, newValue, URL)
+
+	// possibly stop if configured to ask for permission when accepting changes
 	if !config.AcceptChanges && diff != "" {
 		logger.Warnf("oonirun: %s changed as follows:\n\n%s", URL, diff)
 		logger.Warnf("oonirun: we are not going to run this link until you accept changes")
 		return ErrNeedToAcceptChanges
 	}
+
+	// in case there are changes, update the descriptor
 	if diff != "" {
 		if err := cache.Update(config.KVStore, URL, newValue); err != nil {
 			return err
 		}
 	}
-	return V2MeasureDescriptor(ctx, config, newValue) // handles nil newValue gracefully
+
+	// measure using the possibly-new descriptor
+	//
+	// note: this function gracefully handles nil values
+	return V2MeasureDescriptor(ctx, config, newValue)
 }

internal/oonirun/v2_test.go

@@ -12,10 +12,13 @@ import (
 	"github.com/ooni/probe-cli/v3/internal/kvstore"
 	"github.com/ooni/probe-cli/v3/internal/mocks"
 	"github.com/ooni/probe-cli/v3/internal/model"
+	"github.com/ooni/probe-cli/v3/internal/netxlite"
 	"github.com/ooni/probe-cli/v3/internal/runtimex"
+	"github.com/ooni/probe-cli/v3/internal/testingx"
 )
 
 func TestOONIRunV2LinkCommonCase(t *testing.T) {
+	// make a local server that returns a reasonable descriptor for the example experiment
 	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		descriptor := &V2Descriptor{
 			Name:        "",
@@ -33,8 +36,10 @@ func TestOONIRunV2LinkCommonCase(t *testing.T) {
 		runtimex.PanicOnError(err, "json.Marshal failed")
 		w.Write(data)
 	}))
+
 	defer server.Close()
 	ctx := context.Background()
+
 	config := &LinkConfig{
 		AcceptChanges: true, // avoid "oonirun: need to accept changes" error
 		Annotations: map[string]string{
@@ -42,19 +47,24 @@ func TestOONIRunV2LinkCommonCase(t *testing.T) {
 		},
 		KVStore:     &kvstore.Memory{},
 		MaxRuntime:  0,
-		NoCollector: true,
+		NoCollector: true, // disable collector so we don't submit
 		NoJSON:      true,
 		Random:      false,
 		ReportFile:  "",
 		Session:     newMinimalFakeSession(),
 	}
+
+	// create a link runner for the local server URL
 	r := NewLinkRunner(config, server.URL)
+
+	// run and verify that we could run without getting errors
 	if err := r.Run(ctx); err != nil {
 		t.Fatal(err)
 	}
 }
 
 func TestOONIRunV2LinkCannotUpdateCache(t *testing.T) {
+	// make a server that returns a minimal descriptor for the example experiment
 	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		descriptor := &V2Descriptor{
 			Name:        "",
@@ -72,8 +82,12 @@ func TestOONIRunV2LinkCannotUpdateCache(t *testing.T) {
 		runtimex.PanicOnError(err, "json.Marshal failed")
 		w.Write(data)
 	}))
+
 	defer server.Close()
 	ctx := context.Background()
+
+	// create with a key value store that returns an empty cache and fails to update
+	// the cache afterwards such that we can see if we detect such an error
 	expected := errors.New("mocked")
 	config := &LinkConfig{
 		AcceptChanges: true, // avoid "oonirun: need to accept changes" error
@@ -95,14 +109,21 @@ func TestOONIRunV2LinkCannotUpdateCache(t *testing.T) {
 		ReportFile:  "",
 		Session:     newMinimalFakeSession(),
 	}
+
+	// create new runner for the local server URL
 	r := NewLinkRunner(config, server.URL)
+
+	// attempt to run the link
 	err := r.Run(ctx)
+
+	// make sure we exactly got the cache updating error
 	if !errors.Is(err, expected) {
 		t.Fatal("unexpected err", err)
 	}
 }
 
 func TestOONIRunV2LinkWithoutAcceptChanges(t *testing.T) {
+	// make a local server that would return a reasonable descriptor
 	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		descriptor := &V2Descriptor{
 			Name:        "",
@@ -120,8 +141,11 @@ func TestOONIRunV2LinkWithoutAcceptChanges(t *testing.T) {
 		runtimex.PanicOnError(err, "json.Marshal failed")
 		w.Write(data)
 	}))
+
 	defer server.Close()
 	ctx := context.Background()
+
+	// create a minimal link configuration
 	config := &LinkConfig{
 		AcceptChanges: false, // should see "oonirun: need to accept changes" error
 		Annotations: map[string]string{
@@ -135,19 +159,29 @@ func TestOONIRunV2LinkWithoutAcceptChanges(t *testing.T) {
 		ReportFile:  "",
 		Session:     newMinimalFakeSession(),
 	}
+
+	// create a new runner for the local server URL
 	r := NewLinkRunner(config, server.URL)
+
+	// attempt to run the link
 	err := r.Run(ctx)
+
+	// make sure the error indicates we need to accept changes
 	if !errors.Is(err, ErrNeedToAcceptChanges) {
 		t.Fatal("unexpected err", err)
 	}
 }
 
 func TestOONIRunV2LinkNilDescriptor(t *testing.T) {
+	// create a local server that returns a literal "null" as the JSON descriptor
 	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		w.Write([]byte("null"))
 	}))
+
 	defer server.Close()
 	ctx := context.Background()
+
+	// create a minimal link configuration
 	config := &LinkConfig{
 		AcceptChanges: true, // avoid "oonirun: need to accept changes" error
 		Annotations: map[string]string{
@@ -161,14 +195,24 @@ func TestOONIRunV2LinkNilDescriptor(t *testing.T) {
 		ReportFile:  "",
 		Session:     newMinimalFakeSession(),
 	}
+
+	// attempt to run the link at the local server
 	r := NewLinkRunner(config, server.URL)
+
+	// make sure we correctly handled an invalid "null" descriptor
 	if err := r.Run(ctx); err != nil {
 		t.Fatal(err)
+		t.Fatal("unexpected error", err)
 	}
 }
 
 func TestOONIRunV2LinkEmptyTestName(t *testing.T) {
+	// load the count of the number of cases where the test name was empty so we can
+	// later on check whether this count has increased due to running this test
 	emptyTestNamesPrev := v2CountEmptyNettestNames.Load()
+
+	// create a local server that will respond with a minimal descriptor that
+	// actually contains an empty test name, which is what we want to test
 	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		descriptor := &V2Descriptor{
 			Name:        "",
@@ -186,8 +230,11 @@ func TestOONIRunV2LinkEmptyTestName(t *testing.T) {
 		runtimex.PanicOnError(err, "json.Marshal failed")
 		w.Write(data)
 	}))
+
 	defer server.Close()
 	ctx := context.Background()
+
+	// create a minimal link configuration
 	config := &LinkConfig{
 		AcceptChanges: true, // avoid "oonirun: need to accept changes" error
 		Annotations: map[string]string{
@@ -201,30 +248,116 @@ func TestOONIRunV2LinkEmptyTestName(t *testing.T) {
 		ReportFile:  "",
 		Session:     newMinimalFakeSession(),
 	}
+
+	// construct a link runner relative to the local server URL
 	r := NewLinkRunner(config, server.URL)
+
+	// attempt to run and verify there's no error (the code only emits a warning in this case)
 	if err := r.Run(ctx); err != nil {
 		t.Fatal(err)
 	}
+
+	// make sure the loop for running nettests continued where we expected it to do so
 	if v2CountEmptyNettestNames.Load() != emptyTestNamesPrev+1 {
 		t.Fatal("expected to see 1 more instance of empty nettest names")
 	}
 }
 
+func TestOONIRunV2LinkConnectionResetByPeer(t *testing.T) {
+	// create a local server that will reset the connection immediately.
+	// actually contains an empty test name, which is what we want to test
+	server := testingx.MustNewHTTPServer(testingx.HTTPHandlerReset())
+
+	defer server.Close()
+	ctx := context.Background()
+
+	// create a minimal link configuration
+	config := &LinkConfig{
+		AcceptChanges: true, // avoid "oonirun: need to accept changes" error
+		Annotations: map[string]string{
+			"platform": "linux",
+		},
+		KVStore:     &kvstore.Memory{},
+		MaxRuntime:  0,
+		NoCollector: true,
+		NoJSON:      true,
+		Random:      false,
+		ReportFile:  "",
+		Session:     newMinimalFakeSession(),
+	}
+
+	// construct a link runner relative to the local server URL
+	r := NewLinkRunner(config, server.URL)
+
+	// attempt to run and verify we got ECONNRESET
+	if err := r.Run(ctx); !errors.Is(err, netxlite.ECONNRESET) {
+		t.Fatal("unexpected error", err)
+	}
+}
+
+func TestOONIRunV2LinkNonParseableJSON(t *testing.T) {
+	// create a local server that will respond with a non-parseable JSON.
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Write([]byte(`{`))
+	}))
+
+	defer server.Close()
+	ctx := context.Background()
+
+	// create a minimal link configuration
+	config := &LinkConfig{
+		AcceptChanges: true, // avoid "oonirun: need to accept changes" error
+		Annotations: map[string]string{
+			"platform": "linux",
+		},
+		KVStore:     &kvstore.Memory{},
+		MaxRuntime:  0,
+		NoCollector: true,
+		NoJSON:      true,
+		Random:      false,
+		ReportFile:  "",
+		Session:     newMinimalFakeSession(),
+	}
+
+	// construct a link runner relative to the local server URL
+	r := NewLinkRunner(config, server.URL)
+
+	// attempt to run and verify there's a JSON parsing error
+	if err := r.Run(ctx); err == nil || err.Error() != "unexpected end of JSON input" {
+		t.Fatal("unexpected error", err)
+	}
+}
+
 func TestV2MeasureDescriptor(t *testing.T) {
+
 	t.Run("with nil descriptor", func(t *testing.T) {
 		ctx := context.Background()
 		config := &LinkConfig{}
+
+		// invoke the function with a nil descriptor and make sure the code
+		// is correctly handling this specific case by returnning error
 		err := V2MeasureDescriptor(ctx, config, nil)
+
 		if !errors.Is(err, ErrNilDescriptor) {
 			t.Fatal("unexpected err", err)
 		}
 	})
 
 	t.Run("with failing experiment", func(t *testing.T) {
+		// load the previous count of failed experiments so we can check that it increased later
 		previousFailedExperiments := v2CountFailedExperiments.Load()
+
 		expected := errors.New("mocked error")
+
 		ctx := context.Background()
 		sess := newMinimalFakeSession()
+
+		// create a mocked submitter that will panic in case we try to submit, such that
+		// this test fails with a panic if we go as far as attempting to submit
+		//
+		// Note: the convention is that we do not submit experiment results when the
+		// experiment measurement function returns a non-nil error, since such an error
+		// represents a fundamental failure in setting up the experiment
 		sess.MockNewSubmitter = func(ctx context.Context) (model.Submitter, error) {
 			subm := &mocks.Submitter{
 				MockSubmit: func(ctx context.Context, m *model.Measurement) error {
@@ -233,6 +366,9 @@ func TestV2MeasureDescriptor(t *testing.T) {
 			}
 			return subm, nil
 		}
+
+		// mock an experiment builder where we have the measurement function fail by returning
+		// an error, which has the meaning indicated in the previous comment
 		sess.MockNewExperimentBuilder = func(name string) (model.ExperimentBuilder, error) {
 			eb := &mocks.ExperimentBuilder{
 				MockInputPolicy: func() model.InputPolicy {
@@ -258,6 +394,8 @@ func TestV2MeasureDescriptor(t *testing.T) {
 			}
 			return eb, nil
 		}
+
+		// create a mostly empty config referring to the session
 		config := &LinkConfig{
 			AcceptChanges: false,
 			Annotations:   map[string]string{},
@@ -269,6 +407,8 @@ func TestV2MeasureDescriptor(t *testing.T) {
 			ReportFile:    "",
 			Session:       sess,
 		}
+
+		// create a mostly empty descriptor referring to the example experiment
 		descr := &V2Descriptor{
 			Name:        "",
 			Description: "",
@@ -279,10 +419,18 @@ func TestV2MeasureDescriptor(t *testing.T) {
 				TestName: "example",
 			}},
 		}
+
+		// attempt to measure this descriptor
 		err := V2MeasureDescriptor(ctx, config, descr)
+
+		// here we do not expect to see an error because the implementation continues
+		// until it has run all experiments and just emits warning messages
 		if err != nil {
 			t.Fatal(err)
 		}
+
+		// however there's also a count of the number of times we failed to load
+		// an experiment and we use that to make sure the code failed where we expected
 		if v2CountFailedExperiments.Load() != previousFailedExperiments+1 {
 			t.Fatal("expected to see a failed experiment")
 		}
@@ -290,9 +438,13 @@ func TestV2MeasureDescriptor(t *testing.T) {
 }
 
 func TestV2MeasureHTTPS(t *testing.T) {
+
 	t.Run("when we cannot load from cache", func(t *testing.T) {
 		expected := errors.New("mocked error")
 		ctx := context.Background()
+
+		// construct the link configuration with a key-value store that fails
+		// with a well-know error when attempting to load.
 		config := &LinkConfig{
 			AcceptChanges: false,
 			Annotations:   map[string]string{},
@@ -308,15 +460,22 @@ func TestV2MeasureHTTPS(t *testing.T) {
 			ReportFile:  "",
 			Session:     newMinimalFakeSession(),
 		}
+
+		// attempt to measure with the given config (there's no need to pass an URL
+		// here because we should fail to load from the cache first)
 		err := v2MeasureHTTPS(ctx, config, "")
+
+		// verify that we've actually got the expected error
 		if !errors.Is(err, expected) {
 			t.Fatal("unexpected err", err)
 		}
 	})
 
 	t.Run("when we cannot pull changes", func(t *testing.T) {
+		// create and immediately cancel a context so that HTTP would fail
 		ctx, cancel := context.WithCancel(context.Background())
 		cancel() // fail immediately
+
 		config := &LinkConfig{
 			AcceptChanges: false,
 			Annotations:   map[string]string{},
@@ -328,25 +487,39 @@ func TestV2MeasureHTTPS(t *testing.T) {
 			ReportFile:    "",
 			Session:       newMinimalFakeSession(),
 		}
-		err := v2MeasureHTTPS(ctx, config, "https://example.com") // should not use URL
+
+		// attempt to measure with a random URL (which is fine since we shouldn't use it)
+		err := v2MeasureHTTPS(ctx, config, "https://example.com")
+
+		// make sure that we've actually go the expected error
 		if !errors.Is(err, context.Canceled) {
 			t.Fatal("unexpected err", err)
 		}
 	})
+
 }
 
 func TestV2DescriptorCacheLoad(t *testing.T) {
-	t.Run("cannot unmarshal cache content", func(t *testing.T) {
+
+	t.Run("handle the case where we cannot unmarshal the cache content", func(t *testing.T) {
+		// write an invalid serialized JSON into the cache
 		fsstore := &kvstore.Memory{}
 		if err := fsstore.Set(v2DescriptorCacheKey, []byte("{")); err != nil {
 			t.Fatal(err)
 		}
+
+		// attempt to load descriptors
 		cache, err := v2DescriptorCacheLoad(fsstore)
+
+		// make sure we cannot unmarshal
 		if err == nil || err.Error() != "unexpected end of JSON input" {
 			t.Fatal("unexpected err", err)
 		}
+
+		// make sure the returned cache is nil
 		if cache != nil {
 			t.Fatal("expected nil cache")
 		}
 	})
+
 }