feat(webconnectivityalgo): test OpportunisticDNSOverHTTPSURLProvider (#…

…1497)

Part of ooni/probe#2669

internal/webconnectivityalgo/dnsoverhttps.go

@@ -12,15 +12,17 @@ import (
 	"time"
 )
 
-// TODO(bassosimone): consider whether factoring out this code
-// and storing the state on disk instead of using memory
-
 // TODO(bassosimone): consider unifying somehow this code and
 // the systemresolver code (or maybe just the list of resolvers)
 
 // OpportunisticDNSOverHTTPSURLProvider allows to perform opportunistic DNS-over-HTTPS
 // measurements as part of Web Connectivity LTE. The zero value of this struct is not valid,
 // please use [NewOpportunisticDNSOverHTTPSURLProvider] to construct.
+//
+// Implementation note: this code uses memory to keep track of the resolvers and know
+// when to perform the next opportunistic check. It seems pointless to use the disk since
+// invocations of Web Connectivity typically consist of multiple URLs and therefore run
+// for a few minutes. Hence, storing state on disk seems a bit overkill here.
 type OpportunisticDNSOverHTTPSURLProvider struct {
 	// interval is the next interval after which to measure.
 	interval time.Duration
@@ -34,34 +36,58 @@ type OpportunisticDNSOverHTTPSURLProvider struct {
 	// t is when we last run an opportunistic measurement.
 	t time.Time
 
+	// timeNow is the function to get the current time.
+	timeNow func() time.Time
+
 	// urls contains the urls of known DoH services.
 	urls []string
 }
 
 // NewOpportunisticDNSOverHTTPSURLProvider creates a new [*OpportunisticDNSOverHTTPSURLProvider].
 func NewOpportunisticDNSOverHTTPSURLProvider(urls ...string) *OpportunisticDNSOverHTTPSURLProvider {
-	return &OpportunisticDNSOverHTTPSURLProvider{
+	o := &OpportunisticDNSOverHTTPSURLProvider{
 		interval: 0,
 		mu:       &sync.Mutex{},
-		rnd:      rand.New(rand.NewSource(time.Now().UnixNano())),
+		rnd:      nil, // configured below
 		t:        time.Time{},
+		timeNow:  time.Now,
 		urls:     urls,
 	}
+	o.seed(o.timeNow()) // allow unit tests to reconfigure the seed we use
+	return o
+}
+
+func (o *OpportunisticDNSOverHTTPSURLProvider) seed(t time.Time) {
+	o.rnd = rand.New(rand.NewSource(t.UnixNano()))
 }
 
 // MaybeNextURL returns the next URL to measure, if any. Our aim is to perform
 // periodic, opportunistic DoH measurements as part of Web Connectivity.
 func (o *OpportunisticDNSOverHTTPSURLProvider) MaybeNextURL() (string, bool) {
-	now := time.Now()
+	// obtain the current time
+	now := o.timeNow()
+
+	// make sure there's mutual exclusion
 	o.mu.Lock()
 	defer o.mu.Unlock()
-	if o.t.IsZero() || now.Sub(o.t) > o.interval {
+
+	// Make sure we run periodically but now always, since there is no point in
+	// always using DNS-over-HTTPS rather the aim is to opportunistically try using
+	// it so to collect data on whether it's actually WAI.
+	if len(o.urls) > 0 && (o.t.IsZero() || now.Sub(o.t) > o.interval) {
+
+		// shuffle the list according to the selected random profile
 		o.rnd.Shuffle(len(o.urls), func(i, j int) {
 			o.urls[i], o.urls[j] = o.urls[j], o.urls[i]
 		})
+
+		// register the current invocation and remember to run again later
 		o.t = now
 		o.interval = time.Duration(20+o.rnd.Uint32()%20) * time.Second
+
+		// return the selected URL to the caller
 		return o.urls[0], true
 	}
+
 	return "", false
 }

internal/webconnectivityalgo/dnsoverhttps_test.go

@@ -0,0 +1,150 @@
+package webconnectivityalgo
+
+import (
+	"testing"
+	"time"
+
+	"github.com/google/go-cmp/cmp"
+	"github.com/ooni/probe-cli/v3/internal/runtimex"
+)
+
+type testTimeProvider struct {
+	t0    time.Time
+	times []time.Duration
+	idx   int
+}
+
+func (ttp *testTimeProvider) timeNow() time.Time {
+	runtimex.Assert(ttp.idx < len(ttp.times), "out of bounds")
+	mockedTime := ttp.t0.Add(ttp.times[ttp.idx])
+	ttp.idx++
+	return mockedTime
+}
+
+func TestOpportunisticDNSOverHTTPSURLProvider(t *testing.T) {
+
+	// expectation is an expectation of a test case.
+	type expectation struct {
+		URL  string
+		Good bool
+	}
+
+	// testcase is a test case implemented by this testing function.
+	type testcase struct {
+		// name is the test case name.
+		name string
+
+		// timeNow is the function to obtain time. In case it is zero, we're
+		// not goint to reconfigure the time fetching function.
+		timeNow func() time.Time
+
+		// seed is the random seed or zero. In case it is zero, we're not
+		// going to reconfigure the random see we use.
+		seed time.Time
+
+		// urls contains the URLs to use.
+		urls []string
+
+		// expect contains the expectations.
+		expect []expectation
+	}
+
+	// cases contains test cases.
+	cases := []testcase{{
+		name:    "without any URL",
+		timeNow: nil,
+		seed:    time.Time{},
+		urls:    []string{},
+		expect: []expectation{{
+			URL:  "",
+			Good: false,
+		}, {
+			URL:  "",
+			Good: false,
+		}, {
+			URL:  "",
+			Good: false,
+		}},
+	}, {
+		name: "with a single URL we get it and then need to wait",
+		timeNow: (&testTimeProvider{
+			t0: time.Date(2024, 2, 8, 9, 8, 7, 6, time.UTC),
+			times: []time.Duration{
+				0,               // should return URL
+				1 * time.Second, // too early to get another URL
+				5 * time.Second, // ditto
+			},
+			idx: 0,
+		}).timeNow,
+		seed: time.Date(2024, 2, 8, 9, 8, 7, 6, time.UTC),
+		urls: []string{
+			"https://dns.google/dns-query",
+		},
+		expect: []expectation{{
+			URL:  "https://dns.google/dns-query",
+			Good: true,
+		}, {
+			URL:  "",
+			Good: false,
+		}, {
+			URL:  "",
+			Good: false,
+		}},
+	}, {
+		name: "with multiple URLs and long wait times we have shuffling",
+		timeNow: (&testTimeProvider{
+			t0: time.Date(2024, 2, 8, 9, 8, 7, 6, time.UTC),
+			times: []time.Duration{
+				0,                 // should return URL
+				60 * time.Minute,  // ditto
+				120 * time.Minute, // ditto
+			},
+			idx: 0,
+		}).timeNow,
+		seed: time.Date(2024, 2, 8, 9, 8, 7, 6, time.UTC),
+		urls: []string{
+			"https://dns.google/dns-query",
+			"https://cloudflare-dns.com/dns-query",
+		},
+		expect: []expectation{{
+			URL:  "https://cloudflare-dns.com/dns-query",
+			Good: true,
+		}, {
+			URL:  "https://dns.google/dns-query",
+			Good: true,
+		}, {
+			URL:  "https://dns.google/dns-query",
+			Good: true,
+		}},
+	}}
+
+	// run test cases
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			oup := NewOpportunisticDNSOverHTTPSURLProvider(tc.urls...)
+
+			// note: we need to reconfigure timeNow before resetting the seed
+			if tc.timeNow != nil {
+				oup.timeNow = tc.timeNow
+			}
+			if !tc.seed.IsZero() {
+				oup.seed(tc.seed)
+			} else {
+				oup.seed(oup.timeNow())
+			}
+
+			var got []expectation
+			for len(got) < len(tc.expect) {
+				url, good := oup.MaybeNextURL()
+				got = append(got, expectation{
+					URL:  url,
+					Good: good,
+				})
+			}
+
+			if diff := cmp.Diff(tc.expect, got); diff != "" {
+				t.Fatal(diff)
+			}
+		})
+	}
+}