refactor(netxlite): make sure we always use netxmocks (#399)

* refactor(netxlite): make sure we always use netmocks

While there, improve logging and make sure we test 100% of the
package with unit tests only. (We don't need to have integration
testing in this package because it's fairly simple/obvious.)

Part of https://github.com/ooni/probe/issues/1505

* further improve logs

internal/netxlite/dialer.go

@@ -38,8 +38,7 @@ func (d *DialerResolver) DialContext(ctx context.Context, network, address strin
 	if err != nil {
 		return nil, err
 	}
-	var addrs []string
+	addrs, err := d.lookupHost(ctx, onlyhost)
-	addrs, err = d.lookupHost(ctx, onlyhost)
 	if err != nil {
 		return nil, err
 	}
@@ -67,9 +66,12 @@ func (d *DialerResolver) lookupHost(ctx context.Context, hostname string) ([]str
 	return d.Resolver.LookupHost(ctx, hostname)
 }
 
-// DialerLogger is a Dialer with logging
+// DialerLogger is a Dialer with logging.
 type DialerLogger struct {
-	Dialer
+	// Dialer is the underlying dialer.
+	Dialer Dialer
+
+	// Logger is the underlying logger.
 	Logger Logger
 }
 
@@ -80,7 +82,11 @@ func (d *DialerLogger) DialContext(ctx context.Context, network, address string)
 	d.Logger.Debugf("dial %s/%s...", address, network)
 	start := time.Now()
 	conn, err := d.Dialer.DialContext(ctx, network, address)
-	stop := time.Now()
+	elapsed := time.Since(start)
-	d.Logger.Debugf("dial %s/%s... %+v in %s", address, network, err, stop.Sub(start))
+	if err != nil {
-	return conn, err
+		d.Logger.Debugf("dial %s/%s... %s in %s", address, network, err, elapsed)
+		return nil, err
+	}
+	d.Logger.Debugf("dial %s/%s... ok in %s", address, network, elapsed)
+	return conn, nil
 }

internal/netxlite/dialer_test.go

@@ -14,10 +14,10 @@ import (
 )
 
 func TestDialerResolverNoPort(t *testing.T) {
-	dialer := &DialerResolver{Dialer: new(net.Dialer), Resolver: DefaultResolver}
+	dialer := &DialerResolver{Dialer: &net.Dialer{}, Resolver: DefaultResolver}
-	conn, err := dialer.DialContext(context.Background(), "tcp", "antani.ooni.nu")
+	conn, err := dialer.DialContext(context.Background(), "tcp", "ooni.nu")
-	if err == nil {
+	if err == nil || !strings.HasSuffix(err.Error(), "missing port in address") {
-		t.Fatal("expected an error here")
+		t.Fatal("not the error we expected", err)
 	}
 	if conn != nil {
 		t.Fatal("expected a nil conn here")
@@ -25,8 +25,10 @@ func TestDialerResolverNoPort(t *testing.T) {
 }
 
 func TestDialerResolverLookupHostAddress(t *testing.T) {
-	dialer := &DialerResolver{Dialer: new(net.Dialer), Resolver: MockableResolver{
+	dialer := &DialerResolver{Dialer: new(net.Dialer), Resolver: &netxmocks.Resolver{
-		Err: errors.New("mocked error"),
+		MockLookupHost: func(ctx context.Context, domain string) ([]string, error) {
+			return nil, errors.New("we should not call this function")
+		},
 	}}
 	addrs, err := dialer.lookupHost(context.Background(), "1.1.1.1")
 	if err != nil {
@@ -39,35 +41,21 @@ func TestDialerResolverLookupHostAddress(t *testing.T) {
 
 func TestDialerResolverLookupHostFailure(t *testing.T) {
 	expected := errors.New("mocked error")
-	dialer := &DialerResolver{Dialer: new(net.Dialer), Resolver: MockableResolver{
+	dialer := &DialerResolver{Dialer: new(net.Dialer), Resolver: &netxmocks.Resolver{
-		Err: expected,
+		MockLookupHost: func(ctx context.Context, domain string) ([]string, error) {
+			return nil, expected
+		},
 	}}
-	conn, err := dialer.DialContext(context.Background(), "tcp", "dns.google.com:853")
+	ctx := context.Background()
+	conn, err := dialer.DialContext(ctx, "tcp", "dns.google.com:853")
 	if !errors.Is(err, expected) {
-		t.Fatal("not the error we expected")
+		t.Fatal("not the error we expected", err)
 	}
 	if conn != nil {
 		t.Fatal("expected nil conn")
 	}
 }
 
-type MockableResolver struct {
-	Addresses []string
-	Err       error
-}
-
-func (r MockableResolver) LookupHost(ctx context.Context, host string) ([]string, error) {
-	return r.Addresses, r.Err
-}
-
-func (r MockableResolver) Network() string {
-	return "mockable"
-}
-
-func (r MockableResolver) Address() string {
-	return ""
-}
-
 func TestDialerResolverDialForSingleIPFails(t *testing.T) {
 	dialer := &DialerResolver{Dialer: &netxmocks.Dialer{
 		MockDialContext: func(ctx context.Context, network string, address string) (net.Conn, error) {
@@ -89,8 +77,10 @@ func TestDialerResolverDialForManyIPFails(t *testing.T) {
 			MockDialContext: func(ctx context.Context, network string, address string) (net.Conn, error) {
 				return nil, io.EOF
 			},
-		}, Resolver: MockableResolver{
+		}, Resolver: &netxmocks.Resolver{
-			Addresses: []string{"1.1.1.1", "8.8.8.8"},
+			MockLookupHost: func(ctx context.Context, domain string) ([]string, error) {
+				return []string{"1.1.1.1", "8.8.8.8"}, nil
+			},
 		}}
 	conn, err := dialer.DialContext(context.Background(), "tcp", "dot.dns:853")
 	if !errors.Is(err, io.EOF) {
@@ -110,8 +100,10 @@ func TestDialerResolverDialForManyIPSuccess(t *testing.T) {
 				},
 			}, nil
 		},
-	}, Resolver: MockableResolver{
+	}, Resolver: &netxmocks.Resolver{
-		Addresses: []string{"1.1.1.1", "8.8.8.8"},
+		MockLookupHost: func(ctx context.Context, domain string) ([]string, error) {
+			return []string{"1.1.1.1", "8.8.8.8"}, nil
+		},
 	}}
 	conn, err := dialer.DialContext(context.Background(), "tcp", "dot.dns:853")
 	if err != nil {
@@ -123,6 +115,29 @@ func TestDialerResolverDialForManyIPSuccess(t *testing.T) {
 	conn.Close()
 }
 
+func TestDialerLoggerSuccess(t *testing.T) {
+	d := &DialerLogger{
+		Dialer: &netxmocks.Dialer{
+			MockDialContext: func(ctx context.Context, network string, address string) (net.Conn, error) {
+				return &netxmocks.Conn{
+					MockClose: func() error {
+						return nil
+					},
+				}, nil
+			},
+		},
+		Logger: log.Log,
+	}
+	conn, err := d.DialContext(context.Background(), "tcp", "www.google.com:443")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if conn == nil {
+		t.Fatal("expected non-nil conn here")
+	}
+	conn.Close()
+}
+
 func TestDialerLoggerFailure(t *testing.T) {
 	d := &DialerLogger{
 		Dialer: &netxmocks.Dialer{
@@ -141,18 +156,6 @@ func TestDialerLoggerFailure(t *testing.T) {
 	}
 }
 
-func TestDefaultDialerWorks(t *testing.T) {
-	ctx, cancel := context.WithCancel(context.Background())
-	cancel() // fail immediately
-	conn, err := DefaultDialer.DialContext(ctx, "tcp", "8.8.8.8:853")
-	if err == nil || !strings.HasSuffix(err.Error(), "operation was canceled") {
-		t.Fatal("not the error we expected", err)
-	}
-	if conn != nil {
-		t.Fatal("expected nil conn here")
-	}
-}
-
 func TestDefaultDialerHasTimeout(t *testing.T) {
 	expected := 15 * time.Second
 	if DefaultDialer.Timeout != expected {

internal/netxlite/legacy_test.go

@@ -14,7 +14,6 @@ func TestReduceErrors(t *testing.T) {
 			t.Fatal("wrong result")
 		}
 	})
-
 	t.Run("single error", func(t *testing.T) {
 		err := errors.New("mocked error")
 		result := ReduceErrors([]error{err})
@@ -22,7 +21,6 @@ func TestReduceErrors(t *testing.T) {
 			t.Fatal("wrong result")
 		}
 	})
-
 	t.Run("multiple errors", func(t *testing.T) {
 		err1 := errors.New("mocked error #1")
 		err2 := errors.New("mocked error #2")
@@ -31,7 +29,6 @@ func TestReduceErrors(t *testing.T) {
 			t.Fatal("wrong result")
 		}
 	})
-
 	t.Run("multiple errors with meaningful ones", func(t *testing.T) {
 		err1 := errors.New("mocked error #1")
 		err2 := &errorx.ErrWrapper{

internal/netxlite/resolver.go

@@ -48,9 +48,13 @@ func (r ResolverLogger) LookupHost(ctx context.Context, hostname string) ([]stri
 	r.Logger.Debugf("resolve %s...", hostname)
 	start := time.Now()
 	addrs, err := r.Resolver.LookupHost(ctx, hostname)
-	stop := time.Now()
+	elapsed := time.Since(start)
-	r.Logger.Debugf("resolve %s... (%+v, %+v) in %s", hostname, addrs, err, stop.Sub(start))
+	if err != nil {
-	return addrs, err
+		r.Logger.Debugf("resolve %s... %s in %s", hostname, err, elapsed)
+		return nil, err
+	}
+	r.Logger.Debugf("resolve %s... %+v in %s", hostname, addrs, elapsed)
+	return addrs, nil
 }
 
 type resolverNetworker interface {

internal/netxlite/resolver_test.go

@@ -2,13 +2,16 @@ package netxlite
 
 import (
 	"context"
+	"errors"
 	"net"
 	"testing"
 
 	"github.com/apex/log"
+	"github.com/google/go-cmp/cmp"
+	"github.com/ooni/probe-cli/v3/internal/netxmocks"
 )
 
-func TestResolverSystemLookupHost(t *testing.T) {
+func TestResolverSystemNetworkAddress(t *testing.T) {
 	r := ResolverSystem{}
 	if r.Network() != "system" {
 		t.Fatal("invalid Network")
@@ -16,6 +19,10 @@ func TestResolverSystemLookupHost(t *testing.T) {
 	if r.Address() != "" {
 		t.Fatal("invalid Address")
 	}
+}
+
+func TestResolverSystemWorksAsIntended(t *testing.T) {
+	r := ResolverSystem{}
 	addrs, err := r.LookupHost(context.Background(), "dns.google.com")
 	if err != nil {
 		t.Fatal(err)
@@ -25,27 +32,55 @@ func TestResolverSystemLookupHost(t *testing.T) {
 	}
 }
 
-func TestResolverLoggerWithFailure(t *testing.T) {
+func TestResolverLoggerWithSuccess(t *testing.T) {
+	expected := []string{"1.1.1.1"}
 	r := ResolverLogger{
-		Logger:   log.Log,
+		Logger: log.Log,
-		Resolver: DefaultResolver,
+		Resolver: &netxmocks.Resolver{
+			MockLookupHost: func(ctx context.Context, domain string) ([]string, error) {
+				return expected, nil
+			},
+		},
 	}
-	if r.Network() != "system" {
+	addrs, err := r.LookupHost(context.Background(), "dns.google")
-		t.Fatal("invalid Network")
+	if err != nil {
+		t.Fatal(err)
 	}
-	if r.Address() != "" {
+	if diff := cmp.Diff(expected, addrs); diff != "" {
-		t.Fatal("invalid Address")
+		t.Fatal(diff)
 	}
-	addrs, err := r.LookupHost(context.Background(), "nonexistent.antani")
+}
-	if err == nil {
+
-		t.Fatal("expected an error here")
+func TestResolverLoggerWithFailure(t *testing.T) {
+	expected := errors.New("mocked error")
+	r := ResolverLogger{
+		Logger: log.Log,
+		Resolver: &netxmocks.Resolver{
+			MockLookupHost: func(ctx context.Context, domain string) ([]string, error) {
+				return nil, expected
+			},
+		},
+	}
+	addrs, err := r.LookupHost(context.Background(), "dns.google")
+	if !errors.Is(err, expected) {
+		t.Fatal("not the error we expected", err)
 	}
 	if addrs != nil {
 		t.Fatal("expected nil addr here")
 	}
 }
 
-func TestResolverLoggerDefaultNetworkAddress(t *testing.T) {
+func TestResolverLoggerChildNetworkAddress(t *testing.T) {
+	r := &ResolverLogger{Logger: log.Log, Resolver: DefaultResolver}
+	if r.Network() != "system" {
+		t.Fatal("invalid Network")
+	}
+	if r.Address() != "" {
+		t.Fatal("invalid Address")
+	}
+}
+
+func TestResolverLoggerNoChildNetworkAddress(t *testing.T) {
 	r := &ResolverLogger{Logger: log.Log, Resolver: &net.Resolver{}}
 	if r.Network() != "logger" {
 		t.Fatal("invalid Network")

internal/netxmocks/conn.go

@@ -17,42 +17,42 @@ type Conn struct {
 	MockSetWriteDeadline func(t time.Time) error
 }
 
-// Read implements net.Conn.Read
+// Read calls MockRead.
 func (c *Conn) Read(b []byte) (int, error) {
 	return c.MockRead(b)
 }
 
-// Write implements net.Conn.Write
+// Write calls MockWrite.
 func (c *Conn) Write(b []byte) (int, error) {
 	return c.MockWrite(b)
 }
 
-// Close implements net.Conn.Close
+// Close calls MockClose.
 func (c *Conn) Close() error {
 	return c.MockClose()
 }
 
-// LocalAddr returns the local address
+// LocalAddr class MockLocalAddr.
 func (c *Conn) LocalAddr() net.Addr {
 	return c.MockLocalAddr()
 }
 
-// RemoteAddr returns the remote address
+// RemoteAddr calls MockRemoteAddr.
 func (c *Conn) RemoteAddr() net.Addr {
 	return c.MockRemoteAddr()
 }
 
-// SetDeadline sets the connection deadline.
+// SetDeadline calls MockSetDeadline.
 func (c *Conn) SetDeadline(t time.Time) error {
 	return c.MockSetDeadline(t)
 }
 
-// SetReadDeadline sets the read deadline.
+// SetReadDeadline calls MockSetReadDeadline.
 func (c *Conn) SetReadDeadline(t time.Time) error {
 	return c.MockSetReadDeadline(t)
 }
 
-// SetWriteDeadline sets the write deadline.
+// SetWriteDeadline calls MockSetWriteDeadline.
 func (c *Conn) SetWriteDeadline(t time.Time) error {
 	return c.MockSetWriteDeadline(t)
 }

internal/netxmocks/dialer.go

@@ -15,7 +15,7 @@ type Dialer struct {
 	MockDialContext func(ctx context.Context, network, address string) (net.Conn, error)
 }
 
-// DialContext implements Dialer.DialContext.
+// DialContext calls MockDialContext.
 func (d *Dialer) DialContext(ctx context.Context, network, address string) (net.Conn, error) {
 	return d.MockDialContext(ctx, network, address)
 }

internal/netxmocks/resolver.go

@@ -16,17 +16,17 @@ type Resolver struct {
 	MockAddress    func() string
 }
 
-// LookupHost implements Resolver.LookupHost.
+// LookupHost calls MockLookupHost.
 func (r *Resolver) LookupHost(ctx context.Context, domain string) ([]string, error) {
 	return r.MockLookupHost(ctx, domain)
 }
 
-// Address implements Resolver.Address.
+// Address calls MockAddress.
 func (r *Resolver) Address() string {
 	return r.MockAddress()
 }
 
-// Network implements Resolver.Network.
+// Network calls MockNetwork.
 func (r *Resolver) Network() string {
 	return r.MockNetwork()
 }