From 960434e8ddc923c4f1d22fdb53f76406ddf8362e Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 6 Jun 2024 12:04:35 +0200 Subject: [PATCH 1/5] refactor: move engine.InputLoader to inputloading This commit moves the engine.InputLoader type to a new package called inputloading and adapts the naming to avoid stuttering. The reason for moving InputLoader is that the engine package depends on registry, and, per the plan described by the first richer input PR, https://github.com/ooni/probe-cli/pull/1615, we want to move input loading directly inside the registry. To this end, we need to move the input loading feature outside of engine to avoid creating import loops. We keep an integration test inside the engine package because it seems such an integration test was checking both engine and the InputLoader together. We may further refactor this test in the future. Part of https://github.com/ooni/probe-cli/pull/1612 --- cmd/ooniprobe/internal/nettests/dnscheck.go | 4 +- .../internal/nettests/stunreachability.go | 4 +- .../internal/nettests/web_connectivity.go | 4 +- ...est.go => inputloader_integration_test.go} | 9 +- .../inputloading.go} | 54 ++++----- .../inputloading_test.go} | 104 ++++++++++-------- .../testdata/inputloader1.txt | 0 .../testdata/inputloader2.txt | 0 .../testdata/inputloader3.txt | 0 internal/oonirun/experiment.go | 4 +- internal/oonirun/session.go | 4 +- pkg/oonimkall/taskrunner.go | 3 +- pkg/oonimkall/taskrunner_test.go | 4 +- 13 files changed, 110 insertions(+), 84 deletions(-) rename internal/engine/{inputloader_network_test.go => inputloader_integration_test.go} (63%) rename internal/{engine/inputloader.go => inputloading/inputloading.go} (89%) rename internal/{engine/inputloader_test.go => inputloading/inputloading_test.go} (93%) rename internal/{engine => inputloading}/testdata/inputloader1.txt (100%) rename internal/{engine => inputloading}/testdata/inputloader2.txt (100%) rename internal/{engine => inputloading}/testdata/inputloader3.txt (100%) diff --git a/cmd/ooniprobe/internal/nettests/dnscheck.go b/cmd/ooniprobe/internal/nettests/dnscheck.go index 4fe7de5fbd..44c7f86e2a 100644 --- a/cmd/ooniprobe/internal/nettests/dnscheck.go +++ b/cmd/ooniprobe/internal/nettests/dnscheck.go @@ -3,7 +3,7 @@ package nettests import ( "context" - engine "github.com/ooni/probe-cli/v3/internal/engine" + "github.com/ooni/probe-cli/v3/internal/inputloading" "github.com/ooni/probe-cli/v3/internal/model" ) @@ -11,7 +11,7 @@ import ( type DNSCheck struct{} func (n DNSCheck) lookupURLs(ctl *Controller) ([]model.ExperimentTarget, error) { - inputloader := &engine.InputLoader{ + inputloader := &inputloading.Loader{ CheckInConfig: &model.OOAPICheckInConfig{ // not needed because we have default static input in the engine }, diff --git a/cmd/ooniprobe/internal/nettests/stunreachability.go b/cmd/ooniprobe/internal/nettests/stunreachability.go index c09ae23cb2..f77fe3ea84 100644 --- a/cmd/ooniprobe/internal/nettests/stunreachability.go +++ b/cmd/ooniprobe/internal/nettests/stunreachability.go @@ -3,7 +3,7 @@ package nettests import ( "context" - engine "github.com/ooni/probe-cli/v3/internal/engine" + "github.com/ooni/probe-cli/v3/internal/inputloading" "github.com/ooni/probe-cli/v3/internal/model" ) @@ -11,7 +11,7 @@ import ( type STUNReachability struct{} func (n STUNReachability) lookupURLs(ctl *Controller) ([]model.ExperimentTarget, error) { - inputloader := &engine.InputLoader{ + inputloader := &inputloading.Loader{ CheckInConfig: &model.OOAPICheckInConfig{ // not needed because we have default static input in the engine }, diff --git a/cmd/ooniprobe/internal/nettests/web_connectivity.go b/cmd/ooniprobe/internal/nettests/web_connectivity.go index 5d2fc939ea..9131e811a9 100644 --- a/cmd/ooniprobe/internal/nettests/web_connectivity.go +++ b/cmd/ooniprobe/internal/nettests/web_connectivity.go @@ -4,12 +4,12 @@ import ( "context" "github.com/apex/log" - engine "github.com/ooni/probe-cli/v3/internal/engine" + "github.com/ooni/probe-cli/v3/internal/inputloading" "github.com/ooni/probe-cli/v3/internal/model" ) func (n WebConnectivity) lookupURLs(ctl *Controller, categories []string) ([]model.ExperimentTarget, error) { - inputloader := &engine.InputLoader{ + inputloader := &inputloading.Loader{ CheckInConfig: &model.OOAPICheckInConfig{ // Setting Charging and OnWiFi to true causes the CheckIn // API to return to us as much URL as possible with the diff --git a/internal/engine/inputloader_network_test.go b/internal/engine/inputloader_integration_test.go similarity index 63% rename from internal/engine/inputloader_network_test.go rename to internal/engine/inputloader_integration_test.go index 040e85ac20..a5a0bad598 100644 --- a/internal/engine/inputloader_network_test.go +++ b/internal/engine/inputloader_integration_test.go @@ -6,10 +6,17 @@ import ( "github.com/apex/log" "github.com/ooni/probe-cli/v3/internal/engine" + "github.com/ooni/probe-cli/v3/internal/inputloading" "github.com/ooni/probe-cli/v3/internal/kvstore" "github.com/ooni/probe-cli/v3/internal/model" ) +// This historical integration test ensures that we're able to fetch URLs from +// the dev infrastructure. We say this test's historical because the inputloading.Loader +// belonged to the engine package before we introduced richer input. It kind of feels +// good to keep this integration test here since we want to use a real session and a real +// Loader and double check whether we can get inputs. In a more distant future it would +// kind of make sense to have a broader package with this kind of integration tests. func TestInputLoaderInputOrQueryBackendWithNoInput(t *testing.T) { if testing.Short() { t.Skip("skip test in short mode") @@ -29,7 +36,7 @@ func TestInputLoaderInputOrQueryBackendWithNoInput(t *testing.T) { t.Fatal(err) } defer sess.Close() - il := &engine.InputLoader{ + il := &inputloading.Loader{ InputPolicy: model.InputOrQueryBackend, Session: sess, } diff --git a/internal/engine/inputloader.go b/internal/inputloading/inputloading.go similarity index 89% rename from internal/engine/inputloader.go rename to internal/inputloading/inputloading.go index 06c31b5d57..3b339cceaf 100644 --- a/internal/engine/inputloader.go +++ b/internal/inputloading/inputloading.go @@ -1,4 +1,5 @@ -package engine +// Package inputloading contains common code to load input. +package inputloading import ( "bufio" @@ -16,7 +17,7 @@ import ( "github.com/ooni/probe-cli/v3/internal/stuninput" ) -// These errors are returned by the InputLoader. +// These errors are returned by the [*Loader]. var ( ErrNoURLsReturned = errors.New("no URLs returned") ErrDetectedEmptyFile = errors.New("file did not contain any input") @@ -25,21 +26,20 @@ var ( ErrNoStaticInput = errors.New("no static input for this experiment") ) -// InputLoaderSession is the session according to an InputLoader. We -// introduce this abstraction because it helps us with testing. -type InputLoaderSession interface { +// Session is the session according to a [*Loader]. +type Session interface { CheckIn(ctx context.Context, config *model.OOAPICheckInConfig) (*model.OOAPICheckInResult, error) FetchOpenVPNConfig(ctx context.Context, provider, cc string) (*model.OOAPIVPNProviderConfig, error) } -// InputLoaderLogger is the logger according to an InputLoader. -type InputLoaderLogger interface { +// Logger is the [model.Logger] according to a [*Loader]. +type Logger interface { // Warnf formats and emits a warning message. Warnf(format string, v ...interface{}) } -// InputLoader loads input according to the specified policy +// Loader loads input according to the specified policy // either from command line and input files or from OONI services. The // behaviour depends on the input policy as described below. // @@ -74,7 +74,7 @@ type InputLoaderLogger interface { // // We gather input from StaticInput and SourceFiles. If there is // input, we return it. Otherwise, we return an error. -type InputLoader struct { +type Loader struct { // CheckInConfig contains options for the CheckIn API. If // not set, then we'll create a default config. If set but // there are fields inside it that are not set, then we @@ -91,14 +91,14 @@ type InputLoader struct { // this field. InputPolicy model.InputPolicy - // Logger is the optional logger that the InputLoader + // Logger is the optional logger that the [*Loader] // should be using. If not set, we will use the default // logger of github.com/apex/log. - Logger InputLoaderLogger + Logger Logger // Session is the current measurement session. You // MUST fill in this field. - Session InputLoaderSession + Session Session // StaticInputs contains optional input to be added // to the resulting input list if possible. @@ -113,7 +113,7 @@ type InputLoader struct { // Load attempts to load input using the specified input loader. We will // return a list of URLs because this is the only input we support. -func (il *InputLoader) Load(ctx context.Context) ([]model.ExperimentTarget, error) { +func (il *Loader) Load(ctx context.Context) ([]model.ExperimentTarget, error) { switch il.InputPolicy { case model.InputOptional: return il.loadOptional() @@ -129,7 +129,7 @@ func (il *InputLoader) Load(ctx context.Context) ([]model.ExperimentTarget, erro } // loadNone implements the InputNone policy. -func (il *InputLoader) loadNone() ([]model.ExperimentTarget, error) { +func (il *Loader) loadNone() ([]model.ExperimentTarget, error) { if len(il.StaticInputs) > 0 || len(il.SourceFiles) > 0 { return nil, ErrNoInputExpected } @@ -140,7 +140,7 @@ func (il *InputLoader) loadNone() ([]model.ExperimentTarget, error) { } // loadOptional implements the InputOptional policy. -func (il *InputLoader) loadOptional() ([]model.ExperimentTarget, error) { +func (il *Loader) loadOptional() ([]model.ExperimentTarget, error) { inputs, err := il.loadLocal() if err == nil && len(inputs) <= 0 { // Implementation note: the convention for input-less experiments is that @@ -151,7 +151,7 @@ func (il *InputLoader) loadOptional() ([]model.ExperimentTarget, error) { } // loadStrictlyRequired implements the InputStrictlyRequired policy. -func (il *InputLoader) loadStrictlyRequired(_ context.Context) ([]model.ExperimentTarget, error) { +func (il *Loader) loadStrictlyRequired(_ context.Context) ([]model.ExperimentTarget, error) { inputs, err := il.loadLocal() if err != nil || len(inputs) > 0 { return inputs, err @@ -160,7 +160,7 @@ func (il *InputLoader) loadStrictlyRequired(_ context.Context) ([]model.Experime } // loadOrQueryBackend implements the InputOrQueryBackend policy. -func (il *InputLoader) loadOrQueryBackend(ctx context.Context) ([]model.ExperimentTarget, error) { +func (il *Loader) loadOrQueryBackend(ctx context.Context) ([]model.ExperimentTarget, error) { inputs, err := il.loadLocal() if err != nil || len(inputs) > 0 { return inputs, err @@ -247,7 +247,7 @@ func staticInputForExperiment(name string) ([]model.ExperimentTarget, error) { } // loadOrStaticDefault implements the InputOrStaticDefault policy. -func (il *InputLoader) loadOrStaticDefault(_ context.Context) ([]model.ExperimentTarget, error) { +func (il *Loader) loadOrStaticDefault(_ context.Context) ([]model.ExperimentTarget, error) { inputs, err := il.loadLocal() if err != nil || len(inputs) > 0 { return inputs, err @@ -256,7 +256,7 @@ func (il *InputLoader) loadOrStaticDefault(_ context.Context) ([]model.Experimen } // loadLocal loads inputs from StaticInputs and SourceFiles. -func (il *InputLoader) loadLocal() ([]model.ExperimentTarget, error) { +func (il *Loader) loadLocal() ([]model.ExperimentTarget, error) { inputs := []model.ExperimentTarget{} for _, input := range il.StaticInputs { inputs = append(inputs, model.NewOOAPIURLInfoWithDefaultCategoryAndCountry(input)) @@ -280,7 +280,7 @@ type inputLoaderOpenFn func(filepath string) (fs.File, error) // readfile reads inputs from the specified file. The open argument should be // compatible with stdlib's fs.Open and helps us with unit testing. -func (il *InputLoader) readfile(filepath string, open inputLoaderOpenFn) ([]model.ExperimentTarget, error) { +func (il *Loader) readfile(filepath string, open inputLoaderOpenFn) ([]model.ExperimentTarget, error) { inputs := []model.ExperimentTarget{} filep, err := open(filepath) if err != nil { @@ -304,7 +304,7 @@ func (il *InputLoader) readfile(filepath string, open inputLoaderOpenFn) ([]mode } // loadRemote loads inputs from a remote source. -func (il *InputLoader) loadRemote(ctx context.Context) ([]model.ExperimentTarget, error) { +func (il *Loader) loadRemote(ctx context.Context) ([]model.ExperimentTarget, error) { switch registry.CanonicalizeExperimentName(il.ExperimentName) { case "openvpn": // TODO(ainghazal): given the semantics of the current API call, in an ideal world we'd need to pass @@ -319,7 +319,7 @@ func (il *InputLoader) loadRemote(ctx context.Context) ([]model.ExperimentTarget } // loadRemoteWebConnectivity loads webconnectivity inputs from a remote source. -func (il *InputLoader) loadRemoteWebConnectivity(ctx context.Context) ([]model.ExperimentTarget, error) { +func (il *Loader) loadRemoteWebConnectivity(ctx context.Context) ([]model.ExperimentTarget, error) { config := il.CheckInConfig if config == nil { // Note: Session.CheckIn documentation says it will fill in @@ -356,7 +356,7 @@ func inputLoaderModelOOAPIURLInfoToModelExperimentTarget( } // loadRemoteOpenVPN loads openvpn inputs from a remote source. -func (il *InputLoader) loadRemoteOpenVPN(ctx context.Context) ([]model.ExperimentTarget, error) { +func (il *Loader) loadRemoteOpenVPN(ctx context.Context) ([]model.ExperimentTarget, error) { // VPN Inputs do not match exactly the semantics expected from [model.OOAPIURLInfo], // since OOAPIURLInfo is oriented towards webconnectivity, // but we force VPN targets in the URL and ignore all the other fields. @@ -393,7 +393,7 @@ func (il *InputLoader) loadRemoteOpenVPN(ctx context.Context) ([]model.Experimen // checkIn executes the check-in and filters the returned URLs to exclude // the URLs that are not part of the requested categories. This is done for // robustness, just in case we or the API do something wrong. -func (il *InputLoader) checkIn( +func (il *Loader) checkIn( ctx context.Context, config *model.OOAPICheckInConfig) (*model.OOAPICheckInResultNettests, error) { reply, err := il.Session.CheckIn(ctx, config) if err != nil { @@ -409,7 +409,7 @@ func (il *InputLoader) checkIn( } // fetchOpenVPNConfig fetches vpn information for the configured providers -func (il *InputLoader) fetchOpenVPNConfig(ctx context.Context, provider string) (*model.OOAPIVPNProviderConfig, error) { +func (il *Loader) fetchOpenVPNConfig(ctx context.Context, provider string) (*model.OOAPIVPNProviderConfig, error) { reply, err := il.Session.FetchOpenVPNConfig(ctx, provider, "XX") if err != nil { return nil, err @@ -420,7 +420,7 @@ func (il *InputLoader) fetchOpenVPNConfig(ctx context.Context, provider string) // preventMistakes makes the code more robust with respect to any possible // integration issue where the backend returns to us URLs that don't // belong to the category codes we requested. -func (il *InputLoader) preventMistakes(input []model.OOAPIURLInfo, categories []string) (output []model.OOAPIURLInfo) { +func (il *Loader) preventMistakes(input []model.OOAPIURLInfo, categories []string) (output []model.OOAPIURLInfo) { if len(categories) <= 0 { return input } @@ -442,7 +442,7 @@ func (il *InputLoader) preventMistakes(input []model.OOAPIURLInfo, categories [] } // logger returns the configured logger or apex/log's default. -func (il *InputLoader) logger() InputLoaderLogger { +func (il *Loader) logger() Logger { if il.Logger != nil { return il.Logger } diff --git a/internal/engine/inputloader_test.go b/internal/inputloading/inputloading_test.go similarity index 93% rename from internal/engine/inputloader_test.go rename to internal/inputloading/inputloading_test.go index 70f3f86cfd..bcb77f4a56 100644 --- a/internal/engine/inputloader_test.go +++ b/internal/inputloading/inputloading_test.go @@ -1,4 +1,4 @@ -package engine +package inputloading import ( "context" @@ -11,15 +11,14 @@ import ( "testing" "time" - "github.com/apex/log" "github.com/google/go-cmp/cmp" - "github.com/ooni/probe-cli/v3/internal/kvstore" + "github.com/ooni/probe-cli/v3/internal/mocks" "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/runtimex" ) func TestInputLoaderInputNoneWithStaticInputs(t *testing.T) { - il := &InputLoader{ + il := &Loader{ StaticInputs: []string{"https://www.google.com/"}, InputPolicy: model.InputNone, } @@ -34,7 +33,7 @@ func TestInputLoaderInputNoneWithStaticInputs(t *testing.T) { } func TestInputLoaderInputNoneWithFilesInputs(t *testing.T) { - il := &InputLoader{ + il := &Loader{ SourceFiles: []string{ "testdata/inputloader1.txt", "testdata/inputloader2.txt", @@ -52,7 +51,7 @@ func TestInputLoaderInputNoneWithFilesInputs(t *testing.T) { } func TestInputLoaderInputNoneWithBothInputs(t *testing.T) { - il := &InputLoader{ + il := &Loader{ StaticInputs: []string{"https://www.google.com/"}, SourceFiles: []string{ "testdata/inputloader1.txt", @@ -71,7 +70,7 @@ func TestInputLoaderInputNoneWithBothInputs(t *testing.T) { } func TestInputLoaderInputNoneWithNoInput(t *testing.T) { - il := &InputLoader{ + il := &Loader{ InputPolicy: model.InputNone, } ctx := context.Background() @@ -85,7 +84,7 @@ func TestInputLoaderInputNoneWithNoInput(t *testing.T) { } func TestInputLoaderInputOptionalWithNoInput(t *testing.T) { - il := &InputLoader{ + il := &Loader{ InputPolicy: model.InputOptional, } ctx := context.Background() @@ -99,7 +98,7 @@ func TestInputLoaderInputOptionalWithNoInput(t *testing.T) { } func TestInputLoaderInputOptionalWithInput(t *testing.T) { - il := &InputLoader{ + il := &Loader{ StaticInputs: []string{"https://www.google.com/"}, SourceFiles: []string{ "testdata/inputloader1.txt", @@ -148,7 +147,7 @@ func TestInputLoaderInputOptionalWithInput(t *testing.T) { } func TestInputLoaderInputOptionalNonexistentFile(t *testing.T) { - il := &InputLoader{ + il := &Loader{ StaticInputs: []string{"https://www.google.com/"}, SourceFiles: []string{ "testdata/inputloader1.txt", @@ -168,7 +167,7 @@ func TestInputLoaderInputOptionalNonexistentFile(t *testing.T) { } func TestInputLoaderInputStrictlyRequiredWithInput(t *testing.T) { - il := &InputLoader{ + il := &Loader{ StaticInputs: []string{"https://www.google.com/"}, SourceFiles: []string{ "testdata/inputloader1.txt", @@ -217,7 +216,7 @@ func TestInputLoaderInputStrictlyRequiredWithInput(t *testing.T) { } func TestInputLoaderInputStrictlyRequiredWithoutInput(t *testing.T) { - il := &InputLoader{ + il := &Loader{ InputPolicy: model.InputStrictlyRequired, } ctx := context.Background() @@ -231,7 +230,7 @@ func TestInputLoaderInputStrictlyRequiredWithoutInput(t *testing.T) { } func TestInputLoaderInputStrictlyRequiredWithEmptyFile(t *testing.T) { - il := &InputLoader{ + il := &Loader{ InputPolicy: model.InputStrictlyRequired, SourceFiles: []string{ "testdata/inputloader1.txt", @@ -250,7 +249,7 @@ func TestInputLoaderInputStrictlyRequiredWithEmptyFile(t *testing.T) { } func TestInputLoaderInputOrStaticDefaultWithInput(t *testing.T) { - il := &InputLoader{ + il := &Loader{ ExperimentName: "dnscheck", StaticInputs: []string{"https://www.google.com/"}, SourceFiles: []string{ @@ -300,7 +299,7 @@ func TestInputLoaderInputOrStaticDefaultWithInput(t *testing.T) { } func TestInputLoaderInputOrStaticDefaultWithEmptyFile(t *testing.T) { - il := &InputLoader{ + il := &Loader{ ExperimentName: "dnscheck", InputPolicy: model.InputOrStaticDefault, SourceFiles: []string{ @@ -320,7 +319,7 @@ func TestInputLoaderInputOrStaticDefaultWithEmptyFile(t *testing.T) { } func TestInputLoaderInputOrStaticDefaultWithoutInputDNSCheck(t *testing.T) { - il := &InputLoader{ + il := &Loader{ ExperimentName: "dnscheck", InputPolicy: model.InputOrStaticDefault, } @@ -347,7 +346,7 @@ func TestInputLoaderInputOrStaticDefaultWithoutInputDNSCheck(t *testing.T) { } func TestInputLoaderInputOrStaticDefaultWithoutInputStunReachability(t *testing.T) { - il := &InputLoader{ + il := &Loader{ ExperimentName: "stunreachability", InputPolicy: model.InputOrStaticDefault, } @@ -383,7 +382,7 @@ func TestStaticBareInputForExperimentWorksWithNonCanonicalNames(t *testing.T) { } func TestInputLoaderInputOrStaticDefaultWithoutInputOtherName(t *testing.T) { - il := &InputLoader{ + il := &Loader{ ExperimentName: "xx", InputPolicy: model.InputOrStaticDefault, } @@ -398,7 +397,7 @@ func TestInputLoaderInputOrStaticDefaultWithoutInputOtherName(t *testing.T) { } func TestInputLoaderInputOrQueryBackendWithInput(t *testing.T) { - il := &InputLoader{ + il := &Loader{ StaticInputs: []string{"https://www.google.com/"}, SourceFiles: []string{ "testdata/inputloader1.txt", @@ -447,18 +446,15 @@ func TestInputLoaderInputOrQueryBackendWithInput(t *testing.T) { } func TestInputLoaderInputOrQueryBackendWithNoInputAndCancelledContext(t *testing.T) { - sess, err := NewSession(context.Background(), SessionConfig{ - KVStore: &kvstore.Memory{}, - Logger: log.Log, - SoftwareName: "miniooni", - SoftwareVersion: "0.1.0-dev", - TempDir: "testdata", - }) - if err != nil { - t.Fatal(err) + sess := &mocks.Session{ + MockCheckIn: func(ctx context.Context, config *model.OOAPICheckInConfig) (*model.OOAPICheckInResult, error) { + if err := ctx.Err(); err != nil { + return nil, err + } + panic("should not arrive here") + }, } - defer sess.Close() - il := &InputLoader{ + il := &Loader{ InputPolicy: model.InputOrQueryBackend, Session: sess, } @@ -474,7 +470,7 @@ func TestInputLoaderInputOrQueryBackendWithNoInputAndCancelledContext(t *testing } func TestInputLoaderInputOrQueryBackendWithEmptyFile(t *testing.T) { - il := &InputLoader{ + il := &Loader{ InputPolicy: model.InputOrQueryBackend, SourceFiles: []string{ "testdata/inputloader1.txt", @@ -513,7 +509,7 @@ func (InputLoaderBrokenFile) Close() error { } func TestInputLoaderReadfileScannerFailure(t *testing.T) { - il := &InputLoader{} + il := &Loader{} out, err := il.readfile("", InputLoaderBrokenFS{}.Open) if !errors.Is(err, syscall.EFAULT) { t.Fatal("not the error we expected") @@ -556,7 +552,7 @@ func (sess *InputLoaderMockableSession) FetchOpenVPNConfig( } func TestInputLoaderCheckInFailure(t *testing.T) { - il := &InputLoader{ + il := &Loader{ Session: &InputLoaderMockableSession{ Error: io.EOF, }, @@ -571,7 +567,7 @@ func TestInputLoaderCheckInFailure(t *testing.T) { } func TestInputLoaderCheckInSuccessWithNilWebConnectivity(t *testing.T) { - il := &InputLoader{ + il := &Loader{ Session: &InputLoaderMockableSession{ Output: &model.OOAPICheckInResult{ Tests: model.OOAPICheckInResultNettests{}, @@ -588,7 +584,7 @@ func TestInputLoaderCheckInSuccessWithNilWebConnectivity(t *testing.T) { } func TestInputLoaderCheckInSuccessWithNoURLs(t *testing.T) { - il := &InputLoader{ + il := &Loader{ Session: &InputLoaderMockableSession{ Output: &model.OOAPICheckInResult{ Tests: model.OOAPICheckInResultNettests{ @@ -619,7 +615,7 @@ func TestInputLoaderCheckInSuccessWithSomeURLs(t *testing.T) { } inputs := []model.OOAPIURLInfo{inputs0, inputs1} expect := []model.ExperimentTarget{&inputs0, &inputs1} - il := &InputLoader{ + il := &Loader{ Session: &InputLoaderMockableSession{ Output: &model.OOAPICheckInResult{ Tests: model.OOAPICheckInResultNettests{ @@ -640,7 +636,7 @@ func TestInputLoaderCheckInSuccessWithSomeURLs(t *testing.T) { } func TestInputLoaderOpenVPNSuccessWithNoInput(t *testing.T) { - il := &InputLoader{ + il := &Loader{ ExperimentName: "openvpn", InputPolicy: model.InputOrQueryBackend, Session: &InputLoaderMockableSession{ @@ -661,7 +657,7 @@ func TestInputLoaderOpenVPNSuccessWithNoInput(t *testing.T) { } func TestInputLoaderOpenVPNSuccessWithNoInputAndAPICall(t *testing.T) { - il := &InputLoader{ + il := &Loader{ ExperimentName: "openvpn", InputPolicy: model.InputOrQueryBackend, Session: &InputLoaderMockableSession{ @@ -686,7 +682,7 @@ func TestInputLoaderOpenVPNSuccessWithNoInputAndAPICall(t *testing.T) { func TestInputLoaderOpenVPNWithAPIFailureAndFallback(t *testing.T) { expected := errors.New("mocked API error") - il := &InputLoader{ + il := &Loader{ ExperimentName: "openvpn", InputPolicy: model.InputOrQueryBackend, Session: &InputLoaderMockableSession{ @@ -702,6 +698,28 @@ func TestInputLoaderOpenVPNWithAPIFailureAndFallback(t *testing.T) { } } +func TestInputLoaderOpenVPNWithNoReturnedURLs(t *testing.T) { + il := &Loader{ + ExperimentName: "openvpn", + InputPolicy: model.InputOrQueryBackend, + Session: &InputLoaderMockableSession{ + FetchOpenVPNConfigOutput: &model.OOAPIVPNProviderConfig{ + Provider: "riseupvpn", + Config: &model.OOAPIVPNConfig{}, + Inputs: []string{}, + DateUpdated: time.Time{}, + }, + }, + } + out, err := il.loadRemote(context.Background()) + if !errors.Is(err, ErrNoURLsReturned) { + t.Fatal("unexpected a error") + } + if len(out) != 0 { + t.Fatal("we expected no fallback URLs") + } +} + func TestPreventMistakesWithCategories(t *testing.T) { input := []model.OOAPIURLInfo{{ CategoryCode: "NEWS", @@ -725,7 +743,7 @@ func TestPreventMistakesWithCategories(t *testing.T) { URL: "https://addons.mozilla.org/", CountryCode: "XX", }} - il := &InputLoader{} + il := &Loader{} output := il.preventMistakes(input, []string{"NEWS", "FILE"}) if diff := cmp.Diff(desired, output); diff != "" { t.Fatal(diff) @@ -746,7 +764,7 @@ func TestPreventMistakesWithoutCategoriesAndNil(t *testing.T) { URL: "https://addons.mozilla.org/", CountryCode: "XX", }} - il := &InputLoader{} + il := &Loader{} output := il.preventMistakes(input, nil) if diff := cmp.Diff(input, output); diff != "" { t.Fatal(diff) @@ -767,7 +785,7 @@ func TestPreventMistakesWithoutCategoriesAndEmpty(t *testing.T) { URL: "https://addons.mozilla.org/", CountryCode: "XX", }} - il := &InputLoader{} + il := &Loader{} output := il.preventMistakes(input, []string{}) if diff := cmp.Diff(input, output); diff != "" { t.Fatal(diff) @@ -782,7 +800,7 @@ func (ilfl *InputLoaderFakeLogger) Warnf(format string, v ...interface{}) {} func TestInputLoaderLoggerWorksAsIntended(t *testing.T) { logger := &InputLoaderFakeLogger{} - inputLoader := &InputLoader{Logger: logger} + inputLoader := &Loader{Logger: logger} out := inputLoader.logger() if out != logger { t.Fatal("logger not working as intended") diff --git a/internal/engine/testdata/inputloader1.txt b/internal/inputloading/testdata/inputloader1.txt similarity index 100% rename from internal/engine/testdata/inputloader1.txt rename to internal/inputloading/testdata/inputloader1.txt diff --git a/internal/engine/testdata/inputloader2.txt b/internal/inputloading/testdata/inputloader2.txt similarity index 100% rename from internal/engine/testdata/inputloader2.txt rename to internal/inputloading/testdata/inputloader2.txt diff --git a/internal/engine/testdata/inputloader3.txt b/internal/inputloading/testdata/inputloader3.txt similarity index 100% rename from internal/engine/testdata/inputloader3.txt rename to internal/inputloading/testdata/inputloader3.txt diff --git a/internal/oonirun/experiment.go b/internal/oonirun/experiment.go index 7558d71fc8..01eada8abb 100644 --- a/internal/oonirun/experiment.go +++ b/internal/oonirun/experiment.go @@ -12,8 +12,8 @@ import ( "sync/atomic" "time" - "github.com/ooni/probe-cli/v3/internal/engine" "github.com/ooni/probe-cli/v3/internal/humanize" + "github.com/ooni/probe-cli/v3/internal/inputloading" "github.com/ooni/probe-cli/v3/internal/model" ) @@ -200,7 +200,7 @@ func (ed *Experiment) newInputLoader(inputPolicy model.InputPolicy) inputLoader if ed.newInputLoaderFn != nil { return ed.newInputLoaderFn(inputPolicy) } - return &engine.InputLoader{ + return &inputloading.Loader{ CheckInConfig: &model.OOAPICheckInConfig{ RunType: model.RunTypeManual, OnWiFi: true, // meaning: not on 4G diff --git a/internal/oonirun/session.go b/internal/oonirun/session.go index 64566dbeca..4b55da17de 100644 --- a/internal/oonirun/session.go +++ b/internal/oonirun/session.go @@ -10,14 +10,14 @@ package oonirun // import ( - "github.com/ooni/probe-cli/v3/internal/engine" + "github.com/ooni/probe-cli/v3/internal/inputloading" "github.com/ooni/probe-cli/v3/internal/model" ) // Session is the definition of Session used by this package. type Session interface { // A Session is also an InputLoaderSession. - engine.InputLoaderSession + inputloading.Session // A Session is also a SubmitterSession. SubmitterSession diff --git a/pkg/oonimkall/taskrunner.go b/pkg/oonimkall/taskrunner.go index 5702ed6499..78f2cec6a1 100644 --- a/pkg/oonimkall/taskrunner.go +++ b/pkg/oonimkall/taskrunner.go @@ -8,6 +8,7 @@ import ( "time" "github.com/ooni/probe-cli/v3/internal/engine" + "github.com/ooni/probe-cli/v3/internal/inputloading" "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/runtimex" ) @@ -191,7 +192,7 @@ func (r *runnerForTask) Run(rootCtx context.Context) { } case model.InputOrStaticDefault: if len(r.settings.Inputs) <= 0 { - inputs, err := engine.StaticBareInputForExperiment(r.settings.Name) + inputs, err := inputloading.StaticBareInputForExperiment(r.settings.Name) if err != nil { r.emitter.EmitFailureStartup("no default static input for this experiment") return diff --git a/pkg/oonimkall/taskrunner_test.go b/pkg/oonimkall/taskrunner_test.go index 1a3e732062..1c349dfdd0 100644 --- a/pkg/oonimkall/taskrunner_test.go +++ b/pkg/oonimkall/taskrunner_test.go @@ -7,7 +7,7 @@ import ( "time" "github.com/google/go-cmp/cmp" - engine "github.com/ooni/probe-cli/v3/internal/engine" + "github.com/ooni/probe-cli/v3/internal/inputloading" "github.com/ooni/probe-cli/v3/internal/model" ) @@ -645,7 +645,7 @@ func TestTaskRunnerRun(t *testing.T) { {Key: eventTypeStatusProgress, Count: 1}, {Key: eventTypeStatusReportCreate, Count: 1}, } - allEntries, err := engine.StaticBareInputForExperiment(experimentName) + allEntries, err := inputloading.StaticBareInputForExperiment(experimentName) if err != nil { t.Fatal(err) } From f05ba4ac6c0d0d5c1c3ac937a866215f89507aaa Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 6 Jun 2024 12:13:12 +0200 Subject: [PATCH 2/5] x --- cmd/ooniprobe/internal/nettests/dnscheck.go | 4 ++-- cmd/ooniprobe/internal/nettests/stunreachability.go | 4 ++-- cmd/ooniprobe/internal/nettests/web_connectivity.go | 4 ++-- internal/engine/inputloader_integration_test.go | 6 +++--- internal/oonirun/experiment.go | 4 ++-- internal/oonirun/session.go | 4 ++-- internal/{inputloading => targetloading}/inputloading.go | 4 ++-- .../{inputloading => targetloading}/inputloading_test.go | 2 +- .../testdata/inputloader1.txt | 0 .../testdata/inputloader2.txt | 0 .../testdata/inputloader3.txt | 0 pkg/oonimkall/taskrunner.go | 4 ++-- pkg/oonimkall/taskrunner_test.go | 4 ++-- 13 files changed, 20 insertions(+), 20 deletions(-) rename internal/{inputloading => targetloading}/inputloading.go (99%) rename internal/{inputloading => targetloading}/inputloading_test.go (99%) rename internal/{inputloading => targetloading}/testdata/inputloader1.txt (100%) rename internal/{inputloading => targetloading}/testdata/inputloader2.txt (100%) rename internal/{inputloading => targetloading}/testdata/inputloader3.txt (100%) diff --git a/cmd/ooniprobe/internal/nettests/dnscheck.go b/cmd/ooniprobe/internal/nettests/dnscheck.go index 44c7f86e2a..6824aa8af0 100644 --- a/cmd/ooniprobe/internal/nettests/dnscheck.go +++ b/cmd/ooniprobe/internal/nettests/dnscheck.go @@ -3,7 +3,7 @@ package nettests import ( "context" - "github.com/ooni/probe-cli/v3/internal/inputloading" + "github.com/ooni/probe-cli/v3/internal/targetloading" "github.com/ooni/probe-cli/v3/internal/model" ) @@ -11,7 +11,7 @@ import ( type DNSCheck struct{} func (n DNSCheck) lookupURLs(ctl *Controller) ([]model.ExperimentTarget, error) { - inputloader := &inputloading.Loader{ + inputloader := &targetloading.Loader{ CheckInConfig: &model.OOAPICheckInConfig{ // not needed because we have default static input in the engine }, diff --git a/cmd/ooniprobe/internal/nettests/stunreachability.go b/cmd/ooniprobe/internal/nettests/stunreachability.go index f77fe3ea84..6e2dcc267f 100644 --- a/cmd/ooniprobe/internal/nettests/stunreachability.go +++ b/cmd/ooniprobe/internal/nettests/stunreachability.go @@ -3,7 +3,7 @@ package nettests import ( "context" - "github.com/ooni/probe-cli/v3/internal/inputloading" + "github.com/ooni/probe-cli/v3/internal/targetloading" "github.com/ooni/probe-cli/v3/internal/model" ) @@ -11,7 +11,7 @@ import ( type STUNReachability struct{} func (n STUNReachability) lookupURLs(ctl *Controller) ([]model.ExperimentTarget, error) { - inputloader := &inputloading.Loader{ + inputloader := &targetloading.Loader{ CheckInConfig: &model.OOAPICheckInConfig{ // not needed because we have default static input in the engine }, diff --git a/cmd/ooniprobe/internal/nettests/web_connectivity.go b/cmd/ooniprobe/internal/nettests/web_connectivity.go index 9131e811a9..10fb759a2b 100644 --- a/cmd/ooniprobe/internal/nettests/web_connectivity.go +++ b/cmd/ooniprobe/internal/nettests/web_connectivity.go @@ -4,12 +4,12 @@ import ( "context" "github.com/apex/log" - "github.com/ooni/probe-cli/v3/internal/inputloading" + "github.com/ooni/probe-cli/v3/internal/targetloading" "github.com/ooni/probe-cli/v3/internal/model" ) func (n WebConnectivity) lookupURLs(ctl *Controller, categories []string) ([]model.ExperimentTarget, error) { - inputloader := &inputloading.Loader{ + inputloader := &targetloading.Loader{ CheckInConfig: &model.OOAPICheckInConfig{ // Setting Charging and OnWiFi to true causes the CheckIn // API to return to us as much URL as possible with the diff --git a/internal/engine/inputloader_integration_test.go b/internal/engine/inputloader_integration_test.go index a5a0bad598..712596b151 100644 --- a/internal/engine/inputloader_integration_test.go +++ b/internal/engine/inputloader_integration_test.go @@ -6,13 +6,13 @@ import ( "github.com/apex/log" "github.com/ooni/probe-cli/v3/internal/engine" - "github.com/ooni/probe-cli/v3/internal/inputloading" + "github.com/ooni/probe-cli/v3/internal/targetloading" "github.com/ooni/probe-cli/v3/internal/kvstore" "github.com/ooni/probe-cli/v3/internal/model" ) // This historical integration test ensures that we're able to fetch URLs from -// the dev infrastructure. We say this test's historical because the inputloading.Loader +// the dev infrastructure. We say this test's historical because the targetloading.Loader // belonged to the engine package before we introduced richer input. It kind of feels // good to keep this integration test here since we want to use a real session and a real // Loader and double check whether we can get inputs. In a more distant future it would @@ -36,7 +36,7 @@ func TestInputLoaderInputOrQueryBackendWithNoInput(t *testing.T) { t.Fatal(err) } defer sess.Close() - il := &inputloading.Loader{ + il := &targetloading.Loader{ InputPolicy: model.InputOrQueryBackend, Session: sess, } diff --git a/internal/oonirun/experiment.go b/internal/oonirun/experiment.go index 01eada8abb..29972ee73f 100644 --- a/internal/oonirun/experiment.go +++ b/internal/oonirun/experiment.go @@ -13,7 +13,7 @@ import ( "time" "github.com/ooni/probe-cli/v3/internal/humanize" - "github.com/ooni/probe-cli/v3/internal/inputloading" + "github.com/ooni/probe-cli/v3/internal/targetloading" "github.com/ooni/probe-cli/v3/internal/model" ) @@ -200,7 +200,7 @@ func (ed *Experiment) newInputLoader(inputPolicy model.InputPolicy) inputLoader if ed.newInputLoaderFn != nil { return ed.newInputLoaderFn(inputPolicy) } - return &inputloading.Loader{ + return &targetloading.Loader{ CheckInConfig: &model.OOAPICheckInConfig{ RunType: model.RunTypeManual, OnWiFi: true, // meaning: not on 4G diff --git a/internal/oonirun/session.go b/internal/oonirun/session.go index 4b55da17de..e740c74ac9 100644 --- a/internal/oonirun/session.go +++ b/internal/oonirun/session.go @@ -10,14 +10,14 @@ package oonirun // import ( - "github.com/ooni/probe-cli/v3/internal/inputloading" + "github.com/ooni/probe-cli/v3/internal/targetloading" "github.com/ooni/probe-cli/v3/internal/model" ) // Session is the definition of Session used by this package. type Session interface { // A Session is also an InputLoaderSession. - inputloading.Session + targetloading.Session // A Session is also a SubmitterSession. SubmitterSession diff --git a/internal/inputloading/inputloading.go b/internal/targetloading/inputloading.go similarity index 99% rename from internal/inputloading/inputloading.go rename to internal/targetloading/inputloading.go index 3b339cceaf..8329e91e0a 100644 --- a/internal/inputloading/inputloading.go +++ b/internal/targetloading/inputloading.go @@ -1,5 +1,5 @@ -// Package inputloading contains common code to load input. -package inputloading +// Package targetloading contains common code to load input. +package targetloading import ( "bufio" diff --git a/internal/inputloading/inputloading_test.go b/internal/targetloading/inputloading_test.go similarity index 99% rename from internal/inputloading/inputloading_test.go rename to internal/targetloading/inputloading_test.go index bcb77f4a56..5424828692 100644 --- a/internal/inputloading/inputloading_test.go +++ b/internal/targetloading/inputloading_test.go @@ -1,4 +1,4 @@ -package inputloading +package targetloading import ( "context" diff --git a/internal/inputloading/testdata/inputloader1.txt b/internal/targetloading/testdata/inputloader1.txt similarity index 100% rename from internal/inputloading/testdata/inputloader1.txt rename to internal/targetloading/testdata/inputloader1.txt diff --git a/internal/inputloading/testdata/inputloader2.txt b/internal/targetloading/testdata/inputloader2.txt similarity index 100% rename from internal/inputloading/testdata/inputloader2.txt rename to internal/targetloading/testdata/inputloader2.txt diff --git a/internal/inputloading/testdata/inputloader3.txt b/internal/targetloading/testdata/inputloader3.txt similarity index 100% rename from internal/inputloading/testdata/inputloader3.txt rename to internal/targetloading/testdata/inputloader3.txt diff --git a/pkg/oonimkall/taskrunner.go b/pkg/oonimkall/taskrunner.go index 78f2cec6a1..7ff6203894 100644 --- a/pkg/oonimkall/taskrunner.go +++ b/pkg/oonimkall/taskrunner.go @@ -8,7 +8,7 @@ import ( "time" "github.com/ooni/probe-cli/v3/internal/engine" - "github.com/ooni/probe-cli/v3/internal/inputloading" + "github.com/ooni/probe-cli/v3/internal/targetloading" "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/runtimex" ) @@ -192,7 +192,7 @@ func (r *runnerForTask) Run(rootCtx context.Context) { } case model.InputOrStaticDefault: if len(r.settings.Inputs) <= 0 { - inputs, err := inputloading.StaticBareInputForExperiment(r.settings.Name) + inputs, err := targetloading.StaticBareInputForExperiment(r.settings.Name) if err != nil { r.emitter.EmitFailureStartup("no default static input for this experiment") return diff --git a/pkg/oonimkall/taskrunner_test.go b/pkg/oonimkall/taskrunner_test.go index 1c349dfdd0..d7840e8f12 100644 --- a/pkg/oonimkall/taskrunner_test.go +++ b/pkg/oonimkall/taskrunner_test.go @@ -7,7 +7,7 @@ import ( "time" "github.com/google/go-cmp/cmp" - "github.com/ooni/probe-cli/v3/internal/inputloading" + "github.com/ooni/probe-cli/v3/internal/targetloading" "github.com/ooni/probe-cli/v3/internal/model" ) @@ -645,7 +645,7 @@ func TestTaskRunnerRun(t *testing.T) { {Key: eventTypeStatusProgress, Count: 1}, {Key: eventTypeStatusReportCreate, Count: 1}, } - allEntries, err := inputloading.StaticBareInputForExperiment(experimentName) + allEntries, err := targetloading.StaticBareInputForExperiment(experimentName) if err != nil { t.Fatal(err) } From f20490e64f77a99585b26859d934d6e5872f4aa7 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 6 Jun 2024 12:15:42 +0200 Subject: [PATCH 3/5] x --- .../{inputloading.go => targetloading.go} | 2 +- ...tloading_test.go => targetloading_test.go} | 46 +++++++++---------- .../{inputloader1.txt => loader1.txt} | 0 .../{inputloader2.txt => loader2.txt} | 0 .../{inputloader3.txt => loader3.txt} | 0 5 files changed, 24 insertions(+), 24 deletions(-) rename internal/targetloading/{inputloading.go => targetloading.go} (99%) rename internal/targetloading/{inputloading_test.go => targetloading_test.go} (96%) rename internal/targetloading/testdata/{inputloader1.txt => loader1.txt} (100%) rename internal/targetloading/testdata/{inputloader2.txt => loader2.txt} (100%) rename internal/targetloading/testdata/{inputloader3.txt => loader3.txt} (100%) diff --git a/internal/targetloading/inputloading.go b/internal/targetloading/targetloading.go similarity index 99% rename from internal/targetloading/inputloading.go rename to internal/targetloading/targetloading.go index 8329e91e0a..37f32d0c05 100644 --- a/internal/targetloading/inputloading.go +++ b/internal/targetloading/targetloading.go @@ -1,4 +1,4 @@ -// Package targetloading contains common code to load input. +// Package targetloading contains common code to load richer-input targets. package targetloading import ( diff --git a/internal/targetloading/inputloading_test.go b/internal/targetloading/targetloading_test.go similarity index 96% rename from internal/targetloading/inputloading_test.go rename to internal/targetloading/targetloading_test.go index 5424828692..1d5eed32fe 100644 --- a/internal/targetloading/inputloading_test.go +++ b/internal/targetloading/targetloading_test.go @@ -35,8 +35,8 @@ func TestInputLoaderInputNoneWithStaticInputs(t *testing.T) { func TestInputLoaderInputNoneWithFilesInputs(t *testing.T) { il := &Loader{ SourceFiles: []string{ - "testdata/inputloader1.txt", - "testdata/inputloader2.txt", + "testdata/loader1.txt", + "testdata/loader2.txt", }, InputPolicy: model.InputNone, } @@ -54,8 +54,8 @@ func TestInputLoaderInputNoneWithBothInputs(t *testing.T) { il := &Loader{ StaticInputs: []string{"https://www.google.com/"}, SourceFiles: []string{ - "testdata/inputloader1.txt", - "testdata/inputloader2.txt", + "testdata/loader1.txt", + "testdata/loader2.txt", }, InputPolicy: model.InputNone, } @@ -101,8 +101,8 @@ func TestInputLoaderInputOptionalWithInput(t *testing.T) { il := &Loader{ StaticInputs: []string{"https://www.google.com/"}, SourceFiles: []string{ - "testdata/inputloader1.txt", - "testdata/inputloader2.txt", + "testdata/loader1.txt", + "testdata/loader2.txt", }, InputPolicy: model.InputOptional, } @@ -150,9 +150,9 @@ func TestInputLoaderInputOptionalNonexistentFile(t *testing.T) { il := &Loader{ StaticInputs: []string{"https://www.google.com/"}, SourceFiles: []string{ - "testdata/inputloader1.txt", + "testdata/loader1.txt", "/nonexistent", - "testdata/inputloader2.txt", + "testdata/loader2.txt", }, InputPolicy: model.InputOptional, } @@ -170,8 +170,8 @@ func TestInputLoaderInputStrictlyRequiredWithInput(t *testing.T) { il := &Loader{ StaticInputs: []string{"https://www.google.com/"}, SourceFiles: []string{ - "testdata/inputloader1.txt", - "testdata/inputloader2.txt", + "testdata/loader1.txt", + "testdata/loader2.txt", }, InputPolicy: model.InputStrictlyRequired, } @@ -233,9 +233,9 @@ func TestInputLoaderInputStrictlyRequiredWithEmptyFile(t *testing.T) { il := &Loader{ InputPolicy: model.InputStrictlyRequired, SourceFiles: []string{ - "testdata/inputloader1.txt", - "testdata/inputloader3.txt", // we want it before inputloader2.txt - "testdata/inputloader2.txt", + "testdata/loader1.txt", + "testdata/loader3.txt", // we want it before loader2.txt + "testdata/loader2.txt", }, } ctx := context.Background() @@ -253,8 +253,8 @@ func TestInputLoaderInputOrStaticDefaultWithInput(t *testing.T) { ExperimentName: "dnscheck", StaticInputs: []string{"https://www.google.com/"}, SourceFiles: []string{ - "testdata/inputloader1.txt", - "testdata/inputloader2.txt", + "testdata/loader1.txt", + "testdata/loader2.txt", }, InputPolicy: model.InputOrStaticDefault, } @@ -303,9 +303,9 @@ func TestInputLoaderInputOrStaticDefaultWithEmptyFile(t *testing.T) { ExperimentName: "dnscheck", InputPolicy: model.InputOrStaticDefault, SourceFiles: []string{ - "testdata/inputloader1.txt", - "testdata/inputloader3.txt", // we want it before inputloader2.txt - "testdata/inputloader2.txt", + "testdata/loader1.txt", + "testdata/loader3.txt", // we want it before loader2.txt + "testdata/loader2.txt", }, } ctx := context.Background() @@ -400,8 +400,8 @@ func TestInputLoaderInputOrQueryBackendWithInput(t *testing.T) { il := &Loader{ StaticInputs: []string{"https://www.google.com/"}, SourceFiles: []string{ - "testdata/inputloader1.txt", - "testdata/inputloader2.txt", + "testdata/loader1.txt", + "testdata/loader2.txt", }, InputPolicy: model.InputOrQueryBackend, } @@ -473,9 +473,9 @@ func TestInputLoaderInputOrQueryBackendWithEmptyFile(t *testing.T) { il := &Loader{ InputPolicy: model.InputOrQueryBackend, SourceFiles: []string{ - "testdata/inputloader1.txt", - "testdata/inputloader3.txt", // we want it before inputloader2.txt - "testdata/inputloader2.txt", + "testdata/loader1.txt", + "testdata/loader3.txt", // we want it before loader2.txt + "testdata/loader2.txt", }, } ctx := context.Background() diff --git a/internal/targetloading/testdata/inputloader1.txt b/internal/targetloading/testdata/loader1.txt similarity index 100% rename from internal/targetloading/testdata/inputloader1.txt rename to internal/targetloading/testdata/loader1.txt diff --git a/internal/targetloading/testdata/inputloader2.txt b/internal/targetloading/testdata/loader2.txt similarity index 100% rename from internal/targetloading/testdata/inputloader2.txt rename to internal/targetloading/testdata/loader2.txt diff --git a/internal/targetloading/testdata/inputloader3.txt b/internal/targetloading/testdata/loader3.txt similarity index 100% rename from internal/targetloading/testdata/inputloader3.txt rename to internal/targetloading/testdata/loader3.txt From a362d365dc37a908dfad2b849911e33e1a798975 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 6 Jun 2024 12:23:59 +0200 Subject: [PATCH 4/5] x --- cmd/ooniprobe/internal/nettests/dnscheck.go | 6 +- .../internal/nettests/stunreachability.go | 6 +- .../internal/nettests/web_connectivity.go | 6 +- .../engine/inputloader_integration_test.go | 4 +- internal/mocks/experimentinputloader.go | 19 --- internal/mocks/experimenttargetloader.go | 19 +++ ...test.go => experimenttargetloader_test.go} | 2 +- internal/model/experiment.go | 4 +- internal/oonirun/experiment.go | 28 ++-- internal/oonirun/experiment_test.go | 26 ++-- internal/oonirun/session.go | 4 +- internal/oonirun/v1.go | 2 +- internal/oonirun/v2.go | 2 +- internal/targetloading/targetloading.go | 20 +-- internal/targetloading/targetloading_test.go | 122 +++++++++--------- pkg/oonimkall/taskrunner.go | 8 +- 16 files changed, 139 insertions(+), 139 deletions(-) delete mode 100644 internal/mocks/experimentinputloader.go create mode 100644 internal/mocks/experimenttargetloader.go rename internal/mocks/{experimentinputloader_test.go => experimenttargetloader_test.go} (93%) diff --git a/cmd/ooniprobe/internal/nettests/dnscheck.go b/cmd/ooniprobe/internal/nettests/dnscheck.go index 6824aa8af0..cb53560155 100644 --- a/cmd/ooniprobe/internal/nettests/dnscheck.go +++ b/cmd/ooniprobe/internal/nettests/dnscheck.go @@ -3,15 +3,15 @@ package nettests import ( "context" - "github.com/ooni/probe-cli/v3/internal/targetloading" "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/targetloading" ) // DNSCheck nettest implementation. type DNSCheck struct{} func (n DNSCheck) lookupURLs(ctl *Controller) ([]model.ExperimentTarget, error) { - inputloader := &targetloading.Loader{ + targetloader := &targetloading.Loader{ CheckInConfig: &model.OOAPICheckInConfig{ // not needed because we have default static input in the engine }, @@ -21,7 +21,7 @@ func (n DNSCheck) lookupURLs(ctl *Controller) ([]model.ExperimentTarget, error) SourceFiles: ctl.InputFiles, StaticInputs: ctl.Inputs, } - testlist, err := inputloader.Load(context.Background()) + testlist, err := targetloader.Load(context.Background()) if err != nil { return nil, err } diff --git a/cmd/ooniprobe/internal/nettests/stunreachability.go b/cmd/ooniprobe/internal/nettests/stunreachability.go index 6e2dcc267f..743d53e8f2 100644 --- a/cmd/ooniprobe/internal/nettests/stunreachability.go +++ b/cmd/ooniprobe/internal/nettests/stunreachability.go @@ -3,15 +3,15 @@ package nettests import ( "context" - "github.com/ooni/probe-cli/v3/internal/targetloading" "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/targetloading" ) // STUNReachability nettest implementation. type STUNReachability struct{} func (n STUNReachability) lookupURLs(ctl *Controller) ([]model.ExperimentTarget, error) { - inputloader := &targetloading.Loader{ + targetloader := &targetloading.Loader{ CheckInConfig: &model.OOAPICheckInConfig{ // not needed because we have default static input in the engine }, @@ -21,7 +21,7 @@ func (n STUNReachability) lookupURLs(ctl *Controller) ([]model.ExperimentTarget, SourceFiles: ctl.InputFiles, StaticInputs: ctl.Inputs, } - testlist, err := inputloader.Load(context.Background()) + testlist, err := targetloader.Load(context.Background()) if err != nil { return nil, err } diff --git a/cmd/ooniprobe/internal/nettests/web_connectivity.go b/cmd/ooniprobe/internal/nettests/web_connectivity.go index 10fb759a2b..7e4bcad996 100644 --- a/cmd/ooniprobe/internal/nettests/web_connectivity.go +++ b/cmd/ooniprobe/internal/nettests/web_connectivity.go @@ -4,12 +4,12 @@ import ( "context" "github.com/apex/log" - "github.com/ooni/probe-cli/v3/internal/targetloading" "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/targetloading" ) func (n WebConnectivity) lookupURLs(ctl *Controller, categories []string) ([]model.ExperimentTarget, error) { - inputloader := &targetloading.Loader{ + targetloader := &targetloading.Loader{ CheckInConfig: &model.OOAPICheckInConfig{ // Setting Charging and OnWiFi to true causes the CheckIn // API to return to us as much URL as possible with the @@ -27,7 +27,7 @@ func (n WebConnectivity) lookupURLs(ctl *Controller, categories []string) ([]mod SourceFiles: ctl.InputFiles, StaticInputs: ctl.Inputs, } - testlist, err := inputloader.Load(context.Background()) + testlist, err := targetloader.Load(context.Background()) if err != nil { return nil, err } diff --git a/internal/engine/inputloader_integration_test.go b/internal/engine/inputloader_integration_test.go index 712596b151..31b21a33ac 100644 --- a/internal/engine/inputloader_integration_test.go +++ b/internal/engine/inputloader_integration_test.go @@ -6,9 +6,9 @@ import ( "github.com/apex/log" "github.com/ooni/probe-cli/v3/internal/engine" - "github.com/ooni/probe-cli/v3/internal/targetloading" "github.com/ooni/probe-cli/v3/internal/kvstore" "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/targetloading" ) // This historical integration test ensures that we're able to fetch URLs from @@ -17,7 +17,7 @@ import ( // good to keep this integration test here since we want to use a real session and a real // Loader and double check whether we can get inputs. In a more distant future it would // kind of make sense to have a broader package with this kind of integration tests. -func TestInputLoaderInputOrQueryBackendWithNoInput(t *testing.T) { +func TestTargetLoaderInputOrQueryBackendWithNoInput(t *testing.T) { if testing.Short() { t.Skip("skip test in short mode") } diff --git a/internal/mocks/experimentinputloader.go b/internal/mocks/experimentinputloader.go deleted file mode 100644 index c46db13bef..0000000000 --- a/internal/mocks/experimentinputloader.go +++ /dev/null @@ -1,19 +0,0 @@ -package mocks - -import ( - "context" - - "github.com/ooni/probe-cli/v3/internal/model" -) - -// ExperimentInputLoader mocks model.ExperimentInputLoader -type ExperimentInputLoader struct { - MockLoad func(ctx context.Context) ([]model.ExperimentTarget, error) -} - -var _ model.ExperimentInputLoader = &ExperimentInputLoader{} - -// Load calls MockLoad -func (eil *ExperimentInputLoader) Load(ctx context.Context) ([]model.ExperimentTarget, error) { - return eil.MockLoad(ctx) -} diff --git a/internal/mocks/experimenttargetloader.go b/internal/mocks/experimenttargetloader.go new file mode 100644 index 0000000000..08f153232b --- /dev/null +++ b/internal/mocks/experimenttargetloader.go @@ -0,0 +1,19 @@ +package mocks + +import ( + "context" + + "github.com/ooni/probe-cli/v3/internal/model" +) + +// ExperimentTargetLoader mocks model.ExperimentTargetLoader +type ExperimentTargetLoader struct { + MockLoad func(ctx context.Context) ([]model.ExperimentTarget, error) +} + +var _ model.ExperimentTargetLoader = &ExperimentTargetLoader{} + +// Load calls MockLoad +func (eil *ExperimentTargetLoader) Load(ctx context.Context) ([]model.ExperimentTarget, error) { + return eil.MockLoad(ctx) +} diff --git a/internal/mocks/experimentinputloader_test.go b/internal/mocks/experimenttargetloader_test.go similarity index 93% rename from internal/mocks/experimentinputloader_test.go rename to internal/mocks/experimenttargetloader_test.go index a9d872e4bd..2cdc392b84 100644 --- a/internal/mocks/experimentinputloader_test.go +++ b/internal/mocks/experimenttargetloader_test.go @@ -11,7 +11,7 @@ import ( func TestExperimentInputLoader(t *testing.T) { t.Run("Load", func(t *testing.T) { expected := errors.New("mocked error") - eil := &ExperimentInputLoader{ + eil := &ExperimentTargetLoader{ MockLoad: func(ctx context.Context) ([]model.ExperimentTarget, error) { return nil, expected }, diff --git a/internal/model/experiment.go b/internal/model/experiment.go index bac0ce0a43..9ae3dbcce6 100644 --- a/internal/model/experiment.go +++ b/internal/model/experiment.go @@ -245,8 +245,8 @@ type ExperimentOptionInfo struct { Type string } -// ExperimentInputLoader loads inputs from local or remote sources. -type ExperimentInputLoader interface { +// ExperimentTargetLoader loads targets from local or remote sources. +type ExperimentTargetLoader interface { Load(ctx context.Context) ([]ExperimentTarget, error) } diff --git a/internal/oonirun/experiment.go b/internal/oonirun/experiment.go index 29972ee73f..09deedaedc 100644 --- a/internal/oonirun/experiment.go +++ b/internal/oonirun/experiment.go @@ -13,8 +13,8 @@ import ( "time" "github.com/ooni/probe-cli/v3/internal/humanize" - "github.com/ooni/probe-cli/v3/internal/targetloading" "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/targetloading" ) // experimentShuffledInputs counts how many times we shuffled inputs @@ -60,8 +60,8 @@ type Experiment struct { // newExperimentBuilderFn is OPTIONAL and used for testing. newExperimentBuilderFn func(experimentName string) (model.ExperimentBuilder, error) - // newInputLoaderFn is OPTIONAL and used for testing. - newInputLoaderFn func(inputPolicy model.InputPolicy) inputLoader + // newTargetLoaderFn is OPTIONAL and used for testing. + newTargetLoaderFn func(inputPolicy model.InputPolicy) targetLoader // newSubmitterFn is OPTIONAL and used for testing. newSubmitterFn func(ctx context.Context) (model.Submitter, error) @@ -84,8 +84,8 @@ func (ed *Experiment) Run(ctx context.Context) error { } // 2. create input loader and load input for this experiment - inputLoader := ed.newInputLoader(builder.InputPolicy()) - inputList, err := inputLoader.Load(ctx) + targetLoader := ed.newTargetLoader(builder.InputPolicy()) + targetList, err := targetLoader.Load(ctx) if err != nil { return err } @@ -93,8 +93,8 @@ func (ed *Experiment) Run(ctx context.Context) error { // 3. randomize input, if needed if ed.Random { rnd := rand.New(rand.NewSource(time.Now().UnixNano())) // #nosec G404 -- not really important - rnd.Shuffle(len(inputList), func(i, j int) { - inputList[i], inputList[j] = inputList[j], inputList[i] + rnd.Shuffle(len(targetList), func(i, j int) { + targetList[i], targetList[j] = targetList[j], targetList[i] }) experimentShuffledInputs.Add(1) } @@ -127,7 +127,7 @@ func (ed *Experiment) Run(ctx context.Context) error { } // 8. create an input processor - inputProcessor := ed.newInputProcessor(experiment, inputList, saver, submitter) + inputProcessor := ed.newInputProcessor(experiment, targetList, saver, submitter) // 9. process input and generate measurements return inputProcessor.Run(ctx) @@ -192,13 +192,13 @@ func (ed *Experiment) newExperimentBuilder(experimentName string) (model.Experim return ed.Session.NewExperimentBuilder(ed.Name) } -// inputLoader is an alias for model.ExperimentInputLoader -type inputLoader = model.ExperimentInputLoader +// targetLoader is an alias for [model.ExperimentTargetLoader]. +type targetLoader = model.ExperimentTargetLoader -// newInputLoader creates a new inputLoader. -func (ed *Experiment) newInputLoader(inputPolicy model.InputPolicy) inputLoader { - if ed.newInputLoaderFn != nil { - return ed.newInputLoaderFn(inputPolicy) +// newTargetLoader creates a new [model.ExperimentTargetLoader]. +func (ed *Experiment) newTargetLoader(inputPolicy model.InputPolicy) targetLoader { + if ed.newTargetLoaderFn != nil { + return ed.newTargetLoaderFn(inputPolicy) } return &targetloading.Loader{ CheckInConfig: &model.OOAPICheckInConfig{ diff --git a/internal/oonirun/experiment_test.go b/internal/oonirun/experiment_test.go index 16d6d16bb4..29764d9c8c 100644 --- a/internal/oonirun/experiment_test.go +++ b/internal/oonirun/experiment_test.go @@ -75,7 +75,7 @@ func TestExperimentRunWithFailureToSubmitAndShuffle(t *testing.T) { }, }, newExperimentBuilderFn: nil, - newInputLoaderFn: nil, + newTargetLoaderFn: nil, newSubmitterFn: func(ctx context.Context) (model.Submitter, error) { subm := &mocks.Submitter{ MockSubmit: func(ctx context.Context, m *model.Measurement) error { @@ -165,7 +165,7 @@ func TestExperimentRun(t *testing.T) { ReportFile string Session Session newExperimentBuilderFn func(experimentName string) (model.ExperimentBuilder, error) - newInputLoaderFn func(inputPolicy model.InputPolicy) inputLoader + newTargetLoaderFn func(inputPolicy model.InputPolicy) targetLoader newSubmitterFn func(ctx context.Context) (model.Submitter, error) newSaverFn func() (model.Saver, error) newInputProcessorFn func(experiment model.Experiment, @@ -199,8 +199,8 @@ func TestExperimentRun(t *testing.T) { } return eb, nil }, - newInputLoaderFn: func(inputPolicy model.InputPolicy) inputLoader { - return &mocks.ExperimentInputLoader{ + newTargetLoaderFn: func(inputPolicy model.InputPolicy) targetLoader { + return &mocks.ExperimentTargetLoader{ MockLoad: func(ctx context.Context) ([]model.ExperimentTarget, error) { return nil, errMocked }, @@ -223,8 +223,8 @@ func TestExperimentRun(t *testing.T) { } return eb, nil }, - newInputLoaderFn: func(inputPolicy model.InputPolicy) inputLoader { - return &mocks.ExperimentInputLoader{ + newTargetLoaderFn: func(inputPolicy model.InputPolicy) targetLoader { + return &mocks.ExperimentTargetLoader{ MockLoad: func(ctx context.Context) ([]model.ExperimentTarget, error) { return []model.ExperimentTarget{}, nil }, @@ -263,8 +263,8 @@ func TestExperimentRun(t *testing.T) { } return eb, nil }, - newInputLoaderFn: func(inputPolicy model.InputPolicy) inputLoader { - return &mocks.ExperimentInputLoader{ + newTargetLoaderFn: func(inputPolicy model.InputPolicy) targetLoader { + return &mocks.ExperimentTargetLoader{ MockLoad: func(ctx context.Context) ([]model.ExperimentTarget, error) { return []model.ExperimentTarget{}, nil }, @@ -306,8 +306,8 @@ func TestExperimentRun(t *testing.T) { } return eb, nil }, - newInputLoaderFn: func(inputPolicy model.InputPolicy) inputLoader { - return &mocks.ExperimentInputLoader{ + newTargetLoaderFn: func(inputPolicy model.InputPolicy) targetLoader { + return &mocks.ExperimentTargetLoader{ MockLoad: func(ctx context.Context) ([]model.ExperimentTarget, error) { return []model.ExperimentTarget{}, nil }, @@ -352,8 +352,8 @@ func TestExperimentRun(t *testing.T) { } return eb, nil }, - newInputLoaderFn: func(inputPolicy model.InputPolicy) inputLoader { - return &mocks.ExperimentInputLoader{ + newTargetLoaderFn: func(inputPolicy model.InputPolicy) targetLoader { + return &mocks.ExperimentTargetLoader{ MockLoad: func(ctx context.Context) ([]model.ExperimentTarget, error) { return []model.ExperimentTarget{}, nil }, @@ -392,7 +392,7 @@ func TestExperimentRun(t *testing.T) { ReportFile: tt.fields.ReportFile, Session: tt.fields.Session, newExperimentBuilderFn: tt.fields.newExperimentBuilderFn, - newInputLoaderFn: tt.fields.newInputLoaderFn, + newTargetLoaderFn: tt.fields.newTargetLoaderFn, newSubmitterFn: tt.fields.newSubmitterFn, newSaverFn: tt.fields.newSaverFn, newInputProcessorFn: tt.fields.newInputProcessorFn, diff --git a/internal/oonirun/session.go b/internal/oonirun/session.go index e740c74ac9..0f8fbc3602 100644 --- a/internal/oonirun/session.go +++ b/internal/oonirun/session.go @@ -10,13 +10,13 @@ package oonirun // import ( - "github.com/ooni/probe-cli/v3/internal/targetloading" "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/targetloading" ) // Session is the definition of Session used by this package. type Session interface { - // A Session is also an InputLoaderSession. + // A Session is also an [targetloading.Session]. targetloading.Session // A Session is also a SubmitterSession. diff --git a/internal/oonirun/v1.go b/internal/oonirun/v1.go index 48bf894709..8a5edeaecf 100644 --- a/internal/oonirun/v1.go +++ b/internal/oonirun/v1.go @@ -84,7 +84,7 @@ func v1Measure(ctx context.Context, config *LinkConfig, URL string) error { ReportFile: config.ReportFile, Session: config.Session, newExperimentBuilderFn: nil, - newInputLoaderFn: nil, + newTargetLoaderFn: nil, newSubmitterFn: nil, newSaverFn: nil, newInputProcessorFn: nil, diff --git a/internal/oonirun/v2.go b/internal/oonirun/v2.go index f6b74d32da..4330b07b76 100644 --- a/internal/oonirun/v2.go +++ b/internal/oonirun/v2.go @@ -194,7 +194,7 @@ func V2MeasureDescriptor(ctx context.Context, config *LinkConfig, desc *V2Descri ReportFile: config.ReportFile, Session: config.Session, newExperimentBuilderFn: nil, - newInputLoaderFn: nil, + newTargetLoaderFn: nil, newSubmitterFn: nil, newSaverFn: nil, newInputProcessorFn: nil, diff --git a/internal/targetloading/targetloading.go b/internal/targetloading/targetloading.go index 37f32d0c05..6590c2c4a7 100644 --- a/internal/targetloading/targetloading.go +++ b/internal/targetloading/targetloading.go @@ -243,7 +243,7 @@ func StaticBareInputForExperiment(name string) ([]string, error) { // staticInputForExperiment returns the static input for the given experiment // or an error if there's no static input for the experiment. func staticInputForExperiment(name string) ([]model.ExperimentTarget, error) { - return inputLoaderStringListToModelExperimentTarget(StaticBareInputForExperiment(name)) + return stringListToModelExperimentTarget(StaticBareInputForExperiment(name)) } // loadOrStaticDefault implements the InputOrStaticDefault policy. @@ -275,12 +275,12 @@ func (il *Loader) loadLocal() ([]model.ExperimentTarget, error) { return inputs, nil } -// inputLoaderOpenFn is the type of the function to open a file. -type inputLoaderOpenFn func(filepath string) (fs.File, error) +// openFunc is the type of the function to open a file. +type openFunc func(filepath string) (fs.File, error) // readfile reads inputs from the specified file. The open argument should be // compatible with stdlib's fs.Open and helps us with unit testing. -func (il *Loader) readfile(filepath string, open inputLoaderOpenFn) ([]model.ExperimentTarget, error) { +func (il *Loader) readfile(filepath string, open openFunc) ([]model.ExperimentTarget, error) { inputs := []model.ExperimentTarget{} filep, err := open(filepath) if err != nil { @@ -335,11 +335,11 @@ func (il *Loader) loadRemoteWebConnectivity(ctx context.Context) ([]model.Experi if reply.WebConnectivity == nil || len(reply.WebConnectivity.URLs) <= 0 { return nil, ErrNoURLsReturned } - output := inputLoaderModelOOAPIURLInfoToModelExperimentTarget(reply.WebConnectivity.URLs) + output := modelOOAPIURLInfoToModelExperimentTarget(reply.WebConnectivity.URLs) return output, nil } -func inputLoaderModelOOAPIURLInfoToModelExperimentTarget( +func modelOOAPIURLInfoToModelExperimentTarget( inputs []model.OOAPIURLInfo) (outputs []model.ExperimentTarget) { for _, input := range inputs { // Note: Dammit! Before we switch to go1.22 we need to continue to @@ -372,7 +372,7 @@ func (il *Loader) loadRemoteOpenVPN(ctx context.Context) ([]model.ExperimentTarg // hitting the API too many times. reply, err := il.fetchOpenVPNConfig(ctx, provider) if err != nil { - output := inputLoaderModelOOAPIURLInfoToModelExperimentTarget(urls) + output := modelOOAPIURLInfoToModelExperimentTarget(urls) return output, err } for _, input := range reply.Inputs { @@ -386,7 +386,7 @@ func (il *Loader) loadRemoteOpenVPN(ctx context.Context) ([]model.ExperimentTarg // the experiment on the backend. return nil, ErrNoURLsReturned } - output := inputLoaderModelOOAPIURLInfoToModelExperimentTarget(urls) + output := modelOOAPIURLInfoToModelExperimentTarget(urls) return output, nil } @@ -449,12 +449,12 @@ func (il *Loader) logger() Logger { return log.Log } -// inputLoaderStringListToModelExperimentTarget is an utility function to convert +// stringListToModelExperimentTarget is an utility function to convert // a list of strings containing URLs into a list of model.ExperimentTarget // which would have been returned by an hypothetical backend // API serving input for a test for which we don't have an API // yet (e.g., stunreachability and dnscheck). -func inputLoaderStringListToModelExperimentTarget(input []string, err error) ([]model.ExperimentTarget, error) { +func stringListToModelExperimentTarget(input []string, err error) ([]model.ExperimentTarget, error) { if err != nil { return nil, err } diff --git a/internal/targetloading/targetloading_test.go b/internal/targetloading/targetloading_test.go index 1d5eed32fe..9710ce1b34 100644 --- a/internal/targetloading/targetloading_test.go +++ b/internal/targetloading/targetloading_test.go @@ -17,7 +17,7 @@ import ( "github.com/ooni/probe-cli/v3/internal/runtimex" ) -func TestInputLoaderInputNoneWithStaticInputs(t *testing.T) { +func TestTargetLoaderInputNoneWithStaticInputs(t *testing.T) { il := &Loader{ StaticInputs: []string{"https://www.google.com/"}, InputPolicy: model.InputNone, @@ -32,7 +32,7 @@ func TestInputLoaderInputNoneWithStaticInputs(t *testing.T) { } } -func TestInputLoaderInputNoneWithFilesInputs(t *testing.T) { +func TestTargetLoaderInputNoneWithFilesInputs(t *testing.T) { il := &Loader{ SourceFiles: []string{ "testdata/loader1.txt", @@ -50,7 +50,7 @@ func TestInputLoaderInputNoneWithFilesInputs(t *testing.T) { } } -func TestInputLoaderInputNoneWithBothInputs(t *testing.T) { +func TestTargetLoaderInputNoneWithBothInputs(t *testing.T) { il := &Loader{ StaticInputs: []string{"https://www.google.com/"}, SourceFiles: []string{ @@ -69,7 +69,7 @@ func TestInputLoaderInputNoneWithBothInputs(t *testing.T) { } } -func TestInputLoaderInputNoneWithNoInput(t *testing.T) { +func TestTargetLoaderInputNoneWithNoInput(t *testing.T) { il := &Loader{ InputPolicy: model.InputNone, } @@ -83,7 +83,7 @@ func TestInputLoaderInputNoneWithNoInput(t *testing.T) { } } -func TestInputLoaderInputOptionalWithNoInput(t *testing.T) { +func TestTargetLoaderInputOptionalWithNoInput(t *testing.T) { il := &Loader{ InputPolicy: model.InputOptional, } @@ -97,7 +97,7 @@ func TestInputLoaderInputOptionalWithNoInput(t *testing.T) { } } -func TestInputLoaderInputOptionalWithInput(t *testing.T) { +func TestTargetLoaderInputOptionalWithInput(t *testing.T) { il := &Loader{ StaticInputs: []string{"https://www.google.com/"}, SourceFiles: []string{ @@ -146,7 +146,7 @@ func TestInputLoaderInputOptionalWithInput(t *testing.T) { } } -func TestInputLoaderInputOptionalNonexistentFile(t *testing.T) { +func TestTargetLoaderInputOptionalNonexistentFile(t *testing.T) { il := &Loader{ StaticInputs: []string{"https://www.google.com/"}, SourceFiles: []string{ @@ -166,7 +166,7 @@ func TestInputLoaderInputOptionalNonexistentFile(t *testing.T) { } } -func TestInputLoaderInputStrictlyRequiredWithInput(t *testing.T) { +func TestTargetLoaderInputStrictlyRequiredWithInput(t *testing.T) { il := &Loader{ StaticInputs: []string{"https://www.google.com/"}, SourceFiles: []string{ @@ -215,7 +215,7 @@ func TestInputLoaderInputStrictlyRequiredWithInput(t *testing.T) { } } -func TestInputLoaderInputStrictlyRequiredWithoutInput(t *testing.T) { +func TestTargetLoaderInputStrictlyRequiredWithoutInput(t *testing.T) { il := &Loader{ InputPolicy: model.InputStrictlyRequired, } @@ -229,7 +229,7 @@ func TestInputLoaderInputStrictlyRequiredWithoutInput(t *testing.T) { } } -func TestInputLoaderInputStrictlyRequiredWithEmptyFile(t *testing.T) { +func TestTargetLoaderInputStrictlyRequiredWithEmptyFile(t *testing.T) { il := &Loader{ InputPolicy: model.InputStrictlyRequired, SourceFiles: []string{ @@ -248,7 +248,7 @@ func TestInputLoaderInputStrictlyRequiredWithEmptyFile(t *testing.T) { } } -func TestInputLoaderInputOrStaticDefaultWithInput(t *testing.T) { +func TestTargetLoaderInputOrStaticDefaultWithInput(t *testing.T) { il := &Loader{ ExperimentName: "dnscheck", StaticInputs: []string{"https://www.google.com/"}, @@ -298,7 +298,7 @@ func TestInputLoaderInputOrStaticDefaultWithInput(t *testing.T) { } } -func TestInputLoaderInputOrStaticDefaultWithEmptyFile(t *testing.T) { +func TestTargetLoaderInputOrStaticDefaultWithEmptyFile(t *testing.T) { il := &Loader{ ExperimentName: "dnscheck", InputPolicy: model.InputOrStaticDefault, @@ -318,7 +318,7 @@ func TestInputLoaderInputOrStaticDefaultWithEmptyFile(t *testing.T) { } } -func TestInputLoaderInputOrStaticDefaultWithoutInputDNSCheck(t *testing.T) { +func TestTargetLoaderInputOrStaticDefaultWithoutInputDNSCheck(t *testing.T) { il := &Loader{ ExperimentName: "dnscheck", InputPolicy: model.InputOrStaticDefault, @@ -345,7 +345,7 @@ func TestInputLoaderInputOrStaticDefaultWithoutInputDNSCheck(t *testing.T) { } } -func TestInputLoaderInputOrStaticDefaultWithoutInputStunReachability(t *testing.T) { +func TestTargetLoaderInputOrStaticDefaultWithoutInputStunReachability(t *testing.T) { il := &Loader{ ExperimentName: "stunreachability", InputPolicy: model.InputOrStaticDefault, @@ -381,7 +381,7 @@ func TestStaticBareInputForExperimentWorksWithNonCanonicalNames(t *testing.T) { } } -func TestInputLoaderInputOrStaticDefaultWithoutInputOtherName(t *testing.T) { +func TestTargetLoaderInputOrStaticDefaultWithoutInputOtherName(t *testing.T) { il := &Loader{ ExperimentName: "xx", InputPolicy: model.InputOrStaticDefault, @@ -396,7 +396,7 @@ func TestInputLoaderInputOrStaticDefaultWithoutInputOtherName(t *testing.T) { } } -func TestInputLoaderInputOrQueryBackendWithInput(t *testing.T) { +func TestTargetLoaderInputOrQueryBackendWithInput(t *testing.T) { il := &Loader{ StaticInputs: []string{"https://www.google.com/"}, SourceFiles: []string{ @@ -445,7 +445,7 @@ func TestInputLoaderInputOrQueryBackendWithInput(t *testing.T) { } } -func TestInputLoaderInputOrQueryBackendWithNoInputAndCancelledContext(t *testing.T) { +func TestTargetLoaderInputOrQueryBackendWithNoInputAndCancelledContext(t *testing.T) { sess := &mocks.Session{ MockCheckIn: func(ctx context.Context, config *model.OOAPICheckInConfig) (*model.OOAPICheckInResult, error) { if err := ctx.Err(); err != nil { @@ -469,7 +469,7 @@ func TestInputLoaderInputOrQueryBackendWithNoInputAndCancelledContext(t *testing } } -func TestInputLoaderInputOrQueryBackendWithEmptyFile(t *testing.T) { +func TestTargetLoaderInputOrQueryBackendWithEmptyFile(t *testing.T) { il := &Loader{ InputPolicy: model.InputOrQueryBackend, SourceFiles: []string{ @@ -488,29 +488,29 @@ func TestInputLoaderInputOrQueryBackendWithEmptyFile(t *testing.T) { } } -type InputLoaderBrokenFS struct{} +type TargetLoaderBrokenFS struct{} -func (InputLoaderBrokenFS) Open(filepath string) (fs.File, error) { - return InputLoaderBrokenFile{}, nil +func (TargetLoaderBrokenFS) Open(filepath string) (fs.File, error) { + return TargetLoaderBrokenFile{}, nil } -type InputLoaderBrokenFile struct{} +type TargetLoaderBrokenFile struct{} -func (InputLoaderBrokenFile) Stat() (os.FileInfo, error) { +func (TargetLoaderBrokenFile) Stat() (os.FileInfo, error) { return nil, nil } -func (InputLoaderBrokenFile) Read([]byte) (int, error) { +func (TargetLoaderBrokenFile) Read([]byte) (int, error) { return 0, syscall.EFAULT } -func (InputLoaderBrokenFile) Close() error { +func (TargetLoaderBrokenFile) Close() error { return nil } -func TestInputLoaderReadfileScannerFailure(t *testing.T) { +func TestTargetLoaderReadfileScannerFailure(t *testing.T) { il := &Loader{} - out, err := il.readfile("", InputLoaderBrokenFS{}.Open) + out, err := il.readfile("", TargetLoaderBrokenFS{}.Open) if !errors.Is(err, syscall.EFAULT) { t.Fatal("not the error we expected") } @@ -519,9 +519,9 @@ func TestInputLoaderReadfileScannerFailure(t *testing.T) { } } -// InputLoaderMockableSession is a mockable session -// used by InputLoader tests. -type InputLoaderMockableSession struct { +// TargetLoaderMockableSession is a mockable session +// used by TargetLoader tests. +type TargetLoaderMockableSession struct { // Output contains the output of CheckIn. It should // be nil when Error is not-nil. Output *model.OOAPICheckInResult @@ -535,8 +535,8 @@ type InputLoaderMockableSession struct { Error error } -// CheckIn implements InputLoaderSession.CheckIn. -func (sess *InputLoaderMockableSession) CheckIn( +// CheckIn implements TargetLoaderSession.CheckIn. +func (sess *TargetLoaderMockableSession) CheckIn( ctx context.Context, config *model.OOAPICheckInConfig) (*model.OOAPICheckInResult, error) { if sess.Output == nil && sess.Error == nil { return nil, errors.New("both Output and Error are nil") @@ -544,16 +544,16 @@ func (sess *InputLoaderMockableSession) CheckIn( return sess.Output, sess.Error } -// FetchOpenVPNConfig implements InputLoaderSession.FetchOpenVPNConfig. -func (sess *InputLoaderMockableSession) FetchOpenVPNConfig( +// FetchOpenVPNConfig implements TargetLoaderSession.FetchOpenVPNConfig. +func (sess *TargetLoaderMockableSession) FetchOpenVPNConfig( ctx context.Context, provider, cc string) (*model.OOAPIVPNProviderConfig, error) { runtimex.Assert(!(sess.Error == nil && sess.FetchOpenVPNConfigOutput == nil), "both FetchOpenVPNConfig and Error are nil") return sess.FetchOpenVPNConfigOutput, sess.Error } -func TestInputLoaderCheckInFailure(t *testing.T) { +func TestTargetLoaderCheckInFailure(t *testing.T) { il := &Loader{ - Session: &InputLoaderMockableSession{ + Session: &TargetLoaderMockableSession{ Error: io.EOF, }, } @@ -566,9 +566,9 @@ func TestInputLoaderCheckInFailure(t *testing.T) { } } -func TestInputLoaderCheckInSuccessWithNilWebConnectivity(t *testing.T) { +func TestTargetLoaderCheckInSuccessWithNilWebConnectivity(t *testing.T) { il := &Loader{ - Session: &InputLoaderMockableSession{ + Session: &TargetLoaderMockableSession{ Output: &model.OOAPICheckInResult{ Tests: model.OOAPICheckInResultNettests{}, }, @@ -583,9 +583,9 @@ func TestInputLoaderCheckInSuccessWithNilWebConnectivity(t *testing.T) { } } -func TestInputLoaderCheckInSuccessWithNoURLs(t *testing.T) { +func TestTargetLoaderCheckInSuccessWithNoURLs(t *testing.T) { il := &Loader{ - Session: &InputLoaderMockableSession{ + Session: &TargetLoaderMockableSession{ Output: &model.OOAPICheckInResult{ Tests: model.OOAPICheckInResultNettests{ WebConnectivity: &model.OOAPICheckInInfoWebConnectivity{}, @@ -602,7 +602,7 @@ func TestInputLoaderCheckInSuccessWithNoURLs(t *testing.T) { } } -func TestInputLoaderCheckInSuccessWithSomeURLs(t *testing.T) { +func TestTargetLoaderCheckInSuccessWithSomeURLs(t *testing.T) { inputs0 := model.OOAPIURLInfo{ CategoryCode: "NEWS", CountryCode: "IT", @@ -616,7 +616,7 @@ func TestInputLoaderCheckInSuccessWithSomeURLs(t *testing.T) { inputs := []model.OOAPIURLInfo{inputs0, inputs1} expect := []model.ExperimentTarget{&inputs0, &inputs1} il := &Loader{ - Session: &InputLoaderMockableSession{ + Session: &TargetLoaderMockableSession{ Output: &model.OOAPICheckInResult{ Tests: model.OOAPICheckInResultNettests{ WebConnectivity: &model.OOAPICheckInInfoWebConnectivity{ @@ -635,11 +635,11 @@ func TestInputLoaderCheckInSuccessWithSomeURLs(t *testing.T) { } } -func TestInputLoaderOpenVPNSuccessWithNoInput(t *testing.T) { +func TestTargetLoaderOpenVPNSuccessWithNoInput(t *testing.T) { il := &Loader{ ExperimentName: "openvpn", InputPolicy: model.InputOrQueryBackend, - Session: &InputLoaderMockableSession{ + Session: &TargetLoaderMockableSession{ Error: nil, FetchOpenVPNConfigOutput: &model.OOAPIVPNProviderConfig{ Provider: "riseup", @@ -656,11 +656,11 @@ func TestInputLoaderOpenVPNSuccessWithNoInput(t *testing.T) { } } -func TestInputLoaderOpenVPNSuccessWithNoInputAndAPICall(t *testing.T) { +func TestTargetLoaderOpenVPNSuccessWithNoInputAndAPICall(t *testing.T) { il := &Loader{ ExperimentName: "openvpn", InputPolicy: model.InputOrQueryBackend, - Session: &InputLoaderMockableSession{ + Session: &TargetLoaderMockableSession{ Error: nil, FetchOpenVPNConfigOutput: &model.OOAPIVPNProviderConfig{ Provider: "riseupvpn", @@ -680,12 +680,12 @@ func TestInputLoaderOpenVPNSuccessWithNoInputAndAPICall(t *testing.T) { } } -func TestInputLoaderOpenVPNWithAPIFailureAndFallback(t *testing.T) { +func TestTargetLoaderOpenVPNWithAPIFailureAndFallback(t *testing.T) { expected := errors.New("mocked API error") il := &Loader{ ExperimentName: "openvpn", InputPolicy: model.InputOrQueryBackend, - Session: &InputLoaderMockableSession{ + Session: &TargetLoaderMockableSession{ Error: expected, }, } @@ -698,11 +698,11 @@ func TestInputLoaderOpenVPNWithAPIFailureAndFallback(t *testing.T) { } } -func TestInputLoaderOpenVPNWithNoReturnedURLs(t *testing.T) { +func TestTargetLoaderOpenVPNWithNoReturnedURLs(t *testing.T) { il := &Loader{ ExperimentName: "openvpn", InputPolicy: model.InputOrQueryBackend, - Session: &InputLoaderMockableSession{ + Session: &TargetLoaderMockableSession{ FetchOpenVPNConfigOutput: &model.OOAPIVPNProviderConfig{ Provider: "riseupvpn", Config: &model.OOAPIVPNConfig{}, @@ -792,16 +792,16 @@ func TestPreventMistakesWithoutCategoriesAndEmpty(t *testing.T) { } } -// InputLoaderFakeLogger is a fake InputLoaderLogger. -type InputLoaderFakeLogger struct{} +// TargetLoaderFakeLogger is a fake TargetLoaderLogger. +type TargetLoaderFakeLogger struct{} -// Warnf implements InputLoaderLogger.Warnf -func (ilfl *InputLoaderFakeLogger) Warnf(format string, v ...interface{}) {} +// Warnf implements TargetLoaderLogger.Warnf +func (ilfl *TargetLoaderFakeLogger) Warnf(format string, v ...interface{}) {} -func TestInputLoaderLoggerWorksAsIntended(t *testing.T) { - logger := &InputLoaderFakeLogger{} - inputLoader := &Loader{Logger: logger} - out := inputLoader.logger() +func TestTargetLoaderLoggerWorksAsIntended(t *testing.T) { + logger := &TargetLoaderFakeLogger{} + targetLoader := &Loader{Logger: logger} + out := targetLoader.logger() if out != logger { t.Fatal("logger not working as intended") } @@ -812,7 +812,7 @@ func TestStringListToModelURLInfoWithValidInput(t *testing.T) { "stun://stun.voip.blackberry.com:3478", "stun://stun.altar.com.pl:3478", } - output, err := inputLoaderStringListToModelExperimentTarget(input, nil) + output, err := stringListToModelExperimentTarget(input, nil) if err != nil { t.Fatal(err) } @@ -838,7 +838,7 @@ func TestStringListToModelURLInfoWithInvalidInput(t *testing.T) { "\t", // <- not a valid URL "stun://stun.altar.com.pl:3478", } - output, err := inputLoaderStringListToModelExperimentTarget(input, nil) + output, err := stringListToModelExperimentTarget(input, nil) if err == nil || !strings.HasSuffix(err.Error(), "invalid control character in URL") { t.Fatal("no the error we expected", err) } @@ -854,7 +854,7 @@ func TestStringListToModelURLInfoWithError(t *testing.T) { "stun://stun.altar.com.pl:3478", } expected := errors.New("mocked error") - output, err := inputLoaderStringListToModelExperimentTarget(input, expected) + output, err := stringListToModelExperimentTarget(input, expected) if !errors.Is(err, expected) { t.Fatal("not the error we expected", err) } diff --git a/pkg/oonimkall/taskrunner.go b/pkg/oonimkall/taskrunner.go index 7ff6203894..dcc2e4e81a 100644 --- a/pkg/oonimkall/taskrunner.go +++ b/pkg/oonimkall/taskrunner.go @@ -8,9 +8,9 @@ import ( "time" "github.com/ooni/probe-cli/v3/internal/engine" - "github.com/ooni/probe-cli/v3/internal/targetloading" "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/runtimex" + "github.com/ooni/probe-cli/v3/internal/targetloading" ) // runnerForTask runs a specific task @@ -170,16 +170,16 @@ func (r *runnerForTask) Run(rootCtx context.Context) { builder.SetCallbacks(&runnerCallbacks{emitter: r.emitter}) // TODO(bassosimone): replace the following code with an - // invocation of the InputLoader. Since I am making these + // invocation of the targetloading.Loader. Since I am making these // changes before a release and I've already changed the // code a lot, I'd rather avoid changing it even more, // for the following reason: // - // If we add an call InputLoader here, this code will + // If we add an call targetloading.Loader here, this code will // magically invoke check-in for InputOrQueryBackend, // which we need to make sure the app can handle. This is // the main reason why now I don't fill like properly - // fixing this code and use InputLoader: too much work + // fixing this code and use targetloading.Loader: too much work // in too little time, so mistakes more likely. // // In fact, our current app assumes that it's its From 067e043d6d35539b99be7d6ab758b69ad1a1bec9 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 6 Jun 2024 13:08:59 +0200 Subject: [PATCH 5/5] Update pkg/oonimkall/taskrunner.go Co-authored-by: DecFox <33030671+DecFox@users.noreply.github.com> --- pkg/oonimkall/taskrunner.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/oonimkall/taskrunner.go b/pkg/oonimkall/taskrunner.go index dcc2e4e81a..d5b70947c0 100644 --- a/pkg/oonimkall/taskrunner.go +++ b/pkg/oonimkall/taskrunner.go @@ -175,7 +175,7 @@ func (r *runnerForTask) Run(rootCtx context.Context) { // code a lot, I'd rather avoid changing it even more, // for the following reason: // - // If we add an call targetloading.Loader here, this code will + // If we add and call targetloading.Loader here, this code will // magically invoke check-in for InputOrQueryBackend, // which we need to make sure the app can handle. This is // the main reason why now I don't fill like properly