RUST-979 Non-unified spec tests for load balancer support behavior #473
Conversation
| #[builder(default, setter(skip))] | ||
| #[serde(skip)] | ||
| pub(crate) allow_load_balanced: bool, |
There was a problem hiding this comment.
This particular gate key turned out to be more hassle than it was worth - it meant that attempting to parse options from test json would fail without jumping through a lot of hoops. The options validation now enforces that the load_balanced option is test-only, which seems a reasonable degree of protection.
| pub(crate) heartbeat_freq: Option<Duration>, | ||
|
|
||
| /// Disable server and SRV-polling monitor threads. | ||
| pub(crate) disable_monitoring_threads: bool, |
There was a problem hiding this comment.
Previously known as Topology::new_mocked.
| localhost_test_build_10gen(27018), | ||
| ])); | ||
| let topology = Topology::new(options).unwrap(); | ||
| RUNTIME.delay_for(rescan_interval * 2).await; |
There was a problem hiding this comment.
This is unfortunately long, but also mandated by the spec :(
| @@ -168,8 +94,6 @@ impl Topology { | |||
| handler.handle_topology_opening_event(event); | |||
| } | |||
|
|
|||
| let hosts: Vec<_> = options.hosts.drain(..).collect(); | |||
There was a problem hiding this comment.
This drain was actually causing subtle bugs because code below expected options.hosts to still be populated. As far as I can tell it's fine to remove - it'll just populate the topology server map with some temporary values that get immediately overwritten by the add_new_server loop below.
There was a problem hiding this comment.
Oh wow good catch. And yeah that seems fine.
| } else { | ||
| ClientOptions::parse(&test_file.uri).await | ||
| }; | ||
| async fn run_test(mut test_file: TestFile) { |
There was a problem hiding this comment.
Despite what github's diff would tell you, this was moved mostly unchanged, the two deltas being:
- the
load_balancedskip ResolvedOptionsmatching moved to a method
| // SRV polling is not done for load-balanced clusters. | ||
| #[cfg_attr(feature = "tokio-runtime", tokio::test)] | ||
| #[cfg_attr(feature = "async-std-runtime", async_std::test)] | ||
| async fn load_balanced_no_srv_polling() { |
There was a problem hiding this comment.
This has a different form than the other tests because those directly fire server updates, skipping the rest of the srv-polling machinery, whereas this one needs to test that the dns polling doesn't happen at all.
abr-egn
force-pushed
the
RUST-979/lb-non-unified-tests
branch
from
4227a9f to
c3e171d
Compare
| } else { | ||
| ClientOptions::parse(&test_file.uri).await | ||
| }; | ||
| // TODO RUST-979 unskip these tests |
There was a problem hiding this comment.
This will require an evergreen config for running tests against a loadbalancer, which will come in a follow-up PR.
src/sdam/srv_polling/test.rs
Outdated
| @@ -102,3 +104,24 @@ async fn timeout_error() { | |||
| async fn no_results() { | |||
| run_test(Ok(Vec::new()), DEFAULT_HOSTS.iter().cloned().collect()).await; | |||
| } | |||
|
|
|||
| // SRV polling is not done for load-balanced clusters. | |||
There was a problem hiding this comment.
Can we update this comment to refer to the spec that contains this prose test?
| @@ -168,8 +94,6 @@ impl Topology { | |||
| handler.handle_topology_opening_event(event); | |||
| } | |||
|
|
|||
| let hosts: Vec<_> = options.hosts.drain(..).collect(); | |||
There was a problem hiding this comment.
Oh wow good catch. And yeah that seems fine.
abr-egn
force-pushed
the
RUST-979/lb-non-unified-tests
branch
from
a93b58d to
bbedec2
Compare
RUST-979
This pulls in changes and additions to non-unified spec tests (and, in one case, implements a prose test) that validate various behaviors in support of load balancer functionality.