oonimkall: mobile api for running WebConnectivity (#223)

* poc: mobile api for running WebConnectivity

Yesterday we had a team meeting where we discussed the importance
of stopping using MK wrappers for running experiments.

Here's a PoC showing how we can do that for WebConnectivity. It
seems to me we probably want to add more tests and merge this code
such that we can experiment with it quite soon.

There seems to be opportunities for auto-generating code, BTW.

While working on this PoC, I've also took a chance to revamp the
external documentation of pkg/oonimkall.

* feat: start making the code more abstract

* chore: write unit tests for new code

* fix(oonimkall): improve naming

* refactor: cosmetic changes

* fix: explain

pkg/oonimkall/experiment.go

@@ -0,0 +1,96 @@
+package oonimkall
+
+import (
+	"context"
+
+	"github.com/ooni/probe-cli/v3/internal/engine"
+	"github.com/ooni/probe-cli/v3/internal/engine/model"
+)
+
+// experimentSession is the abstract representation of
+// a session according to an experiment.
+type experimentSession interface {
+	// lock locks the session
+	lock()
+
+	// maybeLookupBackends lookups the backends
+	maybeLookupBackends(ctx context.Context) error
+
+	// maybeLookupLocations lookups the probe location
+	maybeLookupLocation(ctx context.Context) error
+
+	// newExperimentBuilder creates a new experiment builder
+	newExperimentBuilder(name string) (experimentBuilder, error)
+
+	// unlock unlocks the session
+	unlock()
+}
+
+// lock implements experimentSession.lock
+func (sess *Session) lock() {
+	sess.mtx.Lock()
+}
+
+// maybeLookupBackends implements experimentSession.maybeLookupBackends
+func (sess *Session) maybeLookupBackends(ctx context.Context) error {
+	return sess.sessp.MaybeLookupBackendsContext(ctx)
+}
+
+// maybeLookupLocation implements experimentSession.maybeLookupLocation
+func (sess *Session) maybeLookupLocation(ctx context.Context) error {
+	return sess.sessp.MaybeLookupLocationContext(ctx)
+}
+
+// newExperimentBuilder implements experimentSession.newExperimentBuilder
+func (sess *Session) newExperimentBuilder(name string) (experimentBuilder, error) {
+	eb, err := sess.sessp.NewExperimentBuilder(name)
+	if err != nil {
+		return nil, err
+	}
+	return &experimentBuilderWrapper{eb: eb}, nil
+}
+
+// unlock implements experimentSession.unlock
+func (sess *Session) unlock() {
+	sess.mtx.Unlock()
+}
+
+// experimentBuilder is the representation of an experiment
+// builder that we use inside this package.
+type experimentBuilder interface {
+	// newExperiment creates a new experiment instance
+	newExperiment() experiment
+
+	// setCallbacks sets the experiment callbacks
+	setCallbacks(ExperimentCallbacks)
+}
+
+// experimentBuilderWrapper wraps *ExperimentBuilder
+type experimentBuilderWrapper struct {
+	eb *engine.ExperimentBuilder
+}
+
+// newExperiment implements experimentBuilder.newExperiment
+func (eb *experimentBuilderWrapper) newExperiment() experiment {
+	return eb.eb.NewExperiment()
+}
+
+// setCallbacks implements experimentBuilder.setCallbacks
+func (eb *experimentBuilderWrapper) setCallbacks(cb ExperimentCallbacks) {
+	eb.eb.SetCallbacks(cb)
+}
+
+// experiment is the representation of an experiment that
+// we use inside this package for running nettests.
+type experiment interface {
+	// MeasureWithContext runs the measurement with the given input
+	// and context. It returns a measurement or an error.
+	MeasureWithContext(ctx context.Context, input string) (
+		measurement *model.Measurement, err error)
+
+	// KibiBytesSent returns the number of KiB sent.
+	KibiBytesSent() float64
+
+	// KibiBytesReceived returns the number of KiB received.
+	KibiBytesReceived() float64
+}

pkg/oonimkall/experiment_test.go

@@ -0,0 +1,93 @@
+package oonimkall
+
+import (
+	"context"
+	"sync"
+	"sync/atomic"
+
+	"github.com/ooni/probe-cli/v3/internal/engine/model"
+)
+
+// FakeExperimentCallbacks contains fake ExperimentCallbacks.
+type FakeExperimentCallbacks struct{}
+
+// OnProgress implements ExperimentCallbacks.OnProgress
+func (cb *FakeExperimentCallbacks) OnProgress(percentage float64, message string) {}
+
+// FakeExperimentSession is a fake experimentSession
+type FakeExperimentSession struct {
+	ExperimentBuilder       experimentBuilder
+	LockCount               int32
+	LookupBackendsErr       error
+	LookupLocationErr       error
+	NewExperimentBuilderErr error
+	UnlockCount             int32
+}
+
+// lock implements experimentSession.lock
+func (sess *FakeExperimentSession) lock() {
+	atomic.AddInt32(&sess.LockCount, 1)
+}
+
+// maybeLookupBackends implements experimentSession.maybeLookupBackends
+func (sess *FakeExperimentSession) maybeLookupBackends(ctx context.Context) error {
+	return sess.LookupBackendsErr
+}
+
+// maybeLookupLocation implements experimentSession.maybeLookupLocation
+func (sess *FakeExperimentSession) maybeLookupLocation(ctx context.Context) error {
+	return sess.LookupLocationErr
+}
+
+// newExperimentBuilder implements experimentSession.newExperimentBuilder
+func (sess *FakeExperimentSession) newExperimentBuilder(name string) (experimentBuilder, error) {
+	return sess.ExperimentBuilder, sess.NewExperimentBuilderErr
+}
+
+// unlock implements experimentSession.unlock
+func (sess *FakeExperimentSession) unlock() {
+	atomic.AddInt32(&sess.UnlockCount, 1)
+}
+
+// FakeExperimentBuilder is a fake experimentBuilder
+type FakeExperimentBuilder struct {
+	Callbacks  ExperimentCallbacks
+	Experiment experiment
+	mu         sync.Mutex
+}
+
+// newExperiment implements experimentBuilder.newExperiment
+func (eb *FakeExperimentBuilder) newExperiment() experiment {
+	return eb.Experiment
+}
+
+// setCallbacks implements experimentBuilder.setCallbacks
+func (eb *FakeExperimentBuilder) setCallbacks(cb ExperimentCallbacks) {
+	defer eb.mu.Unlock()
+	eb.mu.Lock()
+	eb.Callbacks = cb
+}
+
+// FakeExperiment is a fake experiment
+type FakeExperiment struct {
+	Err         error
+	Measurement *model.Measurement
+	Received    float64
+	Sent        float64
+}
+
+// MeasureWithContext implements experiment.MeasureWithContext.
+func (e *FakeExperiment) MeasureWithContext(ctx context.Context, input string) (
+	measurement *model.Measurement, err error) {
+	return e.Measurement, e.Err
+}
+
+// KibiBytesSent implements experiment.KibiBytesSent
+func (e *FakeExperiment) KibiBytesSent() float64 {
+	return e.Sent
+}
+
+// KibiBytesReceived implements experiment.KibiBytesReceived
+func (e *FakeExperiment) KibiBytesReceived() float64 {
+	return e.Received
+}

pkg/oonimkall/session_integration_test.go

@@ -18,7 +18,7 @@ import (
 	"github.com/ooni/probe-cli/v3/pkg/oonimkall"
 )
 
-func NewSessionWithAssetsDir(assetsDir string) (*oonimkall.Session, error) {
+func NewSessionForTestingWithAssetsDir(assetsDir string) (*oonimkall.Session, error) {
 	return oonimkall.NewSession(&oonimkall.SessionConfig{
 		AssetsDir:        assetsDir,
 		ProbeServicesURL: "https://ams-pg-test.ooni.org/",
@@ -29,8 +29,8 @@ func NewSessionWithAssetsDir(assetsDir string) (*oonimkall.Session, error) {
 	})
 }
 
-func NewSession() (*oonimkall.Session, error) {
+func NewSessionForTesting() (*oonimkall.Session, error) {
-	return NewSessionWithAssetsDir("../testdata/oonimkall/assets")
+	return NewSessionForTestingWithAssetsDir("../testdata/oonimkall/assets")
 }
 
 func TestNewSessionWithInvalidStateDir(t *testing.T) {
@@ -72,7 +72,7 @@ func TestMaybeUpdateResourcesWithCancelledContext(t *testing.T) {
 		t.Fatal(err)
 	}
 	defer os.RemoveAll(dir)
-	sess, err := NewSessionWithAssetsDir(dir)
+	sess, err := NewSessionForTestingWithAssetsDir(dir)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -104,7 +104,7 @@ func TestGeolocateWithCancelledContext(t *testing.T) {
 	if testing.Short() {
 		t.Skip("skip test in short mode")
 	}
-	sess, err := NewSession()
+	sess, err := NewSessionForTesting()
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -123,7 +123,7 @@ func TestGeolocateGood(t *testing.T) {
 	if testing.Short() {
 		t.Skip("skip test in short mode")
 	}
-	sess, err := NewSession()
+	sess, err := NewSessionForTesting()
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -163,7 +163,7 @@ func TestSubmitWithCancelledContext(t *testing.T) {
 	if testing.Short() {
 		t.Skip("skip test in short mode")
 	}
-	sess, err := NewSession()
+	sess, err := NewSessionForTesting()
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -182,7 +182,7 @@ func TestSubmitWithInvalidJSON(t *testing.T) {
 	if testing.Short() {
 		t.Skip("skip test in short mode")
 	}
-	sess, err := NewSession()
+	sess, err := NewSessionForTesting()
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -234,7 +234,7 @@ func TestSubmitMeasurementGood(t *testing.T) {
 	if testing.Short() {
 		t.Skip("skip test in short mode")
 	}
-	sess, err := NewSession()
+	sess, err := NewSessionForTesting()
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -248,7 +248,7 @@ func TestSubmitCancelContextAfterFirstSubmission(t *testing.T) {
 	if testing.Short() {
 		t.Skip("skip test in short mode")
 	}
-	sess, err := NewSession()
+	sess, err := NewSessionForTesting()
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -267,7 +267,7 @@ func TestSubmitCancelContextAfterFirstSubmission(t *testing.T) {
 }
 
 func TestCheckInSuccess(t *testing.T) {
-	sess, err := NewSession()
+	sess, err := NewSessionForTesting()
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -315,7 +315,7 @@ func TestCheckInSuccess(t *testing.T) {
 }
 
 func TestCheckInLookupLocationFailure(t *testing.T) {
-	sess, err := NewSession()
+	sess, err := NewSessionForTesting()
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -342,7 +342,7 @@ func TestCheckInLookupLocationFailure(t *testing.T) {
 }
 
 func TestCheckInNewProbeServicesFailure(t *testing.T) {
-	sess, err := NewSession()
+	sess, err := NewSessionForTesting()
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -371,7 +371,7 @@ func TestCheckInNewProbeServicesFailure(t *testing.T) {
 }
 
 func TestCheckInCheckInFailure(t *testing.T) {
-	sess, err := NewSession()
+	sess, err := NewSessionForTesting()
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -400,7 +400,7 @@ func TestCheckInCheckInFailure(t *testing.T) {
 }
 
 func TestCheckInNoParams(t *testing.T) {
-	sess, err := NewSession()
+	sess, err := NewSessionForTesting()
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -423,7 +423,7 @@ func TestCheckInNoParams(t *testing.T) {
 }
 
 func TestFetchURLListSuccess(t *testing.T) {
-	sess, err := NewSession()
+	sess, err := NewSessionForTesting()
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -448,7 +448,7 @@ func TestFetchURLListSuccess(t *testing.T) {
 }
 
 func TestFetchURLListWithCC(t *testing.T) {
-	sess, err := NewSession()
+	sess, err := NewSessionForTesting()
 	if err != nil {
 		t.Fatal(err)
 	}

pkg/oonimkall/webconnectivity.go

@@ -0,0 +1,91 @@
+package oonimkall
+
+import (
+	"context"
+	"encoding/json"
+
+	"github.com/ooni/probe-cli/v3/internal/engine/runtimex"
+)
+
+// WebConnectivityConfig contains settings for WebConnectivity.
+type WebConnectivityConfig struct {
+	// Callbacks contains the experiment callbacks. This field is
+	// optional. Leave it empty and we'll use a default set of
+	// callbacks that use the session logger.
+	Callbacks ExperimentCallbacks
+
+	// Input contains the URL to measure. This field must be set
+	// by the user, otherwise the experiment fails.
+	Input string
+}
+
+// WebConnectivityResults contains the results of WebConnectivity.
+type WebConnectivityResults struct {
+	// KibiBytesReceived contains the KiB received.
+	KibiBytesReceived float64
+
+	// KibiBytesSent contains the KiB sent.
+	KibiBytesSent float64
+
+	// Measurement contains the resulting measurement.
+	Measurement string
+}
+
+// webConnectivityRunner is the type that runs
+// the WebConnectivity experiment.
+type webConnectivityRunner struct {
+	sess experimentSession
+}
+
+// run runs the WebConnectivity experiment to completion. Both arguments
+// must be correctly initialized. The return value is either a valid
+// results with a nil error, or nil results with an error.
+func (r *webConnectivityRunner) run(ctx context.Context, config *WebConnectivityConfig) (*WebConnectivityResults, error) {
+	select {
+	case <-ctx.Done():
+		return nil, ctx.Err() // helps with testing
+	default:
+		// fallthrough
+	}
+	// TODO(bassosimone): I suspect most of the code for running
+	// experiments is going to be quite redundant. Autogen?
+	defer r.sess.unlock()
+	r.sess.lock()
+	if err := r.sess.maybeLookupBackends(ctx); err != nil {
+		return nil, err
+	}
+	if err := r.sess.maybeLookupLocation(ctx); err != nil {
+		return nil, err
+	}
+	builder, err := r.sess.newExperimentBuilder("web_connectivity")
+	if err != nil {
+		return nil, err
+	}
+	if config.Callbacks != nil {
+		builder.setCallbacks(config.Callbacks)
+	}
+	exp := builder.newExperiment()
+	measurement, err := exp.MeasureWithContext(ctx, config.Input)
+	if err != nil {
+		return nil, err
+	}
+	data, err := json.Marshal(measurement)
+	runtimex.PanicOnError(err, "json.Marshal should not fail here")
+	return &WebConnectivityResults{
+		KibiBytesReceived: exp.KibiBytesReceived(),
+		KibiBytesSent:     exp.KibiBytesSent(),
+		Measurement:       string(data),
+	}, nil
+}
+
+// WebConnectivity runs the WebConnectivity experiment. Both ctx and config
+// MUST NOT be nil. Returns either an error or the experiment results.
+//
+// This function locks the session until it's done. That is, no other operation
+// can be performed as long as this function is pending.
+//
+// This API is currently experimental. We do not promise that we will bump
+// the major version number when changing it.
+func (sess *Session) WebConnectivity(ctx *Context, config *WebConnectivityConfig) (*WebConnectivityResults, error) {
+	return (&webConnectivityRunner{sess: sess}).run(ctx.ctx, config)
+}

pkg/oonimkall/webconnectivity_integration_test.go

@@ -0,0 +1,28 @@
+package oonimkall_test
+
+import (
+	"testing"
+
+	"github.com/ooni/probe-cli/v3/pkg/oonimkall"
+)
+
+func TestSessionWebConnectivity(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skip test in short mode")
+	}
+	sess, err := NewSessionForTesting()
+	if err != nil {
+		t.Fatal(err)
+	}
+	ctx := sess.NewContext()
+	config := &oonimkall.WebConnectivityConfig{
+		Input: "https://www.google.com",
+	}
+	results, err := sess.WebConnectivity(ctx, config)
+	if err != nil {
+		t.Fatal(err)
+	}
+	t.Logf("bytes received: %f", results.KibiBytesReceived)
+	t.Logf("bytes sent: %f", results.KibiBytesSent)
+	t.Logf("measurement: %d bytes", len(results.Measurement))
+}

pkg/oonimkall/webconnectivity_test.go

@@ -0,0 +1,156 @@
+package oonimkall
+
+import (
+	"context"
+	"encoding/json"
+	"errors"
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+	"github.com/ooni/probe-cli/v3/internal/engine/model"
+)
+
+func TestWebConnectivityRunnerWithMaybeLookupBackendsFailure(t *testing.T) {
+	errMocked := errors.New("mocked error")
+	sess := &FakeExperimentSession{LookupBackendsErr: errMocked}
+	runner := &webConnectivityRunner{sess: sess}
+	ctx := context.Background()
+	config := &WebConnectivityConfig{Input: "https://ooni.org"}
+	out, err := runner.run(ctx, config)
+	if !errors.Is(err, errMocked) {
+		t.Fatal("not the error we expected", err)
+	}
+	if out != nil {
+		t.Fatal("expected nil here")
+	}
+	if sess.LockCount != 1 || sess.UnlockCount != 1 {
+		t.Fatal("invalid locking pattern")
+	}
+}
+
+func TestWebConnectivityRunnerWithMaybeLookupLocationFailure(t *testing.T) {
+	errMocked := errors.New("mocked error")
+	sess := &FakeExperimentSession{LookupLocationErr: errMocked}
+	runner := &webConnectivityRunner{sess: sess}
+	ctx := context.Background()
+	config := &WebConnectivityConfig{Input: "https://ooni.org"}
+	out, err := runner.run(ctx, config)
+	if !errors.Is(err, errMocked) {
+		t.Fatal("not the error we expected", err)
+	}
+	if out != nil {
+		t.Fatal("expected nil here")
+	}
+	if sess.LockCount != 1 || sess.UnlockCount != 1 {
+		t.Fatal("invalid locking pattern")
+	}
+}
+
+func TestWebConnectivityRunnerWithNewExperimentBuilderFailure(t *testing.T) {
+	errMocked := errors.New("mocked error")
+	sess := &FakeExperimentSession{NewExperimentBuilderErr: errMocked}
+	runner := &webConnectivityRunner{sess: sess}
+	ctx := context.Background()
+	config := &WebConnectivityConfig{Input: "https://ooni.org"}
+	out, err := runner.run(ctx, config)
+	if !errors.Is(err, errMocked) {
+		t.Fatal("not the error we expected", err)
+	}
+	if out != nil {
+		t.Fatal("expected nil here")
+	}
+	if sess.LockCount != 1 || sess.UnlockCount != 1 {
+		t.Fatal("invalid locking pattern")
+	}
+}
+
+func TestWebConnectivityRunnerWithMeasureFailure(t *testing.T) {
+	errMocked := errors.New("mocked error")
+	cbs := &FakeExperimentCallbacks{}
+	e := &FakeExperiment{Err: errMocked}
+	eb := &FakeExperimentBuilder{Experiment: e}
+	sess := &FakeExperimentSession{ExperimentBuilder: eb}
+	runner := &webConnectivityRunner{sess: sess}
+	ctx := context.Background()
+	config := &WebConnectivityConfig{
+		Callbacks: cbs,
+		Input:     "https://ooni.org",
+	}
+	out, err := runner.run(ctx, config)
+	if !errors.Is(err, errMocked) {
+		t.Fatal("not the error we expected", err)
+	}
+	if out != nil {
+		t.Fatal("expected nil here")
+	}
+	if sess.LockCount != 1 || sess.UnlockCount != 1 {
+		t.Fatal("invalid locking pattern")
+	}
+	if eb.Callbacks != cbs {
+		t.Fatal("unexpected callbacks")
+	}
+}
+
+func TestWebConnectivityRunnerWithNoError(t *testing.T) {
+	// We create a measurement with non default fields. One of them is
+	// enough to check that we are getting in output the non default
+	// data structure that was preconfigured in the mocks.
+	m := &model.Measurement{Input: "https://ooni.org"}
+	cbs := &FakeExperimentCallbacks{}
+	e := &FakeExperiment{Measurement: m, Sent: 10, Received: 128}
+	eb := &FakeExperimentBuilder{Experiment: e}
+	sess := &FakeExperimentSession{ExperimentBuilder: eb}
+	runner := &webConnectivityRunner{sess: sess}
+	ctx := context.Background()
+	config := &WebConnectivityConfig{
+		Callbacks: cbs,
+		Input:     "https://ooni.org",
+	}
+	out, err := runner.run(ctx, config)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if out == nil {
+		t.Fatal("expected non-nil here")
+	}
+	if sess.LockCount != 1 || sess.UnlockCount != 1 {
+		t.Fatal("invalid locking pattern")
+	}
+	if eb.Callbacks != cbs {
+		t.Fatal("unexpected callbacks")
+	}
+	if out.KibiBytesSent != 10 || out.KibiBytesReceived != 128 {
+		t.Fatal("invalid bytes sent or received")
+	}
+	var mm *model.Measurement
+	mdata := []byte(out.Measurement)
+	if err := json.Unmarshal(mdata, &mm); err != nil {
+		t.Fatal(err)
+	}
+	if diff := cmp.Diff(m, mm); diff != "" {
+		t.Fatal(diff)
+	}
+}
+
+func TestWebConnectivityRunWithCancelledContext(t *testing.T) {
+	sess, err := NewSession(&SessionConfig{
+		AssetsDir:        "../testdata/oonimkall/assets",
+		ProbeServicesURL: "https://ams-pg-test.ooni.org/",
+		SoftwareName:     "oonimkall-test",
+		SoftwareVersion:  "0.1.0",
+		StateDir:         "../testdata/oonimkall/state",
+		TempDir:          "../testdata/",
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+	ctx := sess.NewContext()
+	ctx.Cancel() // kill it immediately
+	out, err := sess.WebConnectivity(ctx, &WebConnectivityConfig{})
+	if !errors.Is(err, context.Canceled) {
+		t.Fatal("not the error we expected", err)
+	}
+	if out != nil {
+		t.Fatal("expected nil output here")
+	}
+}