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.

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

test suite: httpx: request failed: 500 Internal Server Error #2539

Closed
bassosimone opened this issue Sep 22, 2023 · 0 comments · Fixed by ooni/probe-cli#1352
Closed

test suite: httpx: request failed: 500 Internal Server Error #2539

bassosimone opened this issue Sep 22, 2023 · 0 comments · Fixed by ooni/probe-cli#1352
Assignees
Labels
bug Something isn't working needs investigation This issue needs extra data and investigation ooni/probe-engine priority/high releaseBlocker This issue blocks releasing

Comments

@bassosimone
Copy link
Contributor

bassosimone commented Sep 22, 2023

See ooni/probe-cli#1297 (comment).

I am going to disable the failing tests while investigating.

@bassosimone bassosimone added bug Something isn't working priority/high needs investigation This issue needs extra data and investigation ooni/probe-engine labels Sep 22, 2023
@bassosimone bassosimone self-assigned this Sep 22, 2023
@bassosimone bassosimone changed the title login_test.go: TestMaybeLoginIdempotent fails with ams-pg-test.ooni.org TestMaybeLoginIdempotent: fails with ams-pg-test.ooni.org Sep 22, 2023
@bassosimone bassosimone changed the title TestMaybeLoginIdempotent: fails with ams-pg-test.ooni.org test suite: httpx: request failed: 500 Internal Server Error Sep 22, 2023
@bassosimone bassosimone added the releaseBlocker This issue blocks releasing label Sep 22, 2023
bassosimone added a commit to ooni/probe-cli that referenced this issue Sep 22, 2023
This diff makes the policy previously known as loadable and now know as
the static policy easier to use, by having a constructor that reads from
a key-value store and by passing it a fallback policy to use.

With this design, it should be possible to have code that uses the
static policy if applied, falling back to whatever policy we are
otherwise constructing into the NewNetwork constructor.

In a subsequent commit, I will hook this code into NewNetwork, so that
we can override the policy by changing the filesystem.

While there, notice several failures in the test suite and apply
workarounds (see ooni/probe#2539,
ooni/probe#2540,
ooni/probe#2541).

Part of ooni/probe#2531.
bassosimone added a commit to ooni/probe-cli that referenced this issue Oct 9, 2023
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
…#1297)

This diff makes the policy previously known as loadable and now know as
the static policy easier to use, by having a constructor that reads from
a key-value store and by passing it a fallback policy to use.

With this design, it should be possible to have code that uses the
static policy if applied, falling back to whatever policy we are
otherwise constructing into the NewNetwork constructor.

In a subsequent commit, I will hook this code into NewNetwork, so that
we can override the policy by changing the filesystem.

While there, notice several failures in the test suite and apply
workarounds (see ooni/probe#2539,
ooni/probe#2540,
ooni/probe#2541).

Part of ooni/probe#2531.
Murphy-OrangeMud pushed a commit to Murphy-OrangeMud/probe-cli that referenced this issue Feb 13, 2024
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
Labels
bug Something isn't working needs investigation This issue needs extra data and investigation ooni/probe-engine priority/high releaseBlocker This issue blocks releasing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant