From acef18a955a58563fa7ca9979f14b8df766219dd Mon Sep 17 00:00:00 2001
From: Simone Basso <bassosimone@gmail.com>
Date: Fri, 25 Jun 2021 11:51:10 +0200
Subject: [PATCH] fix(netx): repair BogonResolver tests (#401)

The BogonResolver relied on its wrapper resolver to pass along the
list of addresses _and_ the error. But the idiomatic thing to do is
often to return `nil` when there is an error.

I broke this very fragile assumption in https://github.com/ooni/probe-cli/pull/399.

I could of course fix it, but this assumption is clearly wrong
and we should not allow such fragile code in the tree.

We are not using BogonIsError much in the tree. The only place in
which we're using it for measuring seems to be dnscheck.

It may be that this surprising behavior was what caused the issue at
https://github.com/ooni/probe/issues/1510 in the first place.

Regardless, let's remove fragile code and adjust the test that was
failing. Also that test is quick so it can run in `-short` mode.

Spotted while working on https://github.com/ooni/probe/issues/1505.
---
 internal/engine/netx/integration_test.go    |  7 ++-----
 internal/engine/netx/resolver/bogon.go      | 11 +++++++----
 internal/engine/netx/resolver/bogon_test.go |  4 ++--
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/internal/engine/netx/integration_test.go b/internal/engine/netx/integration_test.go
index 969a3b0..73fffff 100644
--- a/internal/engine/netx/integration_test.go
+++ b/internal/engine/netx/integration_test.go
@@ -68,9 +68,6 @@ func TestSuccess(t *testing.T) {
 }
 
 func TestBogonResolutionNotBroken(t *testing.T) {
-	if testing.Short() {
-		t.Skip("skip test in short mode")
-	}
 	saver := new(trace.Saver)
 	r := netx.NewResolver(netx.Config{
 		BogonIsError: true,
@@ -87,7 +84,7 @@ func TestBogonResolutionNotBroken(t *testing.T) {
 	if err.Error() != errorx.FailureDNSBogonError {
 		t.Fatal("error not correctly wrapped")
 	}
-	if len(addrs) != 1 || addrs[0] != "127.0.0.1" {
-		t.Fatal("address was not returned")
+	if len(addrs) > 0 {
+		t.Fatal("expected no addresses here")
 	}
 }
diff --git a/internal/engine/netx/resolver/bogon.go b/internal/engine/netx/resolver/bogon.go
index 8103cc3..2ba0591 100644
--- a/internal/engine/netx/resolver/bogon.go
+++ b/internal/engine/netx/resolver/bogon.go
@@ -51,6 +51,11 @@ func IsBogon(address string) bool {
 
 // BogonResolver is a bogon aware resolver. When a bogon is encountered in
 // a reply, this resolver will return an error.
+//
+// Deprecation warning
+//
+// This resolver is deprecated. The right thing to do would be to check
+// for bogons right after a domain name resolution in the nettest.
 type BogonResolver struct {
 	Resolver
 }
@@ -59,10 +64,8 @@ type BogonResolver struct {
 func (r BogonResolver) LookupHost(ctx context.Context, hostname string) ([]string, error) {
 	addrs, err := r.Resolver.LookupHost(ctx, hostname)
 	for _, addr := range addrs {
-		if IsBogon(addr) == true {
-			// We need to return the addrs otherwise the caller cannot see/log/save
-			// the specific addresses that triggered our bogon filter
-			return addrs, errorx.ErrDNSBogon
+		if IsBogon(addr) {
+			return nil, errorx.ErrDNSBogon
 		}
 	}
 	return addrs, err
diff --git a/internal/engine/netx/resolver/bogon_test.go b/internal/engine/netx/resolver/bogon_test.go
index 17aebfe..b12f67d 100644
--- a/internal/engine/netx/resolver/bogon_test.go
+++ b/internal/engine/netx/resolver/bogon_test.go
@@ -32,8 +32,8 @@ func TestBogonAwareResolverWithBogon(t *testing.T) {
 	if !errors.Is(err, errorx.ErrDNSBogon) {
 		t.Fatal("not the error we expected")
 	}
-	if len(addrs) != 1 || addrs[0] != "127.0.0.1" {
-		t.Fatal("expected to see address here")
+	if len(addrs) > 0 {
+		t.Fatal("expected to see nil here")
 	}
 }