## Checklist
- [x] I have read the [contribution guidelines](https://github.com/ooni/probe-cli/blob/master/CONTRIBUTING.md)
- [x] reference issue for this pull request: https://github.com/ooni/probe/issues/1885
- [x] related ooni/spec pull request: N/A
Location of the issue tracker: https://github.com/ooni/probe
## Description
This PR contains a set of changes to move important interfaces and data types into the `./internal/model` package.
The criteria for including an interface or data type in here is roughly that the type should be important and used by several packages. We are especially interested to move more interfaces here to increase modularity.
An additional side effect is that, by reading this package, one should be able to understand more quickly how different parts of the codebase interact with each other.
This is what I want to move in `internal/model`:
- [x] most important interfaces from `internal/netxlite`
- [x] everything that was previously part of `internal/engine/model`
- [x] mocks from `internal/netxlite/mocks` should also be moved in here as a subpackage
* [forwardport] fix(oonimkall): make logger used by tasks unit testable (#623)
This diff forward ports e4b04642c51e7461728b25941624e1b97ef0ec83.
Reference issue: https://github.com/ooni/probe/issues/1903
* [forwardport] feat(oonimkall): improve taskEmitter testability (#624)
This diff forward ports 3e0f01a389c1f4cdd7878ec151aff91870a0bdff.
1. rename eventemitter{,_test}.go => taskemitter{,_test}.go because
the new name is more proper after we merged the internal/task package
inside of the oonimkall package;
2. rename runner.go's `run` function to `runTask`;
3. modify `runTask` to use the new `taskEmitterUsingChan` abstraction
on which we will spend more works in a later point of this list;
4. introduce `runTaskWithEmitter` factory that is called by `runTask`
and allows us to more easily write unit tests;
5. acknowledge that `runner` was not using its `out` field;
6. use the new `taskEmitterWrapper` in `newRunner`;
7. acknowledge that `runnerCallbacks` could use a generic
`taskEmitter` as field type rather than a specific type;
8. rewrite tests to use `runTaskWithEmitter` which leads to
simpler code that does not require a goroutine;
9. acknowledge that the code has been ignoring the `DisabledEvents`
settings for quite some time, so stop supporting it;
10. refactor the `taskEmitter` implementation to be like:
1. we still have the `taskEmitter` interface;
2. `taskEmitterUsingChan` wraps the channel and allows for
emitting events using the channel;
3. `taskEmitterUsingChan` owns an `eof` channel that is
closed by `Close` (which is idempotent) and signals we
should be stop emitting;
4. make sure `runTask` creates a `taskEmitterUsingChan`
and calls its `Close` method when done;
5. completely remove the code for disabling events
since the code was actually ignoring the stting;
6. add a `taskEmitterWrapper` that adds common functions
for emitting events to _any_ `taskWrapper`;
7. write unit tests for `taskEmitterUsingChan` and
for `taskEmitterWrapper`;
11. acknowledge that the abstraction we need for testing is
actually a thread-safe thing that collects events into a
vector containing events and refactor all tests accordingly.
See https://github.com/ooni/probe/issues/1903
* [forwardport] refactor(oonimkall): make the runner unit-testable (#625)
This diff forward ports 9423947faf6980d92d2fe67efe3829e8fef76586.
See https://github.com/ooni/probe/issues/1903
* [forwardport] feat(oonimkall): write unit tests for the runner component (#626)
This diff forward ports 35dd0e3788b8fa99c541452bbb5e0ae4871239e1.
Forward porting note: compared to 35dd0e3788b8fa99c541452bbb5e0ae4871239e1,
the diff I'm committing here is slightly different. In `master` we do not
have the case where a measurement fails and a measurement is returned, thus
I needed to adapt the test to become like this:
```diff
diff --git a/pkg/oonimkall/runner_internal_test.go b/pkg/oonimkall/runner_internal_test.go
index 334b574..84c7436 100644
--- a/pkg/oonimkall/runner_internal_test.go
+++ b/pkg/oonimkall/runner_internal_test.go
@@ -568,15 +568,6 @@ func TestTaskRunnerRun(t *testing.T) {
}, {
Key: failureMeasurement,
Count: 1,
- }, {
- Key: measurement,
- Count: 1,
- }, {
- Key: statusMeasurementSubmission,
- Count: 1,
- }, {
- Key: statusMeasurementDone,
- Count: 1,
}, {
Key: statusEnd,
Count: 1,
```
I still need to write more assertions for each emitted event
but the code we've here is already a great starting point.
See https://github.com/ooni/probe/issues/1903
* [forwardport] refactor(oonimkall): merge files, use proper names, zap unneeded integration tests (#627)
This diff forward ports f894427d24edc9a03fc78306d0093e7b51c46c25.
Forward porting note: this diff is slightly different from the original
mentioned above because it carries forward changes mentioned in the
previous diff caused by a different way of handling a failed measurement
in the master branch compared to the release/3.11 branch.
Move everything that looked like "task's model" inside of the
taskmodel.go file, for consistency.
Make sure it's clear some variables are event types.
Rename the concrete `runner` as `runnerForTask`.
Also, remove now-unnecessary (and flaky!) integration tests
for the `runnerForTask` type.
While there, notice there were wrong URLs that were generated
during the probe-engine => probe-cli move and fix them.
See https://github.com/ooni/probe/issues/1903
* [forwardport] refactor(oonimkall): we can simplify StartTask tests (#628)
This diff forward ports dcf2986c2032d8185d58d24130a7f2c2d61ef2fb.
* refactor(oonimkall): we can simplify StartTask tests
We have enough checks for runnerForTask. So we do not need to
duplicate them when checking for StartTask.
While there, refactor how we start tasks to remove the need for
extra runner functions.
This is the objective I wanted to achieve for oonimkall:
1. less duplicate tests, and
2. more unit tests (which are less flaky)
At this point, we're basically done (pending forwardporting to
master) with https://github.com/ooni/probe/issues/1903.
* fix(oonimkall): TestStartTaskGood shouldn't cancel the test
This creates a race condition where the test may fail if we cannot
complete the whole "Example" test in less than one second.
This should explain the build failures I've seen so far and why
I didn't see those failures when running locally.
When preparing a tutorial for netxlite, I figured it is easier
to tell people "hey, this is the package you should use for all
low-level networking stuff" rather than introducing people to
a set of packages working together where some piece of functionality
is here and some other piece is there.
Part of https://github.com/ooni/probe/issues/1591
The legacy part for now is internal/errorsx. It will stay there until
I figure out whether it also needs some extra bug fixing.
The good part is now in internal/netxlite/errorsx and contains all the
logic for mapping errors. We need to further improve upon this logic
by writing more thorough integration tests for QUIC.
We also need to copy the various dialer, conn, etc adapters that set
errors. We will put them inside netxlite and we will generate errors in
a way that is less crazy with respect to the major operation. (The
idea is to always wrap, given that now we measure in an incremental way
and we don't measure every operation together.)
Part of https://github.com/ooni/probe/issues/1591
* ongoing
* while there, make sure we test everything
* reorganize previous commit
* ensure we have reasonable coverage in session
The code in here would be better with unit tests. We have too many
integration tests and the tests overall are too slow. But it's also
true that I should not write a giant diff as part of this PR.
* fix(pkg.go.dev): import a subpackage containing the assets
We're trying to fix this issue that pkg.go.dev does not build.
Thanks to @hellais for this very neat idea! Let's keep our
fingers crossed and see whether it fixes!
* feat: use embedded geoip databases
Closes https://github.com/ooni/probe/issues/1372.
Work done as part of https://github.com/ooni/probe/issues/1369.
* fix(assetsx): add tests
* feat: simplify and just vendor uncompressed DBs
* remove tests that seems not necessary anymore
* fix: run go mod tidy
* Address https://github.com/ooni/probe-cli/pull/260/files#r605181364
* rewrite a test in a better way
* fix: gently cleanup the legacy assetsdir
Do not remove the whole directory with brute force. Just zap the
files whose name we know. Then attempt to delete the legacy directory
as well. If not empty, just fail. This is fine because it means the
user has stored other files inside the directory.
* fix: create .miniooni if missing
* feat: use go1.16 embedding for resources
We want to embed everything that can be easily embedded. We should, at a
minimum, replace the downloading of resources and bindata.
Ref: https://github.com/ooni/probe/issues/1367.
* fix: get rid of bindata and use go embed instead
* fix: start unbreaking some automatic tests
* fix: fetch resources as part of the mobile build
* fix: convert more stuff to go1.16
I still expect many breakages, but we'll fix them.
* fix: make the windows CI green
* fix: get resources before running QA
* fix: go1.16 uses modules by default
* hopefully fix all other outstanding issues
* fix(QA/telegram.py): add another DC IP address
* Apply suggestions from code review
This is how I did it:
1. `git clone https://github.com/ooni/probe-engine internal/engine`
2. ```
(cd internal/engine && git describe --tags)
v0.23.0
```
3. `nvim go.mod` (merging `go.mod` with `internal/engine/go.mod`
4. `rm -rf internal/.git internal/engine/go.{mod,sum}`
5. `git add internal/engine`
6. `find . -type f -name \*.go -exec sed -i 's@/ooni/probe-engine@/ooni/probe-cli/v3/internal/engine@g' {} \;`
7. `go build ./...` (passes)
8. `go test -race ./...` (temporary failure on RiseupVPN)
9. `go mod tidy`
10. this commit message
Once this piece of work is done, we can build a new version of `ooniprobe` that
is using `internal/engine` directly. We need to do more work to ensure all the
other functionality in `probe-engine` (e.g. making mobile packages) are still WAI.
Part of https://github.com/ooni/probe/issues/1335
