-
Notifications
You must be signed in to change notification settings - Fork 49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(oonimkall): use richer-input-aware target loader #1620
Conversation
This diff completes the set of preliminary richer input diffs. We build the TargetLoader using the ExperimentBuilder, which in turn uses a registry.Factory under the hood. This means that we can load targets for each experiment. Part of ooni/probe#2607
Conflicts: internal/oonirun/experiment.go internal/registry/dnscheck.go internal/targetloading/targetloading.go
This diff simplifies test code in pkg/oonimkall in preparation for further richer-input related changes. Part of ooni/probe#2607
Conflicts: internal/reflectx/reflectx.go internal/reflectx/reflectx_test.go internal/registry/factory_test.go
This diff, which is part of ooni/probe#2752, prints the event occurring when testing, which is useful for me to better understand what is going on while changing the package. Extracted from #1620.
This diff, which is part of ooni/probe#2752, prints the event occurring when testing, which is useful for me to better understand what is going on while changing the package. Extracted from #1620.
Conflicts: docs/design/dd-008-richer-input.md
No functional changes. Only adding newlines and comments.
return &mocks.ExperimentTargetLoader{ | ||
MockLoad: func(ctx context.Context) ([]model.ExperimentTarget, error) { | ||
return nil, errors.New("mocked error") | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because now we don't switch explicitly on the InputPolicy, we need just one test case for failing target loading; as a result, we remove several previously-needed test cases.
runner, emitter := newRunnerForTesting() | ||
fake := fakeSuccessfulDeps() | ||
fake.Builder.MockInputPolicy = func() model.InputPolicy { | ||
return model.InputNone | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need the input policy anymore. We just rely on mocking the target loader in fakeSuccessfulDeps
.
This is true for this test and several tests below it.
} | ||
return | ||
}, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like before, we can remove test cases because we treat experiments uniformly since we use a target loader.
// 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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function can become private because ./pkg/oonimkall
does not need it anymore.
} | ||
return reply, nil | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove this function, since no-one else is using it.
// 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plan shelved in favor of the better plan by @aanorbel: we emit CategoryCode
and CountryCode
during status.measurement_start
and the application updates its database table.
} | ||
return targets, nil | ||
}, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default we mock an experiment taking an empty entry (like dash, ndt, telegram, etc.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
This pull request fixes technical debt where
./pkg/oonimkall
ws using custom code to load experiments targets rather than using a (richer-input-aware) loader. We make sure to use a context with timeout when loading.Web Connectivity should continue to WAI on mobile after this change, given that the app is calling check-in itself and it is still a supported use case to execute an experiment with explicit input.
We also start preparing for simplifying both
./pkg/oonimkall
and the mobile apps, by providing the category code and the country code inside thestatus.measurement_start
event, which will allow mobile apps to update its URLs table in response to such event, rather than manually calling the check-in API and providing inputs to the engine.Closes ooni/probe#2764
Closes ooni/probe#2765