2021-02-03 14:42:51 +01:00
|
|
|
# Contributing to ooni/probe-cli
|
2021-02-02 12:05:47 +01:00
|
|
|
|
|
|
|
|
This is an open source project, and contributions are welcome! You are welcome
|
|
|
|
|
to open pull requests. An open pull request will be reviewed by a core
|
|
|
|
|
developer. The review may request you to apply changes. Once the assigned
|
|
|
|
|
reviewer is satisfied, they will merge the pull request.
|
|
|
|
|
|
|
|
|
|
## Opening issues
|
|
|
|
|
|
|
|
|
|
Please, before opening a new issue, check whether the issue or feature request
|
|
|
|
|
you want us to consider has not already been reported by someone else.
|
|
|
|
|
|
2021-02-03 19:29:12 +01:00
|
|
|
For new issues, please use: [github.com/ooni/probe](
|
|
|
|
|
https://github.com/ooni/probe/issues/new?labels=ooni/probe-cli&assignee=bassosimone).
|
2021-02-03 14:42:51 +01:00
|
|
|
|
2021-02-03 19:29:12 +01:00
|
|
|
Please, also check [github.com/ooni/probe-engine](
|
|
|
|
|
https://github.com/ooni/probe-engine) for legacy issues. This is
|
2021-02-03 14:42:51 +01:00
|
|
|
the repository where the measurement engine previously was located.
|
|
|
|
|
|
2021-02-02 12:05:47 +01:00
|
|
|
## PR requirements
|
|
|
|
|
|
|
|
|
|
Every pull request that introduces new functionality should feature
|
|
|
|
|
comprehensive test coverage. Any pull request that modifies existing
|
|
|
|
|
functionality should pass existing tests. What's more, any new pull
|
|
|
|
|
request that modifies existing functionality should not decrease the
|
|
|
|
|
existing code coverage.
|
|
|
|
|
|
|
|
|
|
Long-running tests should be skipped when running tests in short mode
|
|
|
|
|
using `go test -short`. We prefer external testing to internal
|
|
|
|
|
testing. We generally have a file called `foo_test.go` with tests
|
|
|
|
|
for every `foo.go` file. Sometimes we separate long running
|
|
|
|
|
integration tests in a `foo_integration_test.go` file.
|
|
|
|
|
|
|
|
|
|
If there is a top-level DESIGN.md document, make sure such document is
|
|
|
|
|
kept in sync with code changes you have applied.
|
|
|
|
|
|
|
|
|
|
Do not submit large PRs. A reviewer can best service your PR if the
|
|
|
|
|
code changes are around 200-600 lines. (It is okay to add more changes
|
|
|
|
|
afterwards, if the reviewer asks you to do more work; the key point
|
|
|
|
|
here is that the PR should be reasonably sized when the review starts.)
|
|
|
|
|
|
|
|
|
|
In this vein, we'd rather structure a complex issue as a sequence of
|
|
|
|
|
small PRs, than have a single large PR address it all.
|
|
|
|
|
|
|
|
|
|
As a general rule, a PR is reviewed by reading the whole diff. Let us
|
|
|
|
|
know if you want us to read each diff individually, if you think that's
|
|
|
|
|
functional to better understand your changes.
|
|
|
|
|
|
|
|
|
|
## Code style requirements
|
|
|
|
|
|
|
|
|
|
Please, use `go fmt`, `go vet`, and `golint` to check your code
|
|
|
|
|
contribution before submitting a pull request. Make sure your code
|
|
|
|
|
is documented. At the minimum document all the exported symbols.
|
|
|
|
|
|
|
|
|
|
Make sure you commit `go.mod` and `go.sum` changes. Make sure you
|
|
|
|
|
run `go mod tidy` to minimize such changes.
|
|
|
|
|
|
refactor: flatten and separate (#353)
* refactor(atomicx): move outside the engine package
After merging probe-engine into probe-cli, my impression is that we have
too much unnecessary nesting of packages in this repository.
The idea of this commit and of a bunch of following commits will instead
be to reduce the nesting and simplify the structure.
While there, improve the documentation.
* fix: always use the atomicx package
For consistency, never use sync/atomic and always use ./internal/atomicx
so we can just grep and make sure we're not risking to crash if we make
a subtle mistake on a 32 bit platform.
While there, mention in the contributing guidelines that we want to
always prefer the ./internal/atomicx package over sync/atomic.
* fix(atomicx): remove unnecessary constructor
We don't need a constructor here. The default constructed `&Int64{}`
instance is already usable and the constructor does not add anything to
what we are doing, rather it just creates extra confusion.
* cleanup(atomicx): we are not using Float64
Because atomicx.Float64 is unused, we can safely zap it.
* cleanup(atomicx): simplify impl and improve tests
We can simplify the implementation by using defer and by letting
the Load() method call Add(0).
We can improve tests by making many goroutines updated the
atomic int64 value concurrently.
* refactor(fsx): can live in the ./internal pkg
Let us reduce the amount of nesting. While there, ensure that the
package only exports the bare minimum, and improve the documentation
of the tests, to ease reading the code.
* refactor: move runtimex to ./internal
* refactor: move shellx into the ./internal package
While there, remove unnecessary dependency between packages.
While there, specify in the contributing guidelines that
one should use x/sys/execabs instead of os/exec.
* refactor: move ooapi into the ./internal pkg
* refactor(humanize): move to ./internal and better docs
* refactor: move platform to ./internal
* refactor(randx): move to ./internal
* refactor(multierror): move into the ./internal pkg
* refactor(kvstore): all kvstores in ./internal
Rather than having part of the kvstore inside ./internal/engine/kvstore
and part in ./internal/engine/kvstore.go, let us put every piece of code
that is kvstore related into the ./internal/kvstore package.
* fix(kvstore): always return ErrNoSuchKey on Get() error
It should help to use the kvstore everywhere removing all the
copies that are lingering around the tree.
* sessionresolver: make KVStore mandatory
Simplifies implementation. While there, use the ./internal/kvstore
package rather than having our private implementation.
* fix(ooapi): use the ./internal/kvstore package
* fix(platform): better documentation
2021-06-04 10:34:18 +02:00
|
|
|
## Implementation requirements
|
|
|
|
|
|
2021-06-04 11:39:00 +02:00
|
|
|
- use `./internal/atomicx` rather than `atomic/sync`
|
|
|
|
|
|
2021-06-04 14:02:18 +02:00
|
|
|
- do not use `os/exec`, use `x/sys/execabs` or `./internal/shellx`
|
refactor: flatten and separate (#353)
* refactor(atomicx): move outside the engine package
After merging probe-engine into probe-cli, my impression is that we have
too much unnecessary nesting of packages in this repository.
The idea of this commit and of a bunch of following commits will instead
be to reduce the nesting and simplify the structure.
While there, improve the documentation.
* fix: always use the atomicx package
For consistency, never use sync/atomic and always use ./internal/atomicx
so we can just grep and make sure we're not risking to crash if we make
a subtle mistake on a 32 bit platform.
While there, mention in the contributing guidelines that we want to
always prefer the ./internal/atomicx package over sync/atomic.
* fix(atomicx): remove unnecessary constructor
We don't need a constructor here. The default constructed `&Int64{}`
instance is already usable and the constructor does not add anything to
what we are doing, rather it just creates extra confusion.
* cleanup(atomicx): we are not using Float64
Because atomicx.Float64 is unused, we can safely zap it.
* cleanup(atomicx): simplify impl and improve tests
We can simplify the implementation by using defer and by letting
the Load() method call Add(0).
We can improve tests by making many goroutines updated the
atomic int64 value concurrently.
* refactor(fsx): can live in the ./internal pkg
Let us reduce the amount of nesting. While there, ensure that the
package only exports the bare minimum, and improve the documentation
of the tests, to ease reading the code.
* refactor: move runtimex to ./internal
* refactor: move shellx into the ./internal package
While there, remove unnecessary dependency between packages.
While there, specify in the contributing guidelines that
one should use x/sys/execabs instead of os/exec.
* refactor: move ooapi into the ./internal pkg
* refactor(humanize): move to ./internal and better docs
* refactor: move platform to ./internal
* refactor(randx): move to ./internal
* refactor(multierror): move into the ./internal pkg
* refactor(kvstore): all kvstores in ./internal
Rather than having part of the kvstore inside ./internal/engine/kvstore
and part in ./internal/engine/kvstore.go, let us put every piece of code
that is kvstore related into the ./internal/kvstore package.
* fix(kvstore): always return ErrNoSuchKey on Get() error
It should help to use the kvstore everywhere removing all the
copies that are lingering around the tree.
* sessionresolver: make KVStore mandatory
Simplifies implementation. While there, use the ./internal/kvstore
package rather than having our private implementation.
* fix(ooapi): use the ./internal/kvstore package
* fix(platform): better documentation
2021-06-04 10:34:18 +02:00
|
|
|
|
2021-06-04 14:02:18 +02:00
|
|
|
- use `./internal/fsx.OpenFile` when you need to open a file
|
refactor: flatten and separate (#353)
* refactor(atomicx): move outside the engine package
After merging probe-engine into probe-cli, my impression is that we have
too much unnecessary nesting of packages in this repository.
The idea of this commit and of a bunch of following commits will instead
be to reduce the nesting and simplify the structure.
While there, improve the documentation.
* fix: always use the atomicx package
For consistency, never use sync/atomic and always use ./internal/atomicx
so we can just grep and make sure we're not risking to crash if we make
a subtle mistake on a 32 bit platform.
While there, mention in the contributing guidelines that we want to
always prefer the ./internal/atomicx package over sync/atomic.
* fix(atomicx): remove unnecessary constructor
We don't need a constructor here. The default constructed `&Int64{}`
instance is already usable and the constructor does not add anything to
what we are doing, rather it just creates extra confusion.
* cleanup(atomicx): we are not using Float64
Because atomicx.Float64 is unused, we can safely zap it.
* cleanup(atomicx): simplify impl and improve tests
We can simplify the implementation by using defer and by letting
the Load() method call Add(0).
We can improve tests by making many goroutines updated the
atomic int64 value concurrently.
* refactor(fsx): can live in the ./internal pkg
Let us reduce the amount of nesting. While there, ensure that the
package only exports the bare minimum, and improve the documentation
of the tests, to ease reading the code.
* refactor: move runtimex to ./internal
* refactor: move shellx into the ./internal package
While there, remove unnecessary dependency between packages.
While there, specify in the contributing guidelines that
one should use x/sys/execabs instead of os/exec.
* refactor: move ooapi into the ./internal pkg
* refactor(humanize): move to ./internal and better docs
* refactor: move platform to ./internal
* refactor(randx): move to ./internal
* refactor(multierror): move into the ./internal pkg
* refactor(kvstore): all kvstores in ./internal
Rather than having part of the kvstore inside ./internal/engine/kvstore
and part in ./internal/engine/kvstore.go, let us put every piece of code
that is kvstore related into the ./internal/kvstore package.
* fix(kvstore): always return ErrNoSuchKey on Get() error
It should help to use the kvstore everywhere removing all the
copies that are lingering around the tree.
* sessionresolver: make KVStore mandatory
Simplifies implementation. While there, use the ./internal/kvstore
package rather than having our private implementation.
* fix(ooapi): use the ./internal/kvstore package
* fix(platform): better documentation
2021-06-04 10:34:18 +02:00
|
|
|
|
2021-02-02 12:05:47 +01:00
|
|
|
## Code testing requirements
|
|
|
|
|
|
|
|
|
|
Make sure all tests pass with `go test -race ./...` run from the
|
|
|
|
|
top-level directory of this repository.
|
|
|
|
|
|
|
|
|
|
## Writing a new OONI experiment
|
|
|
|
|
|
|
|
|
|
When you are implementing a new experiment (aka nettest), make sure
|
|
|
|
|
you have read the relevant spec from the [ooni/spec](
|
|
|
|
|
https://github.com/ooni/spec) repository. If the spec is missing,
|
|
|
|
|
please help the pull request reviewer to create it. If the spec is
|
|
|
|
|
not clear, please let us know during the review.
|
|
|
|
|
|
|
|
|
|
When you write a new experiment, keep the measurement phase and the
|
|
|
|
|
results analysis phases as separate functions. This helps us a lot
|
|
|
|
|
to write better unit tests for our code.
|
|
|
|
|
|
|
|
|
|
To get a sense of what we expect from an experiment, see:
|
|
|
|
|
|
2021-02-03 14:42:51 +01:00
|
|
|
- the internal/engine/experiment/example experiment
|
2021-02-02 12:05:47 +01:00
|
|
|
|
2021-02-03 14:42:51 +01:00
|
|
|
- the internal/engine/experiment/webconnectivity experiment
|
2021-02-02 12:05:47 +01:00
|
|
|
|
|
|
|
|
Thank you!
|