From 99ec7ffca98617f15232baa1328dbfcdd533af85 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Fri, 7 Jan 2022 13:17:20 +0100 Subject: [PATCH] fix: ensure experiments return nil when we want to submit (#654) Since https://github.com/ooni/probe-cli/pull/527, if an experiment returns an error, the corresponding measurement is not submitted since the semantics of returning an error is that something fundamental went wrong (e.g., we could not parse the input URL). This diff ensures that all experiments only return and error when something fundamental was wrong and return nil otherwise. Reference issue: https://github.com/ooni/probe/issues/1808. --- internal/engine/experiment/dash/dash.go | 10 +++- internal/engine/experiment/dash/dash_test.go | 6 ++- internal/engine/experiment/dash/doc.go | 4 ++ .../engine/experiment/dnscheck/dnscheck.go | 3 ++ internal/engine/experiment/example/example.go | 4 ++ internal/engine/experiment/ndt7/dial.go | 3 ++ internal/engine/experiment/ndt7/dial_test.go | 6 +-- internal/engine/experiment/ndt7/doc.go | 4 ++ internal/engine/experiment/ndt7/ndt7.go | 20 +++---- internal/engine/experiment/ndt7/ndt7_test.go | 52 ++++++++++++++----- internal/engine/experiment/psiphon/psiphon.go | 6 +-- .../engine/experiment/psiphon/psiphon_test.go | 8 +-- internal/engine/experiment/run/dnscheck.go | 2 - internal/engine/experiment/run/doc.go | 4 ++ internal/engine/experiment/run/run.go | 3 -- internal/engine/experiment/run/run_test.go | 2 +- .../stunreachability/stunreachability.go | 4 +- .../stunreachability/stunreachability_test.go | 10 ++-- internal/engine/experiment/tlstool/tlstool.go | 2 +- internal/engine/experiment/tor/tor.go | 2 +- .../engine/experiment/urlgetter/urlgetter.go | 6 +-- .../experiment/urlgetter/urlgetter_test.go | 8 +-- .../engine/experiment/webconnectivity/doc.go | 4 ++ .../webconnectivity/webconnectivity.go | 3 -- internal/model/experiment.go | 2 +- internal/netxlite/http.go | 7 +++ internal/netxlite/http_test.go | 13 +++++ 27 files changed, 132 insertions(+), 66 deletions(-) create mode 100644 internal/engine/experiment/dash/doc.go create mode 100644 internal/engine/experiment/ndt7/doc.go create mode 100644 internal/engine/experiment/run/doc.go create mode 100644 internal/engine/experiment/webconnectivity/doc.go diff --git a/internal/engine/experiment/dash/dash.go b/internal/engine/experiment/dash/dash.go index 460d250..7807bd5 100644 --- a/internal/engine/experiment/dash/dash.go +++ b/internal/engine/experiment/dash/dash.go @@ -25,7 +25,7 @@ const ( defaultTimeout = 120 * time.Second magicVersion = "0.008000000" testName = "dash" - testVersion = "0.12.0" + testVersion = "0.13.0" totalStep = 15.0 ) @@ -273,7 +273,13 @@ func (m Measurer) Run( } ctx, cancel := context.WithTimeout(ctx, defaultTimeout) defer cancel() - return r.do(ctx) + // Implementation note: we ignore the return value of r.do rather than + // returning it to the caller. We do that because returning an error means + // the measurement failed for some fundamental reason (e.g., the input + // is an URL that you cannot parse). For DASH, this case will never happen + // because there is no input, so always returning nil is fine here. + _ = r.do(ctx) + return nil } // NewExperimentMeasurer creates a new ExperimentMeasurer. diff --git a/internal/engine/experiment/dash/dash_test.go b/internal/engine/experiment/dash/dash_test.go index cd77c70..3961c62 100644 --- a/internal/engine/experiment/dash/dash_test.go +++ b/internal/engine/experiment/dash/dash_test.go @@ -261,7 +261,7 @@ func TestNewExperimentMeasurer(t *testing.T) { if measurer.ExperimentName() != "dash" { t.Fatal("unexpected name") } - if measurer.ExperimentVersion() != "0.12.0" { + if measurer.ExperimentVersion() != "0.13.0" { t.Fatal("unexpected version") } } @@ -280,7 +280,9 @@ func TestMeasureWithCancelledContext(t *testing.T) { measurement, model.NewPrinterCallbacks(log.Log), ) - if !errors.Is(err, context.Canceled) { + // See corresponding comment in Measurer.Run implementation to + // understand why here it's correct to return nil. + if !errors.Is(err, nil) { t.Fatal("unexpected error value") } sk, err := m.GetSummaryKeys(measurement) diff --git a/internal/engine/experiment/dash/doc.go b/internal/engine/experiment/dash/doc.go new file mode 100644 index 0000000..d33d168 --- /dev/null +++ b/internal/engine/experiment/dash/doc.go @@ -0,0 +1,4 @@ +// Package dash implements the DASH network experiment. +// +// Spec: https://github.com/ooni/spec/blob/master/nettests/ts-021-dash.md +package dash diff --git a/internal/engine/experiment/dnscheck/dnscheck.go b/internal/engine/experiment/dnscheck/dnscheck.go index 778377f..8c04c08 100644 --- a/internal/engine/experiment/dnscheck/dnscheck.go +++ b/internal/engine/experiment/dnscheck/dnscheck.go @@ -159,6 +159,9 @@ func (m *Measurer) Run( return ErrUnsupportedURLScheme } + // Implementation note: we must not return an error from now now. Returning an + // error means that we don't have a measurement to submit. + // 4. possibly expand a domain to a list of IP addresses. // // Implementation note: because the resolver we constructed also deals diff --git a/internal/engine/experiment/example/example.go b/internal/engine/experiment/example/example.go index 96ef4e8..4b6389f 100644 --- a/internal/engine/experiment/example/example.go +++ b/internal/engine/experiment/example/example.go @@ -73,6 +73,10 @@ func (m Measurer) Run( <-ctx.Done() sess.Logger().Infof("%s", "Knock, knock, Neo.") callbacks.OnProgress(1.0, m.config.Message) + // Note: if here we return an error, the parent code will assume + // something fundamental was wrong and we don't have a measurement + // to submit to the OONI collector. Keep this in mind when you + // are writing new experiments! return err } diff --git a/internal/engine/experiment/ndt7/dial.go b/internal/engine/experiment/ndt7/dial.go index 80101aa..224cc35 100644 --- a/internal/engine/experiment/ndt7/dial.go +++ b/internal/engine/experiment/ndt7/dial.go @@ -55,6 +55,9 @@ func (mgr dialManager) dialWithTestName(ctx context.Context, testName string) (* headers.Add("User-Agent", mgr.userAgent) mgr.logrequest(mgr.ndt7URL, headers) conn, _, err := dialer.DialContext(ctx, mgr.ndt7URL, headers) + if err != nil { + err = netxlite.NewTopLevelGenericErrWrapper(err) + } mgr.logresponse(err) return conn, err } diff --git a/internal/engine/experiment/ndt7/dial_test.go b/internal/engine/experiment/ndt7/dial_test.go index 56f3629..8f43b4c 100644 --- a/internal/engine/experiment/ndt7/dial_test.go +++ b/internal/engine/experiment/ndt7/dial_test.go @@ -6,11 +6,11 @@ import ( "net/http" "net/http/httptest" "net/url" - "strings" "testing" "github.com/apex/log" "github.com/gorilla/websocket" + "github.com/ooni/probe-cli/v3/internal/netxlite" ) func TestDialDownloadWithCancelledContext(t *testing.T) { @@ -18,7 +18,7 @@ func TestDialDownloadWithCancelledContext(t *testing.T) { cancel() // immediately halt mgr := newDialManager("wss://hostname.fake", log.Log, "miniooni/0.1.0-dev") conn, err := mgr.dialDownload(ctx) - if err == nil || !strings.HasSuffix(err.Error(), "context canceled") { + if err == nil || err.Error() != netxlite.FailureInterrupted { t.Fatal("not the error we expected", err) } if conn != nil { @@ -31,7 +31,7 @@ func TestDialUploadWithCancelledContext(t *testing.T) { cancel() // immediately halt mgr := newDialManager("wss://hostname.fake", log.Log, "miniooni/0.1.0-dev") conn, err := mgr.dialUpload(ctx) - if err == nil || !strings.HasSuffix(err.Error(), "context canceled") { + if err == nil || err.Error() != netxlite.FailureInterrupted { t.Fatal("not the error we expected", err) } if conn != nil { diff --git a/internal/engine/experiment/ndt7/doc.go b/internal/engine/experiment/ndt7/doc.go new file mode 100644 index 0000000..236d304 --- /dev/null +++ b/internal/engine/experiment/ndt7/doc.go @@ -0,0 +1,4 @@ +// Package ndt7 contains the ndt7 network experiment. +// +// See https://github.com/ooni/spec/blob/master/nettests/ts-022-ndt.md +package ndt7 diff --git a/internal/engine/experiment/ndt7/ndt7.go b/internal/engine/experiment/ndt7/ndt7.go index 0eca0bc..5ccb9dd 100644 --- a/internal/engine/experiment/ndt7/ndt7.go +++ b/internal/engine/experiment/ndt7/ndt7.go @@ -1,6 +1,3 @@ -// Package ndt7 contains the ndt7 network experiment. -// -// See https://github.com/ooni/spec/blob/master/nettests/ts-022-ndt.md package ndt7 import ( @@ -8,18 +5,17 @@ import ( "encoding/json" "errors" "fmt" - "net/http" "time" - "github.com/ooni/probe-cli/v3/internal/engine/netx" "github.com/ooni/probe-cli/v3/internal/humanize" "github.com/ooni/probe-cli/v3/internal/mlablocatev2" "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/netxlite" ) const ( testName = "ndt" - testVersion = "0.9.0" + testVersion = "0.10.0" ) // Config contains the experiment settings @@ -80,11 +76,7 @@ type Measurer struct { func (m *Measurer) discover( ctx context.Context, sess model.ExperimentSession) (mlablocatev2.NDT7Result, error) { - httpClient := &http.Client{ - Transport: netx.NewHTTPTransport(netx.Config{ - Logger: sess.Logger(), - }), - } + httpClient := netxlite.NewHTTPClientStdlib(sess.Logger()) defer httpClient.CloseIdleConnections() client := mlablocatev2.NewClient(httpClient, sess.Logger(), sess.UserAgent()) out, err := client.QueryNDT7(ctx) @@ -228,7 +220,7 @@ func (m *Measurer) Run( locateResult, err := m.discover(ctx, sess) if err != nil { tk.Failure = failureFromError(err) - return err + return nil // we still want to submit this measurement } tk.Server = ServerInfo{ Hostname: locateResult.Hostname, @@ -240,7 +232,7 @@ func (m *Measurer) Run( } if err := m.doDownload(ctx, sess, callbacks, tk, locateResult.WSSDownloadURL); err != nil { tk.Failure = failureFromError(err) - return err + return nil // we still want to submit this measurement } callbacks.OnProgress(0.5, fmt.Sprintf(" upload: url: %s", locateResult.WSSUploadURL)) if m.preUploadHook != nil { @@ -248,7 +240,7 @@ func (m *Measurer) Run( } if err := m.doUpload(ctx, sess, callbacks, tk, locateResult.WSSUploadURL); err != nil { tk.Failure = failureFromError(err) - return err + return nil // we still want to submit this measurement } return nil } diff --git a/internal/engine/experiment/ndt7/ndt7_test.go b/internal/engine/experiment/ndt7/ndt7_test.go index d9c4fd3..0616295 100644 --- a/internal/engine/experiment/ndt7/ndt7_test.go +++ b/internal/engine/experiment/ndt7/ndt7_test.go @@ -4,12 +4,12 @@ import ( "context" "errors" "net/http" - "strings" "testing" "github.com/apex/log" "github.com/ooni/probe-cli/v3/internal/engine/mockable" "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/netxlite" ) func TestNewExperimentMeasurer(t *testing.T) { @@ -17,7 +17,7 @@ func TestNewExperimentMeasurer(t *testing.T) { if measurer.ExperimentName() != "ndt" { t.Fatal("unexpected name") } - if measurer.ExperimentVersion() != "0.9.0" { + if measurer.ExperimentVersion() != "0.10.0" { t.Fatal("unexpected version") } } @@ -52,7 +52,7 @@ func TestDoDownloadWithCancelledContext(t *testing.T) { err := m.doDownload( ctx, sess, model.NewPrinterCallbacks(log.Log), new(TestKeys), "ws://host.name") - if err == nil || !strings.HasSuffix(err.Error(), "context canceled") { + if err == nil || err.Error() != netxlite.FailureInterrupted { t.Fatal("not the error we expected", err) } } @@ -69,7 +69,7 @@ func TestDoUploadWithCancelledContext(t *testing.T) { err := m.doUpload( ctx, sess, model.NewPrinterCallbacks(log.Log), new(TestKeys), "ws://host.name") - if err == nil || !strings.HasSuffix(err.Error(), "context canceled") { + if err == nil || err.Error() != netxlite.FailureInterrupted { t.Fatal("not the error we expected", err) } } @@ -83,10 +83,19 @@ func TestRunWithCancelledContext(t *testing.T) { } ctx, cancel := context.WithCancel(context.Background()) cancel() // immediately cancel - err := m.Run(ctx, sess, new(model.Measurement), model.NewPrinterCallbacks(log.Log)) - if !errors.Is(err, context.Canceled) { + meas := &model.Measurement{} + err := m.Run(ctx, sess, meas, model.NewPrinterCallbacks(log.Log)) + // Here we get nil because we still want to submit this measurement + if !errors.Is(err, nil) { t.Fatal("not the error we expected") } + if meas.TestKeys == nil { + t.Fatal("nil test keys") + } + tk := meas.TestKeys.(*TestKeys) + if tk.Failure == nil || *tk.Failure != netxlite.FailureInterrupted { + t.Fatal("unexpected tk.Failure") + } } func TestGood(t *testing.T) { @@ -123,17 +132,27 @@ func TestFailDownload(t *testing.T) { measurer.preDownloadHook = func() { cancel() } + meas := &model.Measurement{} err := measurer.Run( ctx, &mockable.Session{ MockableHTTPClient: http.DefaultClient, MockableLogger: log.Log, }, - new(model.Measurement), + meas, model.NewPrinterCallbacks(log.Log), ) - if err == nil || !strings.HasSuffix(err.Error(), "context canceled") { - t.Fatal("not the error we expected", err) + // We expect a nil failure here because we want to submit anyway + // a measurement that failed to connect to m-lab. + if err != nil { + t.Fatal(err) + } + if meas.TestKeys == nil { + t.Fatal("expected non-nil TestKeys here") + } + tk := meas.TestKeys.(*TestKeys) + if tk.Failure == nil || *tk.Failure != netxlite.FailureInterrupted { + t.Fatal("unexpected tk.Failure") } } @@ -144,17 +163,26 @@ func TestFailUpload(t *testing.T) { measurer.preUploadHook = func() { cancel() } + meas := &model.Measurement{} err := measurer.Run( ctx, &mockable.Session{ MockableHTTPClient: http.DefaultClient, MockableLogger: log.Log, }, - new(model.Measurement), + meas, model.NewPrinterCallbacks(log.Log), ) - if err == nil || !strings.HasSuffix(err.Error(), "context canceled") { - t.Fatal("not the error we expected", err) + // Here we expect a nil error because we want to submit this measurement + if err != nil { + t.Fatal(err) + } + if meas.TestKeys == nil { + t.Fatal("expected non-nil tk.TestKeys here") + } + tk := meas.TestKeys.(*TestKeys) + if tk.Failure == nil || *tk.Failure != netxlite.FailureInterrupted { + t.Fatal("unexpected tk.Failure value") } } diff --git a/internal/engine/experiment/psiphon/psiphon.go b/internal/engine/experiment/psiphon/psiphon.go index 3a09323..2a34b44 100644 --- a/internal/engine/experiment/psiphon/psiphon.go +++ b/internal/engine/experiment/psiphon/psiphon.go @@ -16,7 +16,7 @@ import ( const ( testName = "psiphon" - testVersion = "0.5.1" + testVersion = "0.6.0" ) // Config contains the experiment's configuration. @@ -92,14 +92,14 @@ func (m *Measurer) Run( if m.BeforeGetHook != nil { m.BeforeGetHook(g) } - tk, err := g.Get(ctx) + tk, _ := g.Get(ctx) // ignore error since we have the testkeys and want to submit them cancel() wg.Wait() measurement.TestKeys = &TestKeys{ TestKeys: tk, MaxRuntime: maxruntime, } - return err + return nil } // NewExperimentMeasurer creates a new ExperimentMeasurer. diff --git a/internal/engine/experiment/psiphon/psiphon_test.go b/internal/engine/experiment/psiphon/psiphon_test.go index 43aff1c..112ccdf 100644 --- a/internal/engine/experiment/psiphon/psiphon_test.go +++ b/internal/engine/experiment/psiphon/psiphon_test.go @@ -23,7 +23,7 @@ func TestNewExperimentMeasurer(t *testing.T) { if measurer.ExperimentName() != "psiphon" { t.Fatal("unexpected name") } - if measurer.ExperimentVersion() != "0.5.1" { + if measurer.ExperimentVersion() != "0.6.0" { t.Fatal("unexpected version") } } @@ -35,7 +35,7 @@ func TestRunWithCancelledContext(t *testing.T) { measurement := new(model.Measurement) err := measurer.Run(ctx, newfakesession(), measurement, model.NewPrinterCallbacks(log.Log)) - if !errors.Is(err, context.Canceled) { + if !errors.Is(err, nil) { // nil because we want to submit the measurement t.Fatal("expected another error here") } tk := measurement.TestKeys.(*psiphon.TestKeys) @@ -66,7 +66,7 @@ func TestRunWithCustomInputAndCancelledContext(t *testing.T) { cancel() // fail immediately err := measurer.Run(ctx, newfakesession(), measurement, model.NewPrinterCallbacks(log.Log)) - if !errors.Is(err, context.Canceled) { + if !errors.Is(err, nil) { // nil because we want to submit the measurement t.Fatal("expected another error here") } tk := measurement.TestKeys.(*psiphon.TestKeys) @@ -85,7 +85,7 @@ func TestRunWillPrintSomethingWithCancelledContext(t *testing.T) { } observer := observerCallbacks{progress: &atomicx.Int64{}} err := measurer.Run(ctx, newfakesession(), measurement, observer) - if !errors.Is(err, context.Canceled) { + if !errors.Is(err, nil) { // nil because we want to submit the measurement t.Fatal("expected another error here") } tk := measurement.TestKeys.(*psiphon.TestKeys) diff --git a/internal/engine/experiment/run/dnscheck.go b/internal/engine/experiment/run/dnscheck.go index a116fb5..da3fd84 100644 --- a/internal/engine/experiment/run/dnscheck.go +++ b/internal/engine/experiment/run/dnscheck.go @@ -2,7 +2,6 @@ package run import ( "context" - "sync" "github.com/ooni/probe-cli/v3/internal/engine/experiment/dnscheck" "github.com/ooni/probe-cli/v3/internal/model" @@ -10,7 +9,6 @@ import ( type dnsCheckMain struct { Endpoints *dnscheck.Endpoints - mu sync.Mutex } func (m *dnsCheckMain) do(ctx context.Context, input StructuredInput, diff --git a/internal/engine/experiment/run/doc.go b/internal/engine/experiment/run/doc.go new file mode 100644 index 0000000..bfb076a --- /dev/null +++ b/internal/engine/experiment/run/doc.go @@ -0,0 +1,4 @@ +// Package run contains code to run other experiments. +// +// This code is currently alpha. +package run diff --git a/internal/engine/experiment/run/run.go b/internal/engine/experiment/run/run.go index d633a64..6c38057 100644 --- a/internal/engine/experiment/run/run.go +++ b/internal/engine/experiment/run/run.go @@ -1,6 +1,3 @@ -// Package run contains code to run other experiments. -// -// This code is currently alpha. package run import ( diff --git a/internal/engine/experiment/run/run_test.go b/internal/engine/experiment/run/run_test.go index df79e3c..06ae406 100644 --- a/internal/engine/experiment/run/run_test.go +++ b/internal/engine/experiment/run/run_test.go @@ -63,7 +63,7 @@ func TestRunURLGetterWithCancelledContext(t *testing.T) { sess := &mockable.Session{MockableLogger: log.Log} callbacks := model.NewPrinterCallbacks(log.Log) err := measurer.Run(ctx, sess, measurement, callbacks) - if err == nil { + if err != nil { // here we expected nil b/c we want to submit the measurement t.Fatal(err) } if len(measurement.Extensions) != 6 { diff --git a/internal/engine/experiment/stunreachability/stunreachability.go b/internal/engine/experiment/stunreachability/stunreachability.go index d3c47c9..a19b263 100644 --- a/internal/engine/experiment/stunreachability/stunreachability.go +++ b/internal/engine/experiment/stunreachability/stunreachability.go @@ -22,7 +22,7 @@ import ( const ( testName = "stunreachability" - testVersion = "0.3.0" + testVersion = "0.4.0" ) // Config contains the experiment config. @@ -100,7 +100,7 @@ func (m *Measurer) Run( if err := wrap(tk.run(ctx, m.config, sess, measurement, callbacks, URL.Host)); err != nil { s := err.Error() tk.Failure = &s - return err + return nil // we want to submit this measurement } return nil } diff --git a/internal/engine/experiment/stunreachability/stunreachability_test.go b/internal/engine/experiment/stunreachability/stunreachability_test.go index d928b73..81b1a3f 100644 --- a/internal/engine/experiment/stunreachability/stunreachability_test.go +++ b/internal/engine/experiment/stunreachability/stunreachability_test.go @@ -24,7 +24,7 @@ func TestMeasurerExperimentNameVersion(t *testing.T) { if measurer.ExperimentName() != "stunreachability" { t.Fatal("unexpected ExperimentName") } - if measurer.ExperimentVersion() != "0.3.0" { + if measurer.ExperimentVersion() != "0.4.0" { t.Fatal("unexpected ExperimentVersion") } } @@ -128,7 +128,7 @@ func TestCancelledContext(t *testing.T) { measurement, model.NewPrinterCallbacks(log.Log), ) - if !errors.Is(err, context.Canceled) { + if !errors.Is(err, nil) { // nil because we want to submit t.Fatal("not the error we expected", err) } tk := measurement.TestKeys.(*TestKeys) @@ -168,7 +168,7 @@ func TestNewClientFailure(t *testing.T) { measurement, model.NewPrinterCallbacks(log.Log), ) - if !errors.Is(err, expected) { + if !errors.Is(err, nil) { // nil because we want to submit t.Fatal("not the error we expected") } tk := measurement.TestKeys.(*TestKeys) @@ -202,7 +202,7 @@ func TestStartFailure(t *testing.T) { measurement, model.NewPrinterCallbacks(log.Log), ) - if !errors.Is(err, expected) { + if !errors.Is(err, nil) { // nil because we want to submit t.Fatal("not the error we expected") } tk := measurement.TestKeys.(*TestKeys) @@ -240,7 +240,7 @@ func TestReadFailure(t *testing.T) { measurement, model.NewPrinterCallbacks(log.Log), ) - if !errors.Is(err, stun.ErrTransactionTimeOut) { + if !errors.Is(err, nil) { // nil because we want to submit t.Fatal("not the error we expected") } tk := measurement.TestKeys.(*TestKeys) diff --git a/internal/engine/experiment/tlstool/tlstool.go b/internal/engine/experiment/tlstool/tlstool.go index d61a844..4416d41 100644 --- a/internal/engine/experiment/tlstool/tlstool.go +++ b/internal/engine/experiment/tlstool/tlstool.go @@ -104,7 +104,7 @@ func (m Measurer) Run( Failure: archival.NewFailure(err), } } - return nil + return nil // return nil so we always submit the measurement } func (m Measurer) newDialer(logger model.Logger) netx.Dialer { diff --git a/internal/engine/experiment/tor/tor.go b/internal/engine/experiment/tor/tor.go index d563adf..e95f55b 100644 --- a/internal/engine/experiment/tor/tor.go +++ b/internal/engine/experiment/tor/tor.go @@ -174,7 +174,7 @@ func (m *Measurer) Run( ) error { targets, err := m.gimmeTargets(ctx, sess) if err != nil { - return err + return err // fail the measurement if we cannot get any target } registerExtensions(measurement) m.measureTargets(ctx, sess, measurement, callbacks, targets) diff --git a/internal/engine/experiment/urlgetter/urlgetter.go b/internal/engine/experiment/urlgetter/urlgetter.go index 46c3495..e1ea4e4 100644 --- a/internal/engine/experiment/urlgetter/urlgetter.go +++ b/internal/engine/experiment/urlgetter/urlgetter.go @@ -14,7 +14,7 @@ import ( const ( testName = "urlgetter" - testVersion = "0.1.0" + testVersion = "0.2.0" ) // Config contains the experiment's configuration. @@ -109,9 +109,9 @@ func (m Measurer) Run( Session: sess, Target: string(measurement.Input), } - tk, err := g.Get(ctx) + tk, _ := g.Get(ctx) // ignore error since we have the testkeys and we wanna submit them measurement.TestKeys = &tk - return err + return nil } // NewExperimentMeasurer creates a new ExperimentMeasurer. diff --git a/internal/engine/experiment/urlgetter/urlgetter_test.go b/internal/engine/experiment/urlgetter/urlgetter_test.go index a49ada1..eaa8f7b 100644 --- a/internal/engine/experiment/urlgetter/urlgetter_test.go +++ b/internal/engine/experiment/urlgetter/urlgetter_test.go @@ -18,7 +18,7 @@ func TestMeasurer(t *testing.T) { if m.ExperimentName() != "urlgetter" { t.Fatal("invalid experiment name") } - if m.ExperimentVersion() != "0.1.0" { + if m.ExperimentVersion() != "0.2.0" { t.Fatal("invalid experiment version") } measurement := new(model.Measurement) @@ -27,7 +27,7 @@ func TestMeasurer(t *testing.T) { ctx, &mockable.Session{}, measurement, model.NewPrinterCallbacks(log.Log), ) - if !errors.Is(err, context.Canceled) { + if !errors.Is(err, nil) { // nil because we want to submit the measurement t.Fatal("not the error we expected") } if len(measurement.Extensions) != 6 { @@ -55,7 +55,7 @@ func TestMeasurerDNSCache(t *testing.T) { if m.ExperimentName() != "urlgetter" { t.Fatal("invalid experiment name") } - if m.ExperimentVersion() != "0.1.0" { + if m.ExperimentVersion() != "0.2.0" { t.Fatal("invalid experiment version") } measurement := new(model.Measurement) @@ -64,7 +64,7 @@ func TestMeasurerDNSCache(t *testing.T) { ctx, &mockable.Session{}, measurement, model.NewPrinterCallbacks(log.Log), ) - if !errors.Is(err, context.Canceled) { + if !errors.Is(err, nil) { // nil because we want to submit the measurement t.Fatal("not the error we expected") } if len(measurement.Extensions) != 6 { diff --git a/internal/engine/experiment/webconnectivity/doc.go b/internal/engine/experiment/webconnectivity/doc.go new file mode 100644 index 0000000..8da0775 --- /dev/null +++ b/internal/engine/experiment/webconnectivity/doc.go @@ -0,0 +1,4 @@ +// Package webconnectivity implements OONI's Web Connectivity experiment. +// +// See https://github.com/ooni/spec/blob/master/nettests/ts-017-web-connectivity.md +package webconnectivity diff --git a/internal/engine/experiment/webconnectivity/webconnectivity.go b/internal/engine/experiment/webconnectivity/webconnectivity.go index b81a44c..469acbc 100644 --- a/internal/engine/experiment/webconnectivity/webconnectivity.go +++ b/internal/engine/experiment/webconnectivity/webconnectivity.go @@ -1,6 +1,3 @@ -// Package webconnectivity implements OONI's Web Connectivity experiment. -// -// See https://github.com/ooni/spec/blob/master/nettests/ts-017-web-connectivity.md package webconnectivity import ( diff --git a/internal/model/experiment.go b/internal/model/experiment.go index 36e30fe..44b71ea 100644 --- a/internal/model/experiment.go +++ b/internal/model/experiment.go @@ -105,7 +105,7 @@ type ExperimentMeasurer interface { // return an error in case the experiment could not run (e.g., // a required input is missing). Otherwise, the code should just // set the relevant OONI error inside of the measurement and - // return nil. This is important because the caller may not submit + // return nil. This is important because the caller WILL NOT submit // the measurement if this method returns an error. Run( ctx context.Context, sess ExperimentSession, diff --git a/internal/netxlite/http.go b/internal/netxlite/http.go index 02ad5b8..de00b77 100644 --- a/internal/netxlite/http.go +++ b/internal/netxlite/http.go @@ -257,6 +257,13 @@ func NewHTTPTransportStdlib(logger model.DebugLogger) model.HTTPTransport { return NewHTTPTransport(logger, dialer, tlsDialer) } +// NewHTTPClientStdlib creates a new HTTPClient that uses the +// standard library for TLS and DNS resolutions. +func NewHTTPClientStdlib(logger model.DebugLogger) model.HTTPClient { + txp := NewHTTPTransportStdlib(logger) + return WrapHTTPClient(&http.Client{Transport: txp}) +} + // WrapHTTPClient wraps an HTTP client to add error wrapping capabilities. func WrapHTTPClient(clnt model.HTTPClient) model.HTTPClient { return &httpClientErrWrapper{clnt} diff --git a/internal/netxlite/http_test.go b/internal/netxlite/http_test.go index a835af5..a160c1c 100644 --- a/internal/netxlite/http_test.go +++ b/internal/netxlite/http_test.go @@ -13,6 +13,7 @@ import ( "github.com/apex/log" oohttp "github.com/ooni/oohttp" "github.com/ooni/probe-cli/v3/internal/atomicx" + "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/model/mocks" ) @@ -500,6 +501,18 @@ func TestHTTPClientErrWrapper(t *testing.T) { }) } +func TestNewHTTPClientStdlib(t *testing.T) { + clnt := NewHTTPClientStdlib(model.DiscardLogger) + ewc, ok := clnt.(*httpClientErrWrapper) + if !ok { + t.Fatal("expected *httpClientErrWrapper") + } + _, ok = ewc.HTTPClient.(*http.Client) + if !ok { + t.Fatal("expected *http.Client") + } +} + func TestWrapHTTPClient(t *testing.T) { origClient := &http.Client{} wrapped := WrapHTTPClient(origClient)