refactor: go test -short does not use the host network

.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{