diff --git a/docs/design/dd-008-richer-input.md b/docs/design/dd-008-richer-input.md index 730616747..6590fcbd0 100644 --- a/docs/design/dd-008-richer-input.md +++ b/docs/design/dd-008-richer-input.md @@ -445,6 +445,106 @@ In practice, the code would always use either `InitialOptions` or `ExtraOptions`, but we also wanted to specify priority in case both of them were available. +## Implementation: oonimkall changes + +In [#1620](https://github.com/ooni/probe-cli/pull/1620), we started to +modify the `./pkg/oonimkall` package to support richer input. + +Before this diff, the code was not using a loader for loading targets +for experiments, and the code roughly looked like this: + +```Go +switch builder.InputPolicy() { + case model.InputOrQueryBackend, model.InputStrictlyRequired: + if len(r.settings.Inputs) <= 0 { + r.emitter.EmitFailureStartup("no input provided") + return + } + + case model.InputOrStaticDefault: + if len(r.settings.Inputs) <= 0 { + inputs, err := targetloading.StaticBareInputForExperiment(r.settings.Name) + if err != nil { + r.emitter.EmitFailureStartup("no default static input for this experiment") + return + } + r.settings.Inputs = inputs + } + + case model.InputOptional: + if len(r.settings.Inputs) <= 0 { + r.settings.Inputs = append(r.settings.Inputs, "") + } + + default: // treat this case as engine.InputNone. + if len(r.settings.Inputs) > 0 { + r.emitter.EmitFailureStartup("experiment does not accept input") + return + } + r.settings.Inputs = append(r.settings.Inputs, "") +} +``` + +Basically, we were switching on the experiment builder's `InputPolicy` and +checking whether input was present or absent according to policy. But, we were +not *actually* loading input when needed. + +To support richer input for experiments such as `openvpn`, instead, we must +use a loader and fetch such input, as follows: + +```Go +loader := builder.NewTargetLoader(&model.ExperimentTargetLoaderConfig{ + CheckInConfig: &model.OOAPICheckInConfig{ /* not needed for now */ }, + Session: sess, + StaticInputs: r.settings.Inputs, + SourceFiles: []string{}, +}) +loadCtx, loadCancel := context.WithTimeout(rootCtx, 30*time.Second) +defer loadCancel() +targets, err := loader.Load(loadCtx) +if err != nil { + r.emitter.EmitFailureStartup(err.Error()) + return +} +``` + +After this change, we still assume the mobile app is providing us with +inputs for Web Connectivity. Because the loader honours user-provided inputs, +there's no functional change with the previous behavior. However, if there +is no input, we're going to load it using the proper mechanisms, including +using the correct backend API for the `openvpn` experiment. + +Also, to pave the way for supporting loading for Web Connectivity as well, we +need to supply the information required to populate the URLs table as part +of the `status.measurement_start` event, as follows: + +```diff + type eventMeasurementGeneric struct { ++ CategoryCode string `json:"category_code,omitempty"` ++ CountryCode string `json:"country_code,omitempty"` + Failure string `json:"failure,omitempty"` + Idx int64 `json:"idx"` + Input string `json:"input"` + JSONStr string `json:"json_str,omitempty"` + } + + + r.emitter.Emit(eventTypeStatusMeasurementStart, eventMeasurementGeneric{ ++ CategoryCode: target.Category(), ++ CountryCode: target.Country(), + Idx: int64(idx), + Input: target.Input(), + }) +``` + +By providing the `CategoryCode` and the `CountryCode`, the mobile app is now +able to correctly populate the URLs table ahead of measuring. + +Future work will address passing the correct check-in options to the +experiment runner, so that we can actually remove the mobile app source +code that invokes the check-in API, and simplify both the codebase of +the mobile app and the one of `./pkg/oonimkall`. + ## Next steps This is a rough sequence of next steps that we should expand as we implement @@ -466,11 +566,12 @@ than using the check-in API (and maybe keep the caching support?). its own constructor for the proper target loader, and split the implementation inside of the `targetloader` package to have multiple target loaders. - * rework `pkg/oonimkall` to invoke a target loader rather than relying - on the `InputPolicy` - * make sure richer-input-enabled experiments can run with `oonimkall` after we have performed the previous change + * make sure we're passing the correct check-in settings to `oonimkall` + such that it's possible to run Web Connectivity from mobile using + the loader and we can simplify the mobile app codebase + * devise long term strategy for delivering richer input to `oonimkall` from mobile apps, which we'll need as soon as we convert the IM experiments diff --git a/internal/targetloading/targetloading.go b/internal/targetloading/targetloading.go index bdddb675f..dc9162354 100644 --- a/internal/targetloading/targetloading.go +++ b/internal/targetloading/targetloading.go @@ -215,10 +215,10 @@ var dnsCheckDefaultInput = []string{ var stunReachabilityDefaultInput = stuninput.AsnStunReachabilityInput() -// StaticBareInputForExperiment returns the list of strings an +// staticBareInputForExperiment returns the list of strings an // experiment should use as static input. In case there is no // static input for this experiment, we return an error. -func StaticBareInputForExperiment(name string) ([]string, error) { +func staticBareInputForExperiment(name string) ([]string, error) { // Implementation note: we may be called from pkg/oonimkall // with a non-canonical experiment name, so we need to convert // the experiment name to be canonical before proceeding. @@ -239,7 +239,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 stringListToModelExperimentTarget(StaticBareInputForExperiment(name)) + return stringListToModelExperimentTarget(staticBareInputForExperiment(name)) } // loadOrStaticDefault implements the InputOrStaticDefault policy. @@ -364,15 +364,6 @@ func (il *Loader) checkIn( return &reply.Tests, nil } -// fetchOpenVPNConfig fetches vpn information for the configured providers -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 - } - return reply, nil -} - // 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. diff --git a/pkg/oonimkall/taskmodel.go b/pkg/oonimkall/taskmodel.go index e030cbc8f..41e5b19ea 100644 --- a/pkg/oonimkall/taskmodel.go +++ b/pkg/oonimkall/taskmodel.go @@ -106,10 +106,12 @@ type eventLog struct { } type eventMeasurementGeneric struct { - Failure string `json:"failure,omitempty"` - Idx int64 `json:"idx"` - Input string `json:"input"` - JSONStr string `json:"json_str,omitempty"` + CategoryCode string `json:"category_code,omitempty"` + CountryCode string `json:"country_code,omitempty"` + Failure string `json:"failure,omitempty"` + Idx int64 `json:"idx"` + Input string `json:"input"` + JSONStr string `json:"json_str,omitempty"` } type eventStatusEnd struct { @@ -314,4 +316,8 @@ type settingsOptions struct { // SoftwareVersion is the software version. If this option is not // present, then the library startup will fail. SoftwareVersion string `json:"software_version,omitempty"` + + // TODO(https://github.com/ooni/probe/issues/2767): to support OONI Run v2 descriptors with + // richer input from mobile, here we also need a string-serialization + // of the descriptor options to load. } diff --git a/pkg/oonimkall/taskrunner.go b/pkg/oonimkall/taskrunner.go index 000532e25..498bbc1fc 100644 --- a/pkg/oonimkall/taskrunner.go +++ b/pkg/oonimkall/taskrunner.go @@ -11,7 +11,6 @@ import ( "github.com/ooni/probe-cli/v3/internal/kvstore" "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 @@ -118,6 +117,8 @@ func (r *runnerForTask) Run(rootCtx context.Context) { // // - rootCtx is the root context and is controlled by the user; // + // - loadCtx derives from rootCtx and is used to load inputs; + // // - measCtx derives from rootCtx and is possibly tied to the // maximum runtime and is used to choose when to stop measuring; // @@ -127,28 +128,36 @@ func (r *runnerForTask) Run(rootCtx context.Context) { // See https://github.com/ooni/probe/issues/2037. var logger model.Logger = newTaskLogger(r.emitter, r.settings.LogLevel) r.emitter.Emit(eventTypeStatusQueued, eventEmpty{}) + + // check whether we support the provided settings if r.hasUnsupportedSettings() { // event failureStartup already emitted return } r.emitter.Emit(eventTypeStatusStarted, eventEmpty{}) + + // create a new measurement session sess, err := r.newsession(rootCtx, logger) if err != nil { r.emitter.EmitFailureStartup(err.Error()) return } + + // make sure we emit the status.end event when we're done endEvent := new(eventStatusEnd) defer func() { _ = sess.Close() r.emitter.Emit(eventTypeStatusEnd, endEvent) }() + // create an experiment builder for the given experiment name builder, err := sess.NewExperimentBuilder(r.settings.Name) if err != nil { r.emitter.EmitFailureStartup(err.Error()) return } + // choose the proper OONI backend to use logger.Info("Looking up OONI backends... please, be patient") if err := sess.MaybeLookupBackendsContext(rootCtx); err != nil { r.emitter.EmitFailureStartup(err.Error()) @@ -156,6 +165,7 @@ func (r *runnerForTask) Run(rootCtx context.Context) { } r.emitter.EmitStatusProgress(0.1, "contacted bouncer") + // discover the probe location logger.Info("Looking up your location... please, be patient") if err := sess.MaybeLookupLocationContext(rootCtx); err != nil { r.emitter.EmitFailureGeneric(eventTypeFailureIPLookup, err.Error()) @@ -178,54 +188,37 @@ func (r *runnerForTask) Run(rootCtx context.Context) { ResolverNetworkName: sess.ResolverNetworkName(), }) + // configure the callbacks to emit events builder.SetCallbacks(&runnerCallbacks{emitter: r.emitter}) - // TODO(bassosimone): replace the following code with an - // 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 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 - // 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 - // responsibility to load the inputs, not oonimkall's. - switch builder.InputPolicy() { - case model.InputOrQueryBackend, model.InputStrictlyRequired: - if len(r.settings.Inputs) <= 0 { - r.emitter.EmitFailureStartup("no input provided") - return - } - case model.InputOrStaticDefault: - if len(r.settings.Inputs) <= 0 { - inputs, err := targetloading.StaticBareInputForExperiment(r.settings.Name) - if err != nil { - r.emitter.EmitFailureStartup("no default static input for this experiment") - return - } - r.settings.Inputs = inputs - } - case model.InputOptional: - if len(r.settings.Inputs) <= 0 { - r.settings.Inputs = append(r.settings.Inputs, "") - } - default: // treat this case as engine.InputNone. - if len(r.settings.Inputs) > 0 { - r.emitter.EmitFailureStartup("experiment does not accept input") - return - } - r.settings.Inputs = append(r.settings.Inputs, "") + // load targets using the experiment-specific loader + loader := builder.NewTargetLoader(&model.ExperimentTargetLoaderConfig{ + CheckInConfig: &model.OOAPICheckInConfig{ + // TODO(https://github.com/ooni/probe/issues/2766): to correctly load Web Connectivity targets + // here we need to honour the relevant check-in settings. + }, + Session: sess, + StaticInputs: r.settings.Inputs, + SourceFiles: []string{}, + }) + loadCtx, loadCancel := context.WithTimeout(rootCtx, 30*time.Second) + defer loadCancel() + targets, err := loader.Load(loadCtx) + if err != nil { + r.emitter.EmitFailureStartup(err.Error()) + return } + + // create the new experiment experiment := builder.NewExperiment() + + // make sure we account for the bytes sent and received defer func() { endEvent.DownloadedKB = experiment.KibiBytesReceived() endEvent.UploadedKB = experiment.KibiBytesSent() }() + + // open a new report if possible if !r.settings.Options.NoCollector { logger.Info("Opening report... please, be patient") if err := experiment.OpenReportContext(rootCtx); err != nil { @@ -237,52 +230,72 @@ func (r *runnerForTask) Run(rootCtx context.Context) { ReportID: experiment.ReportID(), }) } + + // create the default context for measuring measCtx, measCancel := context.WithCancel(rootCtx) defer measCancel() + + // create the default context for submitting submitCtx, submitCancel := context.WithCancel(rootCtx) defer submitCancel() + + // Update measCtx and submitCtx to be timeout bound in case there's + // more than one input/target to measure. + // // This deviates a little bit from measurement-kit, for which // a zero timeout is actually valid. Since it does not make much // sense, here we're changing the behaviour. // + // Additionally, since https://github.com/ooni/probe-cli/pull/1620, + // we honour the MaxRuntime for all experiments that have more + // than one input. Previously, it was just Web Connectivity, yet, + // it seems reasonable to honour MaxRuntime everytime the whole + // experiment runtime depends on more than one input. + // // See https://github.com/measurement-kit/measurement-kit/issues/1922 - if r.settings.Options.MaxRuntime > 0 { - // We want to honour max_runtime only when we're running an - // experiment that clearly wants specific input. We could refine - // this policy in the future, but for now this covers in a - // reasonable way web connectivity, so we should be ok. - switch builder.InputPolicy() { - case model.InputOrQueryBackend, model.InputStrictlyRequired: - var ( - cancelMeas context.CancelFunc - cancelSubmit context.CancelFunc - ) - // We give the context used for submitting extra time so that - // it's possible to submit the last measurement. - // - // See https://github.com/ooni/probe/issues/2037 for more info. - maxRuntime := time.Duration(r.settings.Options.MaxRuntime) * time.Second - measCtx, cancelMeas = context.WithTimeout(measCtx, maxRuntime) - defer cancelMeas() - maxRuntime += 30 * time.Second - submitCtx, cancelSubmit = context.WithTimeout(submitCtx, maxRuntime) - defer cancelSubmit() - } + if r.settings.Options.MaxRuntime > 0 && len(targets) > 1 { + var ( + cancelMeas context.CancelFunc + cancelSubmit context.CancelFunc + ) + // We give the context used for submitting extra time so that + // it's possible to submit the last measurement. + // + // See https://github.com/ooni/probe/issues/2037 for more info. + maxRuntime := time.Duration(r.settings.Options.MaxRuntime) * time.Second + measCtx, cancelMeas = context.WithTimeout(measCtx, maxRuntime) + defer cancelMeas() + maxRuntime += 30 * time.Second + submitCtx, cancelSubmit = context.WithTimeout(submitCtx, maxRuntime) + defer cancelSubmit() } - inputCount := len(r.settings.Inputs) + + // prepare for cycling through the targets + inputCount := len(targets) start := time.Now() inflatedMaxRuntime := r.settings.Options.MaxRuntime + r.settings.Options.MaxRuntime/10 eta := start.Add(time.Duration(inflatedMaxRuntime) * time.Second) - for idx, input := range r.settings.Inputs { + + for idx, target := range targets { + // handle the case where the time allocated for measuring has elapsed if measCtx.Err() != nil { break } + + // notify the mobile app that we are about to measure a specific target + // + // note that here we provide also the CategoryCode and the CountryCode + // so that the mobile app can update its URLs table here logger.Infof("Starting measurement with index %d", idx) r.emitter.Emit(eventTypeStatusMeasurementStart, eventMeasurementGeneric{ - Idx: int64(idx), - Input: input, + CategoryCode: target.Category(), + CountryCode: target.Country(), + Idx: int64(idx), + Input: target.Input(), }) - if input != "" && inputCount > 0 { + + // emit progress when there is more than one target to measure + if target.Input() != "" && inputCount > 0 { var percentage float64 if r.settings.Options.MaxRuntime > 0 { now := time.Now() @@ -291,33 +304,29 @@ func (r *runnerForTask) Run(rootCtx context.Context) { percentage = (float64(idx)/float64(inputCount))*0.6 + 0.4 } r.emitter.EmitStatusProgress(percentage, fmt.Sprintf( - "processing %s", input, + "processing %s", target, )) } - // Richer input implementation note: in mobile, we only observe richer input - // for Web Connectivity and only store this kind of input into the database and - // otherwise we ignore richer input for other experiments, which are just - // treated as experimental. As such, the thinking here is that we do not care - // about *passing* richer input from desktop to mobile for some time. When - // we will care, it would most likely suffice to require the Inputs field to - // implement in Java the [model.ExperimentTarget] interface, which is something - // we can always do, since it only has string accessors. + // Perform the measurement proper. m, err := experiment.MeasureWithContext( r.contextForExperiment(measCtx, builder), - model.NewOOAPIURLInfoWithDefaultCategoryAndCountry(input), + target, ) + // Handle the case where our time for measuring has elapsed while + // we were measuring and assume the context interrupted the measurement + // midway, so it doesn't make sense to submit it. if builder.Interruptible() && measCtx.Err() != nil { - // We want to stop here only if interruptible otherwise we want to - // submit measurement and stop at beginning of next iteration break } + + // handle the case where the measurement has failed if err != nil { r.emitter.Emit(eventTypeFailureMeasurement, eventMeasurementGeneric{ Failure: err.Error(), Idx: int64(idx), - Input: input, + Input: target.Input(), }) // Historical note: here we used to fallthrough but, since we have // implemented async measurements, the case where there is an error @@ -325,28 +334,38 @@ func (r *runnerForTask) Run(rootCtx context.Context) { // now the only valid strategy here is to continue. continue } + + // make sure the measurement contains the user-specified annotations m.AddAnnotations(r.settings.Annotations) + + // serialize the measurement to JSON (cannot fail in practice) data, err := json.Marshal(m) runtimex.PanicOnError(err, "measurement.MarshalJSON failed") + + // let the mobile app know about this measurement r.emitter.Emit(eventTypeMeasurement, eventMeasurementGeneric{ Idx: int64(idx), - Input: input, + Input: target.Input(), JSONStr: string(data), }) + + // if possible, submit the measurement to the OONI backend if !r.settings.Options.NoCollector { logger.Info("Submitting measurement... please, be patient") err := experiment.SubmitAndUpdateMeasurementContext(submitCtx, m) warnOnFailure(logger, "cannot submit measurement", err) r.emitter.Emit(measurementSubmissionEventName(err), eventMeasurementGeneric{ Idx: int64(idx), - Input: input, + Input: target.Input(), JSONStr: string(data), Failure: measurementSubmissionFailure(err), }) } + + // let the app know that we're done measuring this entry r.emitter.Emit(eventTypeStatusMeasurementDone, eventMeasurementGeneric{ Idx: int64(idx), - Input: input, + Input: target.Input(), }) } } diff --git a/pkg/oonimkall/taskrunner_test.go b/pkg/oonimkall/taskrunner_test.go index 74d446423..432a8b307 100644 --- a/pkg/oonimkall/taskrunner_test.go +++ b/pkg/oonimkall/taskrunner_test.go @@ -10,7 +10,6 @@ import ( "github.com/ooni/probe-cli/v3/internal/engine" "github.com/ooni/probe-cli/v3/internal/mocks" "github.com/ooni/probe-cli/v3/internal/model" - "github.com/ooni/probe-cli/v3/internal/targetloading" ) func TestMeasurementSubmissionEventName(t *testing.T) { @@ -103,6 +102,10 @@ func TestTaskRunnerRun(t *testing.T) { assertCountEventsByKey(events, eventTypeFailureStartup, 1) }) + // + // Failure in creating a new measurement session: + // + t.Run("with failure when creating a new kvstore", func(t *testing.T) { runner, emitter := newRunnerForTesting() // override the kvstore builder to provoke an error @@ -166,6 +169,10 @@ func TestTaskRunnerRun(t *testing.T) { } }) + // + // Test cases where we successfully create a new measurement session: + // + type eventKeyCount struct { Key string Count int @@ -176,7 +183,7 @@ func TestTaskRunnerRun(t *testing.T) { reduceEventsKeysIgnoreLog := func(t *testing.T, events []*event) (out []eventKeyCount) { var current eventKeyCount for _, ev := range events { - t.Log(ev) + t.Logf("%+v", ev) if ev.Key == eventTypeLog { continue } @@ -270,6 +277,16 @@ func TestTaskRunnerRun(t *testing.T) { return "GARR" }, }, + + Loader: &mocks.ExperimentTargetLoader{ + MockLoad: func(ctx context.Context) ([]model.ExperimentTarget, error) { + // This returns a single entry, which is what dash, ndt, telegram, etc need + targets := []model.ExperimentTarget{ + model.NewOOAPIURLInfoWithDefaultCategoryAndCountry(""), + } + return targets, nil + }, + }, } // The fake session MUST return the fake experiment builder @@ -296,7 +313,7 @@ func TestTaskRunnerRun(t *testing.T) { } } - t.Run("with invalid experiment name", func(t *testing.T) { + t.Run("with invalid experiment name causing failure to create an experiment builder", func(t *testing.T) { runner, emitter := newRunnerForTesting() fake := fakeSuccessfulDeps() fake.Session.MockNewExperimentBuilder = func(name string) (model.ExperimentBuilder, error) { @@ -354,77 +371,15 @@ func TestTaskRunnerRun(t *testing.T) { assertReducedEventsLike(t, expect, reduced) }) - t.Run("with missing input and InputOrQueryBackend policy", func(t *testing.T) { + t.Run("with error during target loading", func(t *testing.T) { runner, emitter := newRunnerForTesting() fake := fakeSuccessfulDeps() - fake.Builder.MockInputPolicy = func() model.InputPolicy { - return model.InputOrQueryBackend - } - runner.newSession = fake.NewSession - events := runAndCollect(runner, emitter) - reduced := reduceEventsKeysIgnoreLog(t, events) - expect := []eventKeyCount{ - {Key: eventTypeStatusQueued, Count: 1}, - {Key: eventTypeStatusStarted, Count: 1}, - {Key: eventTypeStatusProgress, Count: 3}, - {Key: eventTypeStatusGeoIPLookup, Count: 1}, - {Key: eventTypeStatusResolverLookup, Count: 1}, - {Key: eventTypeFailureStartup, Count: 1}, - {Key: eventTypeStatusEnd, Count: 1}, - } - assertReducedEventsLike(t, expect, reduced) - }) - - t.Run("with missing input and InputStrictlyRequired policy", func(t *testing.T) { - runner, emitter := newRunnerForTesting() - fake := fakeSuccessfulDeps() - fake.Builder.MockInputPolicy = func() model.InputPolicy { - return model.InputStrictlyRequired - } - runner.newSession = fake.NewSession - events := runAndCollect(runner, emitter) - reduced := reduceEventsKeysIgnoreLog(t, events) - expect := []eventKeyCount{ - {Key: eventTypeStatusQueued, Count: 1}, - {Key: eventTypeStatusStarted, Count: 1}, - {Key: eventTypeStatusProgress, Count: 3}, - {Key: eventTypeStatusGeoIPLookup, Count: 1}, - {Key: eventTypeStatusResolverLookup, Count: 1}, - {Key: eventTypeFailureStartup, Count: 1}, - {Key: eventTypeStatusEnd, Count: 1}, - } - assertReducedEventsLike(t, expect, reduced) - }) - - t.Run("with InputOrStaticDefault policy and experiment with no static input", - func(t *testing.T) { - runner, emitter := newRunnerForTesting() - runner.settings.Name = "Antani" // no input for this experiment - fake := fakeSuccessfulDeps() - fake.Builder.MockInputPolicy = func() model.InputPolicy { - return model.InputOrStaticDefault - } - runner.newSession = fake.NewSession - events := runAndCollect(runner, emitter) - reduced := reduceEventsKeysIgnoreLog(t, events) - expect := []eventKeyCount{ - {Key: eventTypeStatusQueued, Count: 1}, - {Key: eventTypeStatusStarted, Count: 1}, - {Key: eventTypeStatusProgress, Count: 3}, - {Key: eventTypeStatusGeoIPLookup, Count: 1}, - {Key: eventTypeStatusResolverLookup, Count: 1}, - {Key: eventTypeFailureStartup, Count: 1}, - {Key: eventTypeStatusEnd, Count: 1}, + fake.Builder.MockNewTargetLoader = func(config *model.ExperimentTargetLoaderConfig) model.ExperimentTargetLoader { + return &mocks.ExperimentTargetLoader{ + MockLoad: func(ctx context.Context) ([]model.ExperimentTarget, error) { + return nil, errors.New("mocked error") + }, } - assertReducedEventsLike(t, expect, reduced) - }) - - t.Run("with InputNone policy and provided input", func(t *testing.T) { - runner, emitter := newRunnerForTesting() - runner.settings.Inputs = append(runner.settings.Inputs, "https://x.org/") - fake := fakeSuccessfulDeps() - fake.Builder.MockInputPolicy = func() model.InputPolicy { - return model.InputNone } runner.newSession = fake.NewSession events := runAndCollect(runner, emitter) @@ -462,12 +417,9 @@ func TestTaskRunnerRun(t *testing.T) { assertReducedEventsLike(t, expect, reduced) }) - t.Run("with success and InputNone policy", func(t *testing.T) { + t.Run("with success and just a single entry", func(t *testing.T) { runner, emitter := newRunnerForTesting() fake := fakeSuccessfulDeps() - fake.Builder.MockInputPolicy = func() model.InputPolicy { - return model.InputNone - } runner.newSession = fake.NewSession events := runAndCollect(runner, emitter) reduced := reduceEventsKeysIgnoreLog(t, events) @@ -488,12 +440,9 @@ func TestTaskRunnerRun(t *testing.T) { assertReducedEventsLike(t, expect, reduced) }) - t.Run("with measurement failure and InputNone policy", func(t *testing.T) { + t.Run("with failure and just a single entry", func(t *testing.T) { runner, emitter := newRunnerForTesting() fake := fakeSuccessfulDeps() - fake.Builder.MockInputPolicy = func() model.InputPolicy { - return model.InputNone - } fake.Experiment.MockMeasureWithContext = func(ctx context.Context, target model.ExperimentTarget) (measurement *model.Measurement, err error) { return nil, errors.New("preconditions error") } @@ -521,9 +470,6 @@ func TestTaskRunnerRun(t *testing.T) { // which is what was happening in the above referenced issue. runner, emitter := newRunnerForTesting() fake := fakeSuccessfulDeps() - fake.Builder.MockInputPolicy = func() model.InputPolicy { - return model.InputNone - } fake.Experiment.MockMeasureWithContext = func(ctx context.Context, target model.ExperimentTarget) (measurement *model.Measurement, err error) { return nil, errors.New("preconditions error") } @@ -546,62 +492,25 @@ func TestTaskRunnerRun(t *testing.T) { {Key: eventTypeStatusEnd, Count: 1}, } assertReducedEventsLike(t, expect, reduced) + // TODO(bassosimone): we should probably extend this test to + // make sure we're including the annotation as well }) - t.Run("with success and InputStrictlyRequired", func(t *testing.T) { - runner, emitter := newRunnerForTesting() - runner.settings.Inputs = []string{"a", "b", "c", "d"} - fake := fakeSuccessfulDeps() - fake.Builder.MockInputPolicy = func() model.InputPolicy { - return model.InputStrictlyRequired - } - runner.newSession = fake.NewSession - events := runAndCollect(runner, emitter) - reduced := reduceEventsKeysIgnoreLog(t, events) - expect := []eventKeyCount{ - {Key: eventTypeStatusQueued, Count: 1}, - {Key: eventTypeStatusStarted, Count: 1}, - {Key: eventTypeStatusProgress, Count: 3}, - {Key: eventTypeStatusGeoIPLookup, Count: 1}, - {Key: eventTypeStatusResolverLookup, Count: 1}, - {Key: eventTypeStatusProgress, Count: 1}, - {Key: eventTypeStatusReportCreate, Count: 1}, - // - {Key: eventTypeStatusMeasurementStart, Count: 1}, - {Key: eventTypeStatusProgress, Count: 1}, - {Key: eventTypeMeasurement, Count: 1}, - {Key: eventTypeStatusMeasurementSubmission, Count: 1}, - {Key: eventTypeStatusMeasurementDone, Count: 1}, - // - {Key: eventTypeStatusMeasurementStart, Count: 1}, - {Key: eventTypeStatusProgress, Count: 1}, - {Key: eventTypeMeasurement, Count: 1}, - {Key: eventTypeStatusMeasurementSubmission, Count: 1}, - {Key: eventTypeStatusMeasurementDone, Count: 1}, - // - {Key: eventTypeStatusMeasurementStart, Count: 1}, - {Key: eventTypeStatusProgress, Count: 1}, - {Key: eventTypeMeasurement, Count: 1}, - {Key: eventTypeStatusMeasurementSubmission, Count: 1}, - {Key: eventTypeStatusMeasurementDone, Count: 1}, - // - {Key: eventTypeStatusMeasurementStart, Count: 1}, - {Key: eventTypeStatusProgress, Count: 1}, - {Key: eventTypeMeasurement, Count: 1}, - {Key: eventTypeStatusMeasurementSubmission, Count: 1}, - {Key: eventTypeStatusMeasurementDone, Count: 1}, - // - {Key: eventTypeStatusEnd, Count: 1}, - } - assertReducedEventsLike(t, expect, reduced) - }) - - t.Run("with success and InputOptional and input", func(t *testing.T) { + t.Run("with success and more than one entry", func(t *testing.T) { + inputs := []string{"a", "b", "c", "d"} runner, emitter := newRunnerForTesting() - runner.settings.Inputs = []string{"a", "b", "c", "d"} + runner.settings.Inputs = inputs // this is basically ignored because we override MockLoad fake := fakeSuccessfulDeps() - fake.Builder.MockInputPolicy = func() model.InputPolicy { - return model.InputOptional + fake.Builder.MockNewTargetLoader = func(config *model.ExperimentTargetLoaderConfig) model.ExperimentTargetLoader { + return &mocks.ExperimentTargetLoader{ + MockLoad: func(ctx context.Context) (targets []model.ExperimentTarget, err error) { + // We need to mimic what would happen when settings.Inputs is explicitly provided + for _, input := range inputs { + targets = append(targets, model.NewOOAPIURLInfoWithDefaultCategoryAndCountry(input)) + } + return + }, + } } runner.newSession = fake.NewSession events := runAndCollect(runner, emitter) @@ -644,77 +553,22 @@ func TestTaskRunnerRun(t *testing.T) { assertReducedEventsLike(t, expect, reduced) }) - t.Run("with success and InputOptional and no input", func(t *testing.T) { - runner, emitter := newRunnerForTesting() - fake := fakeSuccessfulDeps() - fake.Builder.MockInputPolicy = func() model.InputPolicy { - return model.InputOptional - } - runner.newSession = fake.NewSession - events := runAndCollect(runner, emitter) - reduced := reduceEventsKeysIgnoreLog(t, events) - expect := []eventKeyCount{ - {Key: eventTypeStatusQueued, Count: 1}, - {Key: eventTypeStatusStarted, Count: 1}, - {Key: eventTypeStatusProgress, Count: 3}, - {Key: eventTypeStatusGeoIPLookup, Count: 1}, - {Key: eventTypeStatusResolverLookup, Count: 1}, - {Key: eventTypeStatusProgress, Count: 1}, - {Key: eventTypeStatusReportCreate, Count: 1}, - // - {Key: eventTypeStatusMeasurementStart, Count: 1}, - {Key: eventTypeMeasurement, Count: 1}, - {Key: eventTypeStatusMeasurementSubmission, Count: 1}, - {Key: eventTypeStatusMeasurementDone, Count: 1}, - // - {Key: eventTypeStatusEnd, Count: 1}, - } - assertReducedEventsLike(t, expect, reduced) - }) - - t.Run("with success and InputOrStaticDefault", func(t *testing.T) { - experimentName := "DNSCheck" - runner, emitter := newRunnerForTesting() - runner.settings.Name = experimentName - fake := fakeSuccessfulDeps() - fake.Builder.MockInputPolicy = func() model.InputPolicy { - return model.InputOrStaticDefault - } - runner.newSession = fake.NewSession - events := runAndCollect(runner, emitter) - reduced := reduceEventsKeysIgnoreLog(t, events) - expect := []eventKeyCount{ - {Key: eventTypeStatusQueued, Count: 1}, - {Key: eventTypeStatusStarted, Count: 1}, - {Key: eventTypeStatusProgress, Count: 3}, - {Key: eventTypeStatusGeoIPLookup, Count: 1}, - {Key: eventTypeStatusResolverLookup, Count: 1}, - {Key: eventTypeStatusProgress, Count: 1}, - {Key: eventTypeStatusReportCreate, Count: 1}, - } - allEntries, err := targetloading.StaticBareInputForExperiment(experimentName) - if err != nil { - t.Fatal(err) - } - // write the correct entries for each expected measurement. - for idx := 0; idx < len(allEntries); idx++ { - expect = append(expect, eventKeyCount{Key: eventTypeStatusMeasurementStart, Count: 1}) - expect = append(expect, eventKeyCount{Key: eventTypeStatusProgress, Count: 1}) - expect = append(expect, eventKeyCount{Key: eventTypeMeasurement, Count: 1}) - expect = append(expect, eventKeyCount{Key: eventTypeStatusMeasurementSubmission, Count: 1}) - expect = append(expect, eventKeyCount{Key: eventTypeStatusMeasurementDone, Count: 1}) - } - expect = append(expect, eventKeyCount{Key: eventTypeStatusEnd, Count: 1}) - assertReducedEventsLike(t, expect, reduced) - }) - t.Run("with success and max runtime", func(t *testing.T) { + inputs := []string{"a", "b", "c", "d"} runner, emitter := newRunnerForTesting() - runner.settings.Inputs = []string{"a", "b", "c", "d"} + runner.settings.Inputs = inputs // this is basically ignored because we override MockLoad runner.settings.Options.MaxRuntime = 2 fake := fakeSuccessfulDeps() - fake.Builder.MockInputPolicy = func() model.InputPolicy { - return model.InputStrictlyRequired + fake.Builder.MockNewTargetLoader = func(config *model.ExperimentTargetLoaderConfig) model.ExperimentTargetLoader { + return &mocks.ExperimentTargetLoader{ + MockLoad: func(ctx context.Context) (targets []model.ExperimentTarget, err error) { + // We need to mimic what would happen when settings.Inputs is explicitly provided + for _, input := range inputs { + targets = append(targets, model.NewOOAPIURLInfoWithDefaultCategoryAndCountry(input)) + } + return + }, + } } fake.Experiment.MockMeasureWithContext = func(ctx context.Context, target model.ExperimentTarget) (measurement *model.Measurement, err error) { time.Sleep(1 * time.Second) @@ -750,12 +604,21 @@ func TestTaskRunnerRun(t *testing.T) { }) t.Run("with interrupted experiment", func(t *testing.T) { + inputs := []string{"a", "b", "c", "d"} runner, emitter := newRunnerForTesting() - runner.settings.Inputs = []string{"a", "b", "c", "d"} + runner.settings.Inputs = inputs // this is basically ignored because we override MockLoad runner.settings.Options.MaxRuntime = 2 fake := fakeSuccessfulDeps() - fake.Builder.MockInputPolicy = func() model.InputPolicy { - return model.InputStrictlyRequired + fake.Builder.MockNewTargetLoader = func(config *model.ExperimentTargetLoaderConfig) model.ExperimentTargetLoader { + return &mocks.ExperimentTargetLoader{ + MockLoad: func(ctx context.Context) (targets []model.ExperimentTarget, err error) { + // We need to mimic what would happen when settings.Inputs is explicitly provided + for _, input := range inputs { + targets = append(targets, model.NewOOAPIURLInfoWithDefaultCategoryAndCountry(input)) + } + return + }, + } } fake.Builder.MockInterruptible = func() bool { return true @@ -786,11 +649,23 @@ func TestTaskRunnerRun(t *testing.T) { }) t.Run("with measurement submission failure", func(t *testing.T) { + // Implementation note: this experiment needs a non-empty input otherwise the + // code will not emit a progress event when it finished measuring the input and + // we would be missing the eventTypeStatusProgress event. + inputs := []string{"a"} runner, emitter := newRunnerForTesting() - runner.settings.Inputs = []string{"a"} + runner.settings.Inputs = inputs // this is basically ignored because we override MockLoad fake := fakeSuccessfulDeps() - fake.Builder.MockInputPolicy = func() model.InputPolicy { - return model.InputStrictlyRequired + fake.Builder.MockNewTargetLoader = func(config *model.ExperimentTargetLoaderConfig) model.ExperimentTargetLoader { + return &mocks.ExperimentTargetLoader{ + MockLoad: func(ctx context.Context) (targets []model.ExperimentTarget, err error) { + // We need to mimic what would happen when settings.Inputs is explicitly provided + for _, input := range inputs { + targets = append(targets, model.NewOOAPIURLInfoWithDefaultCategoryAndCountry(input)) + } + return + }, + } } fake.Experiment.MockSubmitAndUpdateMeasurementContext = func(ctx context.Context, measurement *model.Measurement) error { return errors.New("cannot submit")