Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add a Go tag to test ooniprobe without network #2426

Closed
CyberTailor opened this issue Mar 12, 2023 · 2 comments · Fixed by ooni/probe-cli#1352
Closed

Add a Go tag to test ooniprobe without network #2426

CyberTailor opened this issue Mar 12, 2023 · 2 comments · Fixed by ooni/probe-cli#1352
Assignees
@bassosimone
Labels
bug Something isn't working ooni/probe-cli ooni/probe-engine priority/low user feedback requests that have been added to the backlog as a direct result of user feedback or testing

Comments

@CyberTailor
Copy link

Network tests can be flaky so most packagers want to disable them. Alternatively, mocks can be provided instead.
@bassosimone bassosimone self-assigned this Mar 16, 2023
@bassosimone
Copy link
Contributor

bassosimone commented Mar 16, 2023 •
edited
Loading

Network tests can be flaky so most packagers want to disable them.

I agree. I think you can already skip most if not all the flaky tests using go test -short ./.... Did you already try that?

Alternatively, mocks can be provided instead.

We are working on using an userspace TCP/IP network stack for most tests, which is partially motivated by solving your use case and partially motivated by having more predictable integration tests in censored environments.

(We just merged initial support for that in ooni/probe-cli#1121.)

@bassosimone bassosimone added bug Something isn't working user feedback requests that have been added to the backlog as a direct result of user feedback or testing ooni/probe-cli ooni/probe-engine labels Mar 16, 2023
@bassosimone bassosimone mentioned this issue Oct 9, 2023
Merged
@bassosimone
Copy link
Contributor

@CyberTailor I think we mostly finished this work. Once I merge ooni/probe-cli#1352 there should be the guarantee that go test -short only uses either the localhost or the testing-only emulated network where we use gvisor. The ooni/probe-cli#1352 PR changes tests such that we run go test -short using a separate network namespace with only localhost, which means that likely we're not going to break this in the future.

bassosimone added a commit to ooni/probe-cli that referenced this issue Oct 9, 2023
@bassosimone
refactor: go test -short does not use the host network (#1352)
2edeafa 
This diff refactors the codebase so that we avoid using the host network
when running `go test -short`. This change is good in general, because
now coverage tells us the amount of code we're covering without
depending on interactions with an existing network, which means these
tests behave in the same way in ~any place.

I expect a coverage drop from this PR, because there's some coverage
made with integration testing (if we consider integration tests the
tests that require the host network interface with uncensored internet
access).

To make this happen, I needed to modify the `quictesting` package (now
moved to toplevel and renamed `testingquic`) such that it attempts to
get a known-to-work-well endpoint for QUIC _only_ when the developer
using the package really needs it, rather than on import. Before doing
this, there were several tests that panicked because `quictesting` could
not figure out which IP address to use when you disable the WiFi or run
inside another netns. Now we only figure this IP address out the first
time a test requires us to give it either the domain or the endpoint
that we should use.

To be sure we continue to honour the promise that `go test -short` does
not use the host network, I needed to refactor the CI such that we
measure coverage inside a new network namespace with only localhost
support. I think this compromise is acceptable, since the original ask
was to avoid flaky network tests (see
ooni/probe#2426).

Because of this change in how we run the coverage checks, I am
tentatively enabling also running all tests for pull requests, otherwise
we don't know if a contribution breaks tests using the network.
Hopefully, we should be fine because we are caching previous runs, so a
bunch of tests should already be cached.

Closes ooni/probe#2426.

While there, enable again some backend integration tests, and close
ooni/probe#2539.
Murphy-OrangeMud pushed a commit to Murphy-OrangeMud/probe-cli that referenced this issue Feb 13, 2024
@bassosimone @Murphy-OrangeMud
refactor: go test -short does not use the host network (ooni#1352)
a3a0eb7 
This diff refactors the codebase so that we avoid using the host network
when running `go test -short`. This change is good in general, because
now coverage tells us the amount of code we're covering without
depending on interactions with an existing network, which means these
tests behave in the same way in ~any place.

I expect a coverage drop from this PR, because there's some coverage
made with integration testing (if we consider integration tests the
tests that require the host network interface with uncensored internet
access).

To make this happen, I needed to modify the `quictesting` package (now
moved to toplevel and renamed `testingquic`) such that it attempts to
get a known-to-work-well endpoint for QUIC _only_ when the developer
using the package really needs it, rather than on import. Before doing
this, there were several tests that panicked because `quictesting` could
not figure out which IP address to use when you disable the WiFi or run
inside another netns. Now we only figure this IP address out the first
time a test requires us to give it either the domain or the endpoint
that we should use.

To be sure we continue to honour the promise that `go test -short` does
not use the host network, I needed to refactor the CI such that we
measure coverage inside a new network namespace with only localhost
support. I think this compromise is acceptable, since the original ask
was to avoid flaky network tests (see
ooni/probe#2426).

Because of this change in how we run the coverage checks, I am
tentatively enabling also running all tests for pull requests, otherwise
we don't know if a contribution breaks tests using the network.
Hopefully, we should be fine because we are caching previous runs, so a
bunch of tests should already be cached.

Closes ooni/probe#2426.

While there, enable again some backend integration tests, and close
ooni/probe#2539.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bassosimone bassosimone

Labels
bug Something isn't working ooni/probe-cli ooni/probe-engine priority/low user feedback requests that have been added to the backlog as a direct result of user feedback or testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

refactor: go test -short does not use the host network ooni/probe-cli
2 participants
@bassosimone @CyberTailor