From 2edeafa7795653da174f58811f4be1e31fdd9eed Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Mon, 9 Oct 2023 19:49:48 +0200 Subject: [PATCH] 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 https://github.com/ooni/probe/issues/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 https://github.com/ooni/probe/issues/2426. While there, enable again some backend integration tests, and close https://github.com/ooni/probe/issues/2539. --- .github/workflows/alltests.yml | 1 + .github/workflows/coverage.yml | 2 +- CONTRIBUTING.md | 27 ++++++--- internal/cmd/oohelperd/main_test.go | 4 ++ internal/database/actions_test.go | 4 ++ .../engine/experiment_integration_test.go | 1 - internal/engine/session_integration_test.go | 4 ++ internal/engine/session_internal_test.go | 4 ++ internal/enginelocate/cloudflare_test.go | 4 ++ internal/enginelocate/geolocate_test.go | 4 ++ internal/enginelocate/iplookup_test.go | 4 ++ internal/enginelocate/resolverlookup_test.go | 4 ++ internal/enginelocate/stun_test.go | 8 +++ internal/enginelocate/ubuntu_test.go | 4 ++ internal/experiment/dnscheck/dnscheck_test.go | 8 +++ internal/experiment/echcheck/measure_test.go | 4 ++ internal/experiment/hhfm/hhfm_test.go | 4 ++ internal/experiment/ndt7/dial_test.go | 8 +++ internal/experiment/ndt7/ndt7_test.go | 8 +++ internal/experiment/quicping/quicping_test.go | 8 +++ internal/experiment/signal/signal_test.go | 4 ++ .../stunreachability/stunreachability_test.go | 8 +++ .../tlstool/internal/internal_test.go | 16 +++++ .../urlgetter/getter_integration_test.go | 12 ++++ internal/experiment/urlgetter/multi_test.go | 4 ++ internal/measurexlite/quic_test.go | 8 ++- internal/mlablocatev2/mlablocatev2_test.go | 12 +++- internal/netxlite/integration_test.go | 12 ++-- internal/netxlite/quictesting/quictesting.go | 38 ------------ internal/netxlite/utls_test.go | 4 ++ internal/probeservices/checkin_test.go | 4 ++ internal/probeservices/collector_test.go | 12 ++++ internal/probeservices/login_test.go | 5 +- .../probeservices/measurementmeta_test.go | 4 ++ internal/probeservices/probeservices_test.go | 4 ++ internal/probeservices/psiphon_test.go | 5 +- internal/probeservices/register_test.go | 4 ++ internal/probeservices/tor_test.go | 6 +- internal/ptx/fake_test.go | 4 ++ internal/ptx/ptx_test.go | 4 ++ internal/testingquic/testingquic.go | 59 +++++++++++++++++++ .../testingquic_test.go} | 13 ++-- pkg/oonimkall/xoonirun_test.go | 4 ++ script/linuxcoverage.bash | 18 ++++++ script/linuxcoveragerun.bash | 18 ++++++ 45 files changed, 331 insertions(+), 66 deletions(-) delete mode 100644 internal/netxlite/quictesting/quictesting.go create mode 100644 internal/testingquic/testingquic.go rename internal/{netxlite/quictesting/quictesting_test.go => testingquic/testingquic_test.go} (50%) create mode 100755 script/linuxcoverage.bash create mode 100755 script/linuxcoveragerun.bash diff --git a/.github/workflows/alltests.yml b/.github/workflows/alltests.yml index d1d5c42619..6cdddc5181 100644 --- a/.github/workflows/alltests.yml +++ b/.github/workflows/alltests.yml @@ -1,6 +1,7 @@ # Runs the whole test suite name: alltests on: + pull_request: push: branches: - "release/**" diff --git a/.github/workflows/coverage.yml b/.github/workflows/coverage.yml index 91f485aa80..c25824013d 100644 --- a/.github/workflows/coverage.yml +++ b/.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: diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index deb5860278..2fe1d41621 100644 --- a/CONTRIBUTING.md +++ b/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. diff --git a/internal/cmd/oohelperd/main_test.go b/internal/cmd/oohelperd/main_test.go index 335e07bb3b..3a7013212e 100644 --- a/internal/cmd/oohelperd/main_test.go +++ b/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" diff --git a/internal/database/actions_test.go b/internal/database/actions_test.go index 390ecf6d25..8e9ae7ac48 100644 --- a/internal/database/actions_test.go +++ b/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) diff --git a/internal/engine/experiment_integration_test.go b/internal/engine/experiment_integration_test.go index cb83786a3b..865a45203b 100644 --- a/internal/engine/experiment_integration_test.go +++ b/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") } diff --git a/internal/engine/session_integration_test.go b/internal/engine/session_integration_test.go index 8a0b941b61..7bea2abff1 100644 --- a/internal/engine/session_integration_test.go +++ b/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 { diff --git a/internal/engine/session_internal_test.go b/internal/engine/session_internal_test.go index 454ccd5219..7eea180ef2 100644 --- a/internal/engine/session_internal_test.go +++ b/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 diff --git a/internal/enginelocate/cloudflare_test.go b/internal/enginelocate/cloudflare_test.go index c3213c5060..4fc62e5197 100644 --- a/internal/enginelocate/cloudflare_test.go +++ b/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, diff --git a/internal/enginelocate/geolocate_test.go b/internal/enginelocate/geolocate_test.go index 7a69a68fd1..2154736d8b 100644 --- a/internal/enginelocate/geolocate_test.go +++ b/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()) diff --git a/internal/enginelocate/iplookup_test.go b/internal/enginelocate/iplookup_test.go index f8c171c6ce..650c0eefd8 100644 --- a/internal/enginelocate/iplookup_test.go +++ b/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), diff --git a/internal/enginelocate/resolverlookup_test.go b/internal/enginelocate/resolverlookup_test.go index c02110a221..86b9a9e706 100644 --- a/internal/enginelocate/resolverlookup_test.go +++ b/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, } diff --git a/internal/enginelocate/stun_test.go b/internal/enginelocate/stun_test.go index 5efb96207b..d6a0707cd6 100644 --- a/internal/enginelocate/stun_test.go +++ b/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, diff --git a/internal/enginelocate/ubuntu_test.go b/internal/enginelocate/ubuntu_test.go index e385b1b0f2..b2ede1f07e 100644 --- a/internal/enginelocate/ubuntu_test.go +++ b/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, diff --git a/internal/experiment/dnscheck/dnscheck_test.go b/internal/experiment/dnscheck/dnscheck_test.go index f7beba6aee..a7fb9b336b 100644 --- a/internal/experiment/dnscheck/dnscheck_test.go +++ b/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, } diff --git a/internal/experiment/echcheck/measure_test.go b/internal/experiment/echcheck/measure_test.go index f18a17d640..f49d61dc1c 100644 --- a/internal/experiment/echcheck/measure_test.go +++ b/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{}) diff --git a/internal/experiment/hhfm/hhfm_test.go b/internal/experiment/hhfm/hhfm_test.go index d14c29cae0..eb420066d1 100644 --- a/internal/experiment/hhfm/hhfm_test.go +++ b/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{ diff --git a/internal/experiment/ndt7/dial_test.go b/internal/experiment/ndt7/dial_test.go index 8f43b4c4ef..b2b5788a02 100644 --- a/internal/experiment/ndt7/dial_test.go +++ b/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") diff --git a/internal/experiment/ndt7/ndt7_test.go b/internal/experiment/ndt7/ndt7_test.go index 23e3b9b587..371a289570 100644 --- a/internal/experiment/ndt7/ndt7_test.go +++ b/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) diff --git a/internal/experiment/quicping/quicping_test.go b/internal/experiment/quicping/quicping_test.go index e0b28e37e1..c8705e65c0 100644 --- a/internal/experiment/quicping/quicping_test.go +++ b/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) { diff --git a/internal/experiment/signal/signal_test.go b/internal/experiment/signal/signal_test.go index 8dc6aa3805..1b8456bf03 100644 --- a/internal/experiment/signal/signal_test.go +++ b/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{ diff --git a/internal/experiment/stunreachability/stunreachability_test.go b/internal/experiment/stunreachability/stunreachability_test.go index b7b1acffd4..8336584aaa 100644 --- a/internal/experiment/stunreachability/stunreachability_test.go +++ b/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) { diff --git a/internal/experiment/tlstool/internal/internal_test.go b/internal/experiment/tlstool/internal/internal_test.go index 30c694a220..568e0d12b9 100644 --- a/internal/experiment/tlstool/internal/internal_test.go +++ b/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)) } diff --git a/internal/experiment/urlgetter/getter_integration_test.go b/internal/experiment/urlgetter/getter_integration_test.go index 50eb0f7331..9ea2677dbb 100644 --- a/internal/experiment/urlgetter/getter_integration_test.go +++ b/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{ diff --git a/internal/experiment/urlgetter/multi_test.go b/internal/experiment/urlgetter/multi_test.go index 589ba36e22..e4fe27dcf5 100644 --- a/internal/experiment/urlgetter/multi_test.go +++ b/internal/experiment/urlgetter/multi_test.go @@ -17,6 +17,10 @@ import ( ) func TestMultiIntegration(t *testing.T) { + if testing.Short() { + t.Skip("skip test in short mode") + } + multi := urlgetter.Multi{Session: &mockable.Session{}} inputs := []urlgetter.MultiInput{{ Config: urlgetter.Config{Method: "HEAD", NoFollowRedirects: true}, diff --git a/internal/measurexlite/quic_test.go b/internal/measurexlite/quic_test.go index 7a5eba2ca7..e0d1262574 100644 --- a/internal/measurexlite/quic_test.go +++ b/internal/measurexlite/quic_test.go @@ -13,7 +13,7 @@ import ( "github.com/ooni/probe-cli/v3/internal/mocks" "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/netxlite" - "github.com/ooni/probe-cli/v3/internal/netxlite/quictesting" + "github.com/ooni/probe-cli/v3/internal/testingquic" "github.com/ooni/probe-cli/v3/internal/testingx" "github.com/quic-go/quic-go" ) @@ -276,6 +276,10 @@ func TestNewQUICDialerWithoutResolver(t *testing.T) { } func TestOnQUICHandshakeDoneExtractsTheConnectionState(t *testing.T) { + if testing.Short() { + t.Skip("skip test in short mode") + } + // create a trace trace := NewTrace(0, time.Now()) @@ -286,7 +290,7 @@ func TestOnQUICHandshakeDoneExtractsTheConnectionState(t *testing.T) { // dial with the endpoint we use for testing quicConn, err := quicDialer.DialContext( context.Background(), - quictesting.Endpoint("443"), + testingquic.MustEndpoint("443"), &tls.Config{ InsecureSkipVerify: true, }, diff --git a/internal/mlablocatev2/mlablocatev2_test.go b/internal/mlablocatev2/mlablocatev2_test.go index 11e54374a8..1e497d20d6 100644 --- a/internal/mlablocatev2/mlablocatev2_test.go +++ b/internal/mlablocatev2/mlablocatev2_test.go @@ -14,7 +14,9 @@ import ( ) func TestQueryNDT7Success(t *testing.T) { - // this integration test is ~0.5 s, so we can always run it + if testing.Short() { + t.Skip("skip test in short mode") + } client := NewClient(http.DefaultClient, model.DiscardLogger, "miniooni/0.1.0-dev") result, err := client.QueryNDT7(context.Background()) @@ -48,7 +50,9 @@ func TestQueryNDT7Success(t *testing.T) { } func TestQueryDashSuccess(t *testing.T) { - // this integration test is ~0.5 s, so we can always run it + if testing.Short() { + t.Skip("skip test in short mode") + } client := NewClient(http.DefaultClient, model.DiscardLogger, "miniooni/0.1.0-dev") result, err := client.QueryDash(context.Background()) @@ -82,7 +86,9 @@ func TestQueryDashSuccess(t *testing.T) { } func TestQuery404Response(t *testing.T) { - // this integration test is ~0.5 s, so we can always run it + if testing.Short() { + t.Skip("skip test in short mode") + } client := NewClient(http.DefaultClient, model.DiscardLogger, "miniooni/0.1.0-dev") result, err := client.query(context.Background(), "nonexistent") diff --git a/internal/netxlite/integration_test.go b/internal/netxlite/integration_test.go index 7199599b6c..cdf8378351 100644 --- a/internal/netxlite/integration_test.go +++ b/internal/netxlite/integration_test.go @@ -16,9 +16,9 @@ import ( "github.com/apex/log" "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/netxlite" - "github.com/ooni/probe-cli/v3/internal/netxlite/quictesting" "github.com/ooni/probe-cli/v3/internal/randx" "github.com/ooni/probe-cli/v3/internal/runtimex" + "github.com/ooni/probe-cli/v3/internal/testingquic" "github.com/ooni/probe-cli/v3/internal/testingx" "github.com/quic-go/quic-go" utls "gitlab.com/yawning/utls.git" @@ -487,11 +487,11 @@ func TestMeasureWithQUICDialer(t *testing.T) { // why we're using nil to force netxlite to use the cached // default Mozilla cert pool. config := &tls.Config{ - ServerName: quictesting.Domain, + ServerName: testingquic.MustDomain(), NextProtos: []string{"h3"}, RootCAs: nil, } - sess, err := d.DialContext(ctx, quictesting.Endpoint("443"), config, &quic.Config{}) + sess, err := d.DialContext(ctx, testingquic.MustEndpoint("443"), config, &quic.Config{}) if err != nil { t.Fatal(err) } @@ -510,12 +510,12 @@ func TestMeasureWithQUICDialer(t *testing.T) { // why we're using nil to force netxlite to use the cached // default Mozilla cert pool. config := &tls.Config{ - ServerName: quictesting.Domain, + ServerName: testingquic.MustDomain(), NextProtos: []string{"h3"}, RootCAs: nil, } // Here we assume :1 is filtered - sess, err := d.DialContext(ctx, quictesting.Endpoint("1"), config, &quic.Config{}) + sess, err := d.DialContext(ctx, testingquic.MustEndpoint("1"), config, &quic.Config{}) if err == nil || err.Error() != netxlite.FailureGenericTimeoutError { t.Fatal("not the error we expected", err) } @@ -588,7 +588,7 @@ func TestHTTP3Transport(t *testing.T) { ) txp := netxlite.NewHTTP3Transport(log.Log, d, &tls.Config{}) client := &http.Client{Transport: txp} - URL := (&url.URL{Scheme: "https", Host: quictesting.Domain, Path: "/"}).String() + URL := (&url.URL{Scheme: "https", Host: testingquic.MustDomain(), Path: "/"}).String() resp, err := client.Get(URL) if err != nil { t.Fatal(err) diff --git a/internal/netxlite/quictesting/quictesting.go b/internal/netxlite/quictesting/quictesting.go deleted file mode 100644 index a2a7761455..0000000000 --- a/internal/netxlite/quictesting/quictesting.go +++ /dev/null @@ -1,38 +0,0 @@ -// Package quictesting contains code useful to test QUIC. -package quictesting - -import ( - "context" - "net" - "strings" - "time" - - "github.com/ooni/probe-cli/v3/internal/runtimex" -) - -// Domain is the the domain we should be testing using QUIC. -const Domain = "www.cloudflare.com" - -// Address is the address we should be testing using QUIC. -var Address string - -// Endpoint returns the endpoint to test using QUIC by combining -// the Address variable with the given port. -func Endpoint(port string) string { - return net.JoinHostPort(Address, port) -} - -func init() { - const timeout = 10 * time.Second - ctx, cancel := context.WithTimeout(context.Background(), timeout) - defer cancel() - reso := &net.Resolver{} - addrs, err := reso.LookupHost(ctx, Domain) - runtimex.PanicOnError(err, "reso.LookupHost failed") - for _, addr := range addrs { - if !strings.Contains(addr, ":") { - Address = addr - break - } - } -} diff --git a/internal/netxlite/utls_test.go b/internal/netxlite/utls_test.go index 29d7fbd6ec..f771262d89 100644 --- a/internal/netxlite/utls_test.go +++ b/internal/netxlite/utls_test.go @@ -108,6 +108,10 @@ func TestUTLSConn(t *testing.T) { } func Test_newConnUTLSWithHelloID(t *testing.T) { + if testing.Short() { + t.Skip("skip test in short mode") + } + tests := []struct { name string config *tls.Config diff --git a/internal/probeservices/checkin_test.go b/internal/probeservices/checkin_test.go index 00e3c2bac0..b9ea1ace94 100644 --- a/internal/probeservices/checkin_test.go +++ b/internal/probeservices/checkin_test.go @@ -9,6 +9,10 @@ import ( ) func TestCheckInSuccess(t *testing.T) { + if testing.Short() { + t.Skip("skip test in short mode") + } + client := newclient() client.BaseURL = "https://ams-pg-test.ooni.org" config := model.OOAPICheckInConfig{ diff --git a/internal/probeservices/collector_test.go b/internal/probeservices/collector_test.go index c4bd1359fe..f5f9810d0d 100644 --- a/internal/probeservices/collector_test.go +++ b/internal/probeservices/collector_test.go @@ -71,6 +71,10 @@ func TestNewReportTemplate(t *testing.T) { } func TestReportLifecycle(t *testing.T) { + if testing.Short() { + t.Skip("skip test in short mode") + } + ctx := context.Background() template := model.OOAPIReportTemplate{ DataFormatVersion: model.OOAPIReportDefaultDataFormatVersion, @@ -101,6 +105,10 @@ func TestReportLifecycle(t *testing.T) { } func TestReportLifecycleWrongExperiment(t *testing.T) { + if testing.Short() { + t.Skip("skip test in short mode") + } + ctx := context.Background() template := model.OOAPIReportTemplate{ DataFormatVersion: model.OOAPIReportDefaultDataFormatVersion, @@ -355,6 +363,10 @@ func TestOpenReportCancelledContext(t *testing.T) { } func TestSubmitMeasurementCancelledContext(t *testing.T) { + if testing.Short() { + t.Skip("skip test in short mode") + } + ctx, cancel := context.WithCancel(context.Background()) defer cancel() template := model.OOAPIReportTemplate{ diff --git a/internal/probeservices/login_test.go b/internal/probeservices/login_test.go index b051937fe4..a2bf3345fd 100644 --- a/internal/probeservices/login_test.go +++ b/internal/probeservices/login_test.go @@ -54,7 +54,10 @@ func TestMaybeLogin(t *testing.T) { } func TestMaybeLoginIdempotent(t *testing.T) { - t.Skip("TODO(https://github.com/ooni/probe/issues/2539)") + if testing.Short() { + t.Skip("skip test in short mode") + } + clnt := newclient() ctx := context.Background() metadata := MetadataFixture() diff --git a/internal/probeservices/measurementmeta_test.go b/internal/probeservices/measurementmeta_test.go index 5cb44f37e7..63ed736d73 100644 --- a/internal/probeservices/measurementmeta_test.go +++ b/internal/probeservices/measurementmeta_test.go @@ -15,6 +15,10 @@ import ( ) func TestGetMeasurementMetaWorkingAsIntended(t *testing.T) { + if testing.Short() { + t.Skip("skip test in short mode") + } + client := Client{ APIClientTemplate: httpx.APIClientTemplate{ BaseURL: "https://api.ooni.io/", diff --git a/internal/probeservices/probeservices_test.go b/internal/probeservices/probeservices_test.go index 00c24dc741..dcb32667bf 100644 --- a/internal/probeservices/probeservices_test.go +++ b/internal/probeservices/probeservices_test.go @@ -574,6 +574,10 @@ func TestSelectBestSelectsTheFastest(t *testing.T) { } func TestGetCredsAndAuthNotLoggedIn(t *testing.T) { + if testing.Short() { + t.Skip("skip test in short mode") + } + clnt := newclient() if err := clnt.MaybeRegister(context.Background(), MetadataFixture()); err != nil { t.Fatal(err) diff --git a/internal/probeservices/psiphon_test.go b/internal/probeservices/psiphon_test.go index c216808ca2..3bfacd2de2 100644 --- a/internal/probeservices/psiphon_test.go +++ b/internal/probeservices/psiphon_test.go @@ -8,7 +8,10 @@ import ( ) func TestFetchPsiphonConfig(t *testing.T) { - t.Skip("TODO(https://github.com/ooni/probe/issues/2539)") + if testing.Short() { + t.Skip("skip test in short mode") + } + clnt := newclient() if err := clnt.MaybeRegister(context.Background(), MetadataFixture()); err != nil { t.Fatal(err) diff --git a/internal/probeservices/register_test.go b/internal/probeservices/register_test.go index 12059aa9c3..488132b7d9 100644 --- a/internal/probeservices/register_test.go +++ b/internal/probeservices/register_test.go @@ -47,6 +47,10 @@ func TestMaybeRegister(t *testing.T) { } func TestMaybeRegisterIdempotent(t *testing.T) { + if testing.Short() { + t.Skip("skip test in short mode") + } + clnt := newclient() ctx := context.Background() metadata := MetadataFixture() diff --git a/internal/probeservices/tor_test.go b/internal/probeservices/tor_test.go index 1205bd70e4..62f7065143 100644 --- a/internal/probeservices/tor_test.go +++ b/internal/probeservices/tor_test.go @@ -7,7 +7,6 @@ import ( ) func TestFetchTorTargets(t *testing.T) { - t.Skip("TODO(https://github.com/ooni/probe/issues/2539)") if testing.Short() { t.Skip("skip test in short mode") } @@ -60,7 +59,10 @@ func (clnt *FetchTorTargetsHTTPTransport) RoundTrip(req *http.Request) (*http.Re } func TestFetchTorTargetsSetsQueryString(t *testing.T) { - t.Skip("TODO(https://github.com/ooni/probe/issues/2539)") + if testing.Short() { + t.Skip("skip test in short mode") + } + clnt := newclient() txp := new(FetchTorTargetsHTTPTransport) clnt.HTTPClient = &http.Client{Transport: txp} diff --git a/internal/ptx/fake_test.go b/internal/ptx/fake_test.go index d9d0742636..ed3af55486 100644 --- a/internal/ptx/fake_test.go +++ b/internal/ptx/fake_test.go @@ -6,6 +6,10 @@ import ( ) func TestFakeDialerWorks(t *testing.T) { + if testing.Short() { + t.Skip("skip test in short mode") + } + fd := &FakeDialer{Address: "8.8.8.8:53"} conn, err := fd.DialContext(context.Background()) if err != nil { diff --git a/internal/ptx/ptx_test.go b/internal/ptx/ptx_test.go index f5d1f026ba..88f231cf88 100644 --- a/internal/ptx/ptx_test.go +++ b/internal/ptx/ptx_test.go @@ -25,6 +25,10 @@ func TestListenerLoggerWorks(t *testing.T) { } func TestListenerWorksWithFakeDialer(t *testing.T) { + if testing.Short() { + t.Skip("skip test in short mode") + } + // start the fake PT fd := &FakeDialer{Address: "google.com:80"} lst := &Listener{PTDialer: fd} diff --git a/internal/testingquic/testingquic.go b/internal/testingquic/testingquic.go new file mode 100644 index 0000000000..a19aafa87c --- /dev/null +++ b/internal/testingquic/testingquic.go @@ -0,0 +1,59 @@ +// Package testingquic allows to retrieve the domain and endpoint to use +// for all the integration tests that use QUIC. +// +// See https://github.com/ooni/probe/issues/1873 for context. +package testingquic + +import ( + "context" + "net" + "strings" + "sync" + "time" + + "github.com/ooni/probe-cli/v3/internal/runtimex" +) + +const domain = "www.cloudflare.com" + +var ( + address string + initOnce sync.Once +) + +func mustInit() { + // create a context using a reasonable timeout + const timeout = 10 * time.Second + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + + // instantiate the system resolver + reso := &net.Resolver{} + + // perform the lookup and panic on failure + addrs := runtimex.Try1(reso.LookupHost(ctx, domain)) + + // use the first non IPv6 addr + for _, addr := range addrs { + if !strings.Contains(addr, ":") { + address = addr + return + } + } +} + +// MustEndpoint returns a QUIC endpoint using this package's default address and then given port. +// +// This function PANICS if we cannot find out the IP addr we should use. +func MustEndpoint(port string) string { + initOnce.Do(mustInit) + return net.JoinHostPort(address, port) +} + +// MustDomain returns the domain to use for QUIC testing. +// +// This function PANICS if we cannot find out the IP addr we should use. +func MustDomain() string { + initOnce.Do(mustInit) + return domain +} diff --git a/internal/netxlite/quictesting/quictesting_test.go b/internal/testingquic/testingquic_test.go similarity index 50% rename from internal/netxlite/quictesting/quictesting_test.go rename to internal/testingquic/testingquic_test.go index 0a8f99e390..47c67c4483 100644 --- a/internal/netxlite/quictesting/quictesting_test.go +++ b/internal/testingquic/testingquic_test.go @@ -1,4 +1,4 @@ -package quictesting +package testingquic import ( "net" @@ -6,12 +6,17 @@ import ( ) func TestWorksAsIntended(t *testing.T) { - epnt := Endpoint("443") - addr, port, err := net.SplitHostPort(epnt) + if testing.Short() { + t.Skip("skip test in short mode") + } + + endpoint := MustEndpoint("443") + addr, port, err := net.SplitHostPort(endpoint) if err != nil { t.Fatal(err) } - if addr != Address { + + if addr != address { t.Fatal("invalid addr") } if port != "443" { diff --git a/pkg/oonimkall/xoonirun_test.go b/pkg/oonimkall/xoonirun_test.go index 32fae4d214..7a88193049 100644 --- a/pkg/oonimkall/xoonirun_test.go +++ b/pkg/oonimkall/xoonirun_test.go @@ -16,6 +16,10 @@ import ( func TestOONIRunFetch(t *testing.T) { t.Run("we can fetch a OONI Run link descriptor", func(t *testing.T) { + if testing.Short() { + t.Skip("skip test in short mode") + } + sess, err := NewSessionForTesting() if err != nil { t.Fatal(err) diff --git a/script/linuxcoverage.bash b/script/linuxcoverage.bash new file mode 100755 index 0000000000..944c69fe14 --- /dev/null +++ b/script/linuxcoverage.bash @@ -0,0 +1,18 @@ +#!/bin/bash + +# +# Computes coverage inside an environment where we unshared the network namespace +# to ensure unit tests don't depend on the network. +# + +set -euxo pipefail + +# obtain the full path of the go executable +go=$(which go) + +# populate the vendor directory so we don't need the network in `go test` +go mod vendor + +# run tests using a different network namespace +sudo unshare --net ./script/linuxcoveragerun.bash $go + diff --git a/script/linuxcoveragerun.bash b/script/linuxcoveragerun.bash new file mode 100755 index 0000000000..a3eb15b483 --- /dev/null +++ b/script/linuxcoveragerun.bash @@ -0,0 +1,18 @@ +#!/bin/bash + +# +# Script invoked by ./script/linuxcoverage.bash to run tests with coverage +# using a separate network namespace with only loopback support. +# +# The first an unique argument is the path to the go binary to use. +# + +set -euxo pipefail + +# make sure we have access to loopback since we have many ~unit +# tests using the loopback interface +ip link set lo up + +# make sure we run all the "unit" tests (where "unit" means proper unit +# tests or tests using localhost or tests using netemx). +$1 test -short -race -count 1 -coverprofile=probe-cli.cov ./...