refactor: go test -short does not use the host network (#1352)

This diff refactors the codebase so that we avoid using the host network
when running `go test -short`. This change is good in general, because
now coverage tells us the amount of code we're covering without
depending on interactions with an existing network, which means these
tests behave in the same way in ~any place.

I expect a coverage drop from this PR, because there's some coverage
made with integration testing (if we consider integration tests the
tests that require the host network interface with uncensored internet
access).

To make this happen, I needed to modify the `quictesting` package (now
moved to toplevel and renamed `testingquic`) such that it attempts to
get a known-to-work-well endpoint for QUIC _only_ when the developer
using the package really needs it, rather than on import. Before doing
this, there were several tests that panicked because `quictesting` could
not figure out which IP address to use when you disable the WiFi or run
inside another netns. Now we only figure this IP address out the first
time a test requires us to give it either the domain or the endpoint
that we should use.

To be sure we continue to honour the promise that `go test -short` does
not use the host network, I needed to refactor the CI such that we
measure coverage inside a new network namespace with only localhost
support. I think this compromise is acceptable, since the original ask
was to avoid flaky network tests (see
ooni/probe#2426).

Because of this change in how we run the coverage checks, I am
tentatively enabling also running all tests for pull requests, otherwise
we don't know if a contribution breaks tests using the network.
Hopefully, we should be fine because we are caching previous runs, so a
bunch of tests should already be cached.

Closes ooni/probe#2426.

While there, enable again some backend integration tests, and close
ooni/probe#2539.

.github/workflows/alltests.yml

@@ -1,6 +1,7 @@
 # Runs the whole test suite
 name: alltests
 on:
+  pull_request:
   push:
     branches:
       - "release/**"

.github/workflows/coverage.yml

@@ -24,7 +24,7 @@ jobs:
           go-version: "${{ steps.goversion.outputs.version }}"
           cache-key-suffix: "-coverage-${{ steps.goversion.outputs.version }}"
 
-      - run: go test -short -race -tags shaping -coverprofile=probe-cli.cov ./...
+      - run: ./script/linuxcoverage.bash
 
       - uses: shogo82148/actions-goveralls@v1
         with:

CONTRIBUTING.md

@@ -36,13 +36,26 @@ functionality should pass existing tests. What's more, any new pull
 request that modifies existing functionality should not decrease the
 existing code coverage.
 
-Long-running tests should be skipped when running tests in short mode
-using `go test -short`. We prefer internal testing to external
-testing. We generally have a file called `foo_test.go` with tests
-for every `foo.go` file. Sometimes we separate long running
-integration tests in a `foo_integration_test.go` file. We also
-sometimes have `foo_internal_test.go` when the main body of tests
-for `foo`, i.e., `foo_test.go` uses external testing.
+New code should have full coverage using either localhost or the
+[internal/netemx](./internal/netemx/) package. Try to cover all the
+error paths as well as the important properties of the code you've written
+that you would like to be sure about.
+
+Additional integration tests using the host network are good,
+but they MUST use this pattern:
+
+```Go
+func TestUsingHostNetwork(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skip test in short mode")
+	}
+}
+```
+
+The overall objective here is for `go test -short` to only use localhost
+and [internal/netemx](./internal/netemx/) such that tests are always
+reproducible. Tests using the host network are there to give us extra
+confidence that everything is working as intended.
 
 If there is a top-level DESIGN.md document, make sure such document is
 kept in sync with code changes you have applied.

internal/cmd/oohelperd/main_test.go

@@ -21,6 +21,10 @@ type (
 )
 
 func TestMainRunServerWorkingAsIntended(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skip test in short mode")
+	}
+
 	// let the kernel pick a random free port
 	*apiEndpoint = "127.0.0.1:0"
 

internal/database/actions_test.go

@@ -367,6 +367,10 @@ func TestPerformanceTestKeys(t *testing.T) {
 }
 
 func TestGetMeasurementJSON(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skip test in short mode")
+	}
+
 	tmpfile, err := ioutil.TempFile("", "dbtest")
 	if err != nil {
 		t.Fatal(err)

internal/engine/experiment_integration_test.go

@@ -130,7 +130,6 @@ func TestRunTelegram(t *testing.T) {
 }
 
 func TestRunTor(t *testing.T) {
-	t.Skip("https://github.com/ooni/probe/issues/2539")
 	if testing.Short() {
 		t.Skip("skip test in short mode")
 	}

internal/engine/session_integration_test.go

@@ -492,6 +492,10 @@ func TestNewOrchestraClientProbeServicesNewClientFailure(t *testing.T) {
 }
 
 func TestSessionNewSubmitterReturnsNonNilSubmitter(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skip test in short mode")
+	}
+
 	sess := newSessionForTesting(t)
 	subm, err := sess.NewSubmitter(context.Background())
 	if err != nil {

internal/engine/session_internal_test.go

@@ -225,6 +225,10 @@ func TestNewProbeServicesClientForCheckIn(t *testing.T) {
 }
 
 func TestSessionNewSubmitterWithCancelledContext(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skip test in short mode")
+	}
+
 	sess := newSessionForTesting(t)
 	ctx, cancel := context.WithCancel(context.Background())
 	cancel() // fail immediately

internal/enginelocate/cloudflare_test.go

@@ -12,6 +12,10 @@ import (
 )
 
 func TestIPLookupWorksUsingcloudlflare(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skip test in short mode")
+	}
+
 	ip, err := cloudflareIPLookup(
 		context.Background(),
 		http.DefaultClient,

internal/enginelocate/geolocate_test.go

@@ -266,6 +266,10 @@ func TestLocationLookupSuccessWithResolverLookup(t *testing.T) {
 }
 
 func TestSmoke(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skip test in short mode")
+	}
+
 	config := Config{}
 	task := NewTask(config)
 	result, err := task.Run(context.Background())

internal/enginelocate/iplookup_test.go

@@ -13,6 +13,10 @@ import (
 )
 
 func TestIPLookupGood(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skip test in short mode")
+	}
+
 	ip, err := (ipLookupClient{
 		Logger:    log.Log,
 		Resolver:  netxlite.NewStdlibResolver(model.DiscardLogger),

internal/enginelocate/resolverlookup_test.go

@@ -9,6 +9,10 @@ import (
 )
 
 func TestLookupResolverIPSuccess(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skip test in short mode")
+	}
+
 	rlc := resolverLookupClient{
 		Logger: model.DiscardLogger,
 	}

internal/enginelocate/stun_test.go

@@ -147,6 +147,10 @@ func TestSTUNIPLookupCannotDecodeMessage(t *testing.T) {
 }
 
 func TestIPLookupWorksUsingSTUNEkiga(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skip test in short mode")
+	}
+
 	ip, err := stunEkigaIPLookup(
 		context.Background(),
 		http.DefaultClient,
@@ -163,6 +167,10 @@ func TestIPLookupWorksUsingSTUNEkiga(t *testing.T) {
 }
 
 func TestIPLookupWorksUsingSTUNGoogle(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skip test in short mode")
+	}
+
 	ip, err := stunGoogleIPLookup(
 		context.Background(),
 		http.DefaultClient,

internal/enginelocate/ubuntu_test.go

@@ -35,6 +35,10 @@ func TestUbuntuParseError(t *testing.T) {
 }
 
 func TestIPLookupWorksUsingUbuntu(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skip test in short mode")
+	}
+
 	ip, err := ubuntuIPLookup(
 		context.Background(),
 		http.DefaultClient,

internal/experiment/dnscheck/dnscheck_test.go

@@ -143,6 +143,10 @@ func TestMakeResolverURL(t *testing.T) {
 }
 
 func TestDNSCheckValid(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skip test in short mode")
+	}
+
 	measurer := NewExperimentMeasurer(Config{
 		DefaultAddrs: "1.1.1.1 1.0.0.1",
 	})
@@ -189,6 +193,10 @@ func TestSummaryKeysGeneric(t *testing.T) {
 }
 
 func TestDNSCheckWait(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skip test in short mode")
+	}
+
 	endpoints := &Endpoints{
 		WaitTime: 1 * time.Second,
 	}

internal/experiment/echcheck/measure_test.go

@@ -67,6 +67,10 @@ func TestMeasurerMeasureWithInvalidInput2(t *testing.T) {
 }
 
 func TestMeasurementSuccess(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skip test in short mode")
+	}
+
 	sess := &mockable.Session{MockableLogger: log.Log}
 	callbacks := model.NewPrinterCallbacks(sess.Logger())
 	measurer := NewExperimentMeasurer(Config{})

internal/experiment/hhfm/hhfm_test.go

@@ -32,6 +32,10 @@ func TestNewExperimentMeasurer(t *testing.T) {
 }
 
 func TestSuccess(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skip test in short mode")
+	}
+
 	measurer := hhfm.NewExperimentMeasurer(hhfm.Config{})
 	ctx := context.Background()
 	sess := &mockable.Session{

internal/experiment/ndt7/dial_test.go

@@ -14,6 +14,10 @@ import (
 )
 
 func TestDialDownloadWithCancelledContext(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skip test in short mode")
+	}
+
 	ctx, cancel := context.WithCancel(context.Background())
 	cancel() // immediately halt
 	mgr := newDialManager("wss://hostname.fake", log.Log, "miniooni/0.1.0-dev")
@@ -27,6 +31,10 @@ func TestDialDownloadWithCancelledContext(t *testing.T) {
 }
 
 func TestDialUploadWithCancelledContext(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skip test in short mode")
+	}
+
 	ctx, cancel := context.WithCancel(context.Background())
 	cancel() // immediately halt
 	mgr := newDialManager("wss://hostname.fake", log.Log, "miniooni/0.1.0-dev")

internal/experiment/ndt7/ndt7_test.go

@@ -131,6 +131,10 @@ func TestGood(t *testing.T) {
 }
 
 func TestFailDownload(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skip test in short mode")
+	}
+
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 	measurer := NewExperimentMeasurer(Config{}).(*Measurer)
@@ -162,6 +166,10 @@ func TestFailDownload(t *testing.T) {
 }
 
 func TestFailUpload(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skip test in short mode")
+	}
+
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 	measurer := NewExperimentMeasurer(Config{noDownload: true}).(*Measurer)

internal/experiment/quicping/quicping_test.go

@@ -123,6 +123,10 @@ func TestSuccess(t *testing.T) {
 }
 
 func TestWithCancelledContext(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skip test in short mode")
+	}
+
 	measurer := NewExperimentMeasurer(Config{})
 	measurement := new(model.Measurement)
 	measurement.Input = model.MeasurementTarget("google.com")
@@ -145,6 +149,10 @@ func TestWithCancelledContext(t *testing.T) {
 }
 
 func TestListenFails(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skip test in short mode")
+	}
+
 	expected := errors.New("expected")
 	measurer := NewExperimentMeasurer(Config{
 		netListenUDP: func(network string, laddr *net.UDPAddr) (model.UDPLikeConn, error) {

internal/experiment/signal/signal_test.go

@@ -23,6 +23,10 @@ func TestNewExperimentMeasurer(t *testing.T) {
 }
 
 func TestGood(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skip test in short mode")
+	}
+
 	measurer := signal.NewExperimentMeasurer(signal.Config{})
 	measurement := new(model.Measurement)
 	args := &model.ExperimentArgs{

internal/experiment/stunreachability/stunreachability_test.go

@@ -89,6 +89,10 @@ func TestRunWithUnsupportedURLScheme(t *testing.T) {
 }
 
 func TestRunWithInput(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skip test in short mode")
+	}
+
 	measurer := NewExperimentMeasurer(Config{})
 	measurement := new(model.Measurement)
 	measurement.Input = model.MeasurementTarget(defaultInput)
@@ -158,6 +162,10 @@ func TestCancelledContext(t *testing.T) {
 }
 
 func TestNewClientFailure(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skip test in short mode")
+	}
+
 	config := &Config{}
 	expected := errors.New("mocked error")
 	config.newClient = func(conn stun.Connection, options ...stun.ClientOption) (*stun.Client, error) {

internal/experiment/tlstool/internal/internal_test.go

@@ -25,17 +25,33 @@ func dial(t *testing.T, d model.Dialer) {
 }
 
 func TestNewSNISplitterDialer(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skip test in short mode")
+	}
+
 	dial(t, internal.NewSNISplitterDialer(config))
 }
 
 func TestNewThriceSplitterDialer(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skip test in short mode")
+	}
+
 	dial(t, internal.NewThriceSplitterDialer(config))
 }
 
 func TestNewRandomSplitterDialer(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skip test in short mode")
+	}
+
 	dial(t, internal.NewRandomSplitterDialer(config))
 }
 
 func TestNewVanillaDialer(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skip test in short mode")
+	}
+
 	dial(t, internal.NewVanillaDialer(config))
 }

internal/experiment/urlgetter/getter_integration_test.go

@@ -386,6 +386,10 @@ func TestGetterWithCancelledContextUnknownResolverURL(t *testing.T) {
 }
 
 func TestGetterIntegrationHTTPS(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skip test in short mode")
+	}
+
 	ctx := context.Background()
 	g := urlgetter.Getter{
 		Config: urlgetter.Config{
@@ -510,6 +514,10 @@ func TestGetterIntegrationRedirect(t *testing.T) {
 }
 
 func TestGetterIntegrationTLSHandshake(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skip test in short mode")
+	}
+
 	ctx := context.Background()
 	g := urlgetter.Getter{
 		Config: urlgetter.Config{
@@ -611,6 +619,10 @@ func TestGetterIntegrationTLSHandshake(t *testing.T) {
 }
 
 func TestGetterHTTPSWithTunnel(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skip test in short mode")
+	}
+
 	// quick enough (0.4s) to run with every run
 	ctx := context.Background()
 	g := urlgetter.Getter{