Add single process integration tests #2123
Conversation
divergentdave
force-pushed
the
david/single-process-integration-test
branch
from
b08e793 to
32dd4ca
Compare
integration_tests/src/janus.rs
Outdated
| config_file: PathBuf::new(), | ||
| database_password: None, | ||
| datastore_keys, | ||
| otlp_tracing_metadata: Vec::new(), | ||
| otlp_metrics_metadata: Vec::new(), |
There was a problem hiding this comment.
nit: could we provide Default on CommonBinaryOptions and do CommonBinaryOptions { datastore_keys, ..Default::default() }?
| AggregatorEndpointFragments::VirtualNetwork { path, .. } => path, | ||
| AggregatorEndpointFragments::Localhost { path } => path, | ||
| }; | ||
| Url::parse(&format!("http://127.0.0.1:{port}{path}")).unwrap() |
There was a problem hiding this comment.
nit: should we use http://localhost:{port}{path} here just in case someone only has an IPv6 localhost? Though I notice that this was hard-coded to 127.0.0.1 in the existing code this refactors, so maybe we deliberately forced this to use IPv4 to avoid port collisions across the protocols...
There was a problem hiding this comment.
Using localhost leads to problems when connecting to Docker container ports, because the same exposed container port may be published at different ephemeral ports on 127.0.0.1 and ::1. When you combine that with our HTTP client doing happy eyeballs on both addresses for localhost, that resulted in wires getting crossed between different concurrently running tests. Sticking with 127.0.0.1 here and Container::get_host_port_ipv4() elsewhere keeps things straight.
| @@ -56,3 +85,191 @@ impl<'a> Janus<'a> { | |||
| .get_host_port_ipv4(Aggregator::INTERNAL_SERVING_PORT) | |||
| } | |||
| } | |||
|
|
|||
| /// Represents a running Janus test instance in this process. | |||
| pub struct JanusInProcess { | |||
There was a problem hiding this comment.
There's loads of other tests (such as in janus_aggregator::aggregator::http_handlers) where we set up an in-process instance of Janus by calling janus_aggregator::aggregator::http_handlers::aggregator_handler. I wonder if we should adapt those to use this entry point, or otherwise unify them? Though I don't think that needs to happen in this PR.
There was a problem hiding this comment.
Using the handler directly in those tests means they can run without binding to a port, skip both sides of the HTTP protocol, and manipulate requests and responses in memory. (using trillium-testing) I think it makes sense to keep things as they are, because they each have a different area of interest.
divergentdave
force-pushed
the
david/single-process-integration-test
branch
from
32dd4ca to
2d194ef
Compare
This is part of #2081. This PR adds new integration tests that run both Janus aggregator instances in-process, relying on the existing ephemeral database support for Postgres. This also includes one test with an in-process Janus leader and a Daphne helper running in a container.
Note that the divviup-ts integration tests, or tests with Daphne as the leader, are not supported with in-process aggregators yet. That will require making connections from inside a container to TCP servers on the host, which may be doable using
host.docker.internalon Windows/Mac and--add-hostwithhost-gatewayon Linux, FWIW.