Merge branch 'issue/2607h' into issue/2607i

internal/reflectx/reflectx.go

@@ -8,7 +8,7 @@ import (
 )
 
 // StructOrStructPtrIsZero returns whether a given struct or struct pointer
-// only contains zero value public fields. This function panic if passed a value
+// only contains zero-value public fields. This function panic if passed a value
 // that is neither a pointer to struct nor a struct. This function panics if
 // passed a nil struct pointer.
 func StructOrStructPtrIsZero(vop any) bool {

internal/reflectx/reflectx_test.go

@@ -7,24 +7,27 @@ import (
 )
 
 type example struct {
-	_    struct{}
-	Age  int64
-	Ch   chan int64
-	F    bool
-	Fmp  map[string]*bool
-	Fp   *bool
-	Fv   []bool
-	Fvp  []*bool
-	Name string
-	Ptr  *int64
-	V    []int64
+	Age   int64
+	Ch    chan int64
+	F     bool
+	Fmp   map[string]*bool
+	Fp    *bool
+	Fv    []bool
+	Fvp   []*bool
+	Name  string
+	Ptr   *int64
+	V     []int64
+	namex string
+	num   int64
 }
 
 var nonzero example
 
 func init() {
 	ff := &testingx.FakeFiller{}
 	ff.Fill(&nonzero)
+	nonzero.namex = "foo"
+	nonzero.num = 123
 }
 
 func TestStructOrStructPtrIsZero(t *testing.T) {
@@ -57,6 +60,20 @@ func TestStructOrStructPtrIsZero(t *testing.T) {
 		name:   "[ptr] with nonzero value",
 		input:  &nonzero,
 		expect: false,
+	}, {
+		name: "[struct] with only private fields being nonzero",
+		input: example{
+			namex: "abc",
+			num:   128,
+		},
+		expect: true,
+	}, {
+		name: "[ptr] with only private fields being nonzero",
+		input: &example{
+			namex: "abc",
+			num:   128,
+		},
+		expect: true,
 	}}
 
 	for _, tc := range cases {

internal/registry/example.go

@@ -14,6 +14,11 @@ import (
 func init() {
 	const canonicalName = "example"
 	AllExperiments[canonicalName] = func() *Factory {
+		// TODO(bassosimone,DecFox): as pointed out by @ainghazal, this experiment
+		// should be the one that people modify to start out new experiments, so it's
+		// kind of suboptimal that it has a constructor with explicit experiment
+		// name to ease writing some tests that ./pkg/oonimkall needs given that no
+		// other experiment ever sets the experiment name externally!
 		return &Factory{
 			build: func(config interface{}) model.ExperimentMeasurer {
 				return example.NewExperimentMeasurer(

internal/registry/factory_test.go

@@ -873,37 +873,72 @@ func (c *customTargetLoader) Load(ctx context.Context) ([]model.ExperimentTarget
 	panic("should not be called")
 }
 
-func TestFactoryCustomTargetLoaderForRicherInput(t *testing.T) {
-	// create factory creating a custom target loader
-	factory := &Factory{
-		build:            nil,
-		canonicalName:    "",
-		config:           &customConfig{},
-		enabledByDefault: false,
-		inputPolicy:      "",
-		interruptible:    false,
-		newLoader: func(config *targetloading.Loader, options any) model.ExperimentTargetLoader {
-			return &customTargetLoader{}
-		},
-	}
+func TestFactoryNewTargetLoader(t *testing.T) {
+	t.Run("with custom target loader", func(t *testing.T) {
+		// create factory creating a custom target loader
+		factory := &Factory{
+			build:            nil,
+			canonicalName:    "",
+			config:           &customConfig{},
+			enabledByDefault: false,
+			inputPolicy:      "",
+			interruptible:    false,
+			newLoader: func(config *targetloading.Loader, options any) model.ExperimentTargetLoader {
+				return &customTargetLoader{}
+			},
+		}
 
-	// create config for creating a new target loader
-	config := &model.ExperimentTargetLoaderConfig{
-		CheckInConfig: &model.OOAPICheckInConfig{ /* nothing */ },
-		Session: &mocks.Session{
-			MockLogger: func() model.Logger {
-				return model.DiscardLogger
+		// create config for creating a new target loader
+		config := &model.ExperimentTargetLoaderConfig{
+			CheckInConfig: &model.OOAPICheckInConfig{ /* nothing */ },
+			Session: &mocks.Session{
+				MockLogger: func() model.Logger {
+					return model.DiscardLogger
+				},
 			},
-		},
-		StaticInputs: []string{},
-		SourceFiles:  []string{},
-	}
+			StaticInputs: []string{},
+			SourceFiles:  []string{},
+		}
 
-	// create the loader
-	loader := factory.NewTargetLoader(config)
+		// create the loader
+		loader := factory.NewTargetLoader(config)
 
-	// make sure the type is the one we expected
-	if _, good := loader.(*customTargetLoader); !good {
-		t.Fatalf("expected a *customTargetLoader, got %T", loader)
-	}
+		// make sure the type is the one we expected
+		if _, good := loader.(*customTargetLoader); !good {
+			t.Fatalf("expected a *customTargetLoader, got %T", loader)
+		}
+	})
+
+	t.Run("with default target loader", func(t *testing.T) {
+		// create factory creating a default target loader
+		factory := &Factory{
+			build:            nil,
+			canonicalName:    "",
+			config:           &customConfig{},
+			enabledByDefault: false,
+			inputPolicy:      "",
+			interruptible:    false,
+			newLoader:        nil, // explicitly nil
+		}
+
+		// create config for creating a new target loader
+		config := &model.ExperimentTargetLoaderConfig{
+			CheckInConfig: &model.OOAPICheckInConfig{ /* nothing */ },
+			Session: &mocks.Session{
+				MockLogger: func() model.Logger {
+					return model.DiscardLogger
+				},
+			},
+			StaticInputs: []string{},
+			SourceFiles:  []string{},
+		}
+
+		// create the loader
+		loader := factory.NewTargetLoader(config)
+
+		// make sure the type is the one we expected
+		if _, good := loader.(*targetloading.Loader); !good {
+			t.Fatalf("expected a *targetloading.Loader, got %T", loader)
+		}
+	})
 }

internal/testingx/fakefill.go

@@ -91,6 +91,13 @@ func (ff *FakeFiller) doFill(v reflect.Value) {
 		// switch to the element
 		v = v.Elem()
 	}
+
+	// make sure we skip initialization of fields we cannot initialize
+	// anyway because they're private or immutable
+	if !v.CanSet() {
+		return
+	}
+
 	switch v.Type().Kind() {
 	case reflect.String:
 		v.SetString(ff.getRandomString())

internal/testingx/fakefill_test.go

@@ -3,6 +3,8 @@ package testingx
 import (
 	"testing"
 	"time"
+
+	"github.com/google/go-cmp/cmp"
 )
 
 // exampleStructure is an example structure we fill.
@@ -25,6 +27,7 @@ func TestFakeFillWorksWithCustomTime(t *testing.T) {
 	if req == nil {
 		t.Fatal("we expected non nil here")
 	}
+	t.Log(req)
 }
 
 func TestFakeFillAllocatesIntoAPointerToPointer(t *testing.T) {
@@ -34,6 +37,7 @@ func TestFakeFillAllocatesIntoAPointerToPointer(t *testing.T) {
 	if req == nil {
 		t.Fatal("we expected non nil here")
 	}
+	t.Log(req)
 }
 
 func TestFakeFillAllocatesIntoAMapLikeWithStringKeys(t *testing.T) {
@@ -46,14 +50,15 @@ func TestFakeFillAllocatesIntoAMapLikeWithStringKeys(t *testing.T) {
 	if len(resp) < 1 {
 		t.Fatal("we expected some data here")
 	}
+	t.Log(resp)
 	for _, value := range resp {
 		if value == nil {
 			t.Fatal("expected non-nil here")
 		}
 	}
 }
 
-func TestFakeFillAllocatesIntoAMapLikeWithNonStringKeys(t *testing.T) {
+func TestFakeFillPanicsWithMapsWithNonStringKeys(t *testing.T) {
 	var panicmsg string
 	func() {
 		defer func() {
@@ -83,9 +88,57 @@ func TestFakeFillAllocatesIntoASlice(t *testing.T) {
 	if len(*resp) < 1 {
 		t.Fatal("we expected some data here")
 	}
+	t.Log(resp)
 	for _, entry := range *resp {
 		if entry == nil {
 			t.Fatal("expected non-nil here")
 		}
 	}
 }
+
+func TestFakeFillSkipsPrivateTypes(t *testing.T) {
+	t.Run("with private struct fields", func(t *testing.T) {
+		// define structure with mixed private and public fields
+		type employee struct {
+			ID   int64
+			age  int64
+			name string
+		}
+
+		// create empty employee
+		var person employee
+
+		// fake-fill the employee
+		ff := &FakeFiller{}
+		ff.Fill(&person)
+
+		// define what we expect to see
+		expect := employee{
+			ID:   person.ID,
+			age:  0,
+			name: "",
+		}
+
+		// make sure we've got what we expected
+		//
+		// Note: we cannot use cmp.Diff directly because it cannot
+		// access private fields, so we need to write manual comparison
+		if person != expect {
+			t.Fatal("expected", expect, "got", person)
+		}
+	})
+
+	t.Run("make sure we cannot initialize a non-addressable type", func(t *testing.T) {
+		// create a zero struct
+		shouldRemainZero := exampleStructure{}
+
+		// attempt to fake fill w/o taking the address
+		ff := &FakeFiller{}
+		ff.Fill(shouldRemainZero)
+
+		// make sure it's still zero
+		if diff := cmp.Diff(exampleStructure{}, shouldRemainZero); diff != "" {
+			t.Fatal(diff)
+		}
+	})
+}

pkg/oonimkall/taskmodel.go

@@ -162,6 +162,12 @@ type event struct {
 
 // taskSession abstracts a OONI session.
 type taskSession interface {
+	// A session should be used by an experiment.
+	model.ExperimentSession
+
+	// A session should be used when loading targets.
+	model.ExperimentTargetLoaderSession
+
 	// A session can be closed.
 	io.Closer
 
@@ -196,12 +202,6 @@ type taskSession interface {
 	// ResolverNetworkName must be called after MaybeLookupLocationContext
 	// and returns the resolved resolver's network name.
 	ResolverNetworkName() string
-
-	// A session should be used by an experiment.
-	model.ExperimentSession
-
-	// A session should be used when loading targets.
-	model.ExperimentTargetLoaderSession
 }
 
 //