diff --git a/internal/reflectx/reflectx.go b/internal/reflectx/reflectx.go index ace8b464d..ee26907ae 100644 --- a/internal/reflectx/reflectx.go +++ b/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 { diff --git a/internal/reflectx/reflectx_test.go b/internal/reflectx/reflectx_test.go index c8b74c5bb..f564536aa 100644 --- a/internal/reflectx/reflectx_test.go +++ b/internal/reflectx/reflectx_test.go @@ -7,17 +7,18 @@ 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 @@ -25,6 +26,8 @@ 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 { diff --git a/internal/registry/example.go b/internal/registry/example.go index 4d1db0789..498c09142 100644 --- a/internal/registry/example.go +++ b/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( diff --git a/internal/registry/factory_test.go b/internal/registry/factory_test.go index 20a7ffe74..c92f03157 100644 --- a/internal/registry/factory_test.go +++ b/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) + } + }) } diff --git a/internal/testingx/fakefill.go b/internal/testingx/fakefill.go index fdd7261d0..d756d7476 100644 --- a/internal/testingx/fakefill.go +++ b/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()) diff --git a/internal/testingx/fakefill_test.go b/internal/testingx/fakefill_test.go index 7e3f7f197..4704deb68 100644 --- a/internal/testingx/fakefill_test.go +++ b/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,6 +50,7 @@ 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") @@ -53,7 +58,7 @@ func TestFakeFillAllocatesIntoAMapLikeWithStringKeys(t *testing.T) { } } -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) + } + }) +} diff --git a/pkg/oonimkall/taskmodel.go b/pkg/oonimkall/taskmodel.go index 3bd6ccfee..e030cbc8f 100644 --- a/pkg/oonimkall/taskmodel.go +++ b/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 } //