More tests #94
More tests #94
Conversation
Also did some code modifications to make code testable: 1. exposed UdpRendezvousClient, 2. implemented UdpRendezvousServer constructor with given UdpSock so that we could know what port will be used.
src/udp/mod.rs
Outdated
| @@ -1,4 +1,4 @@ | |||
| pub use self::hole_punch::UdpHolePunchMediator; | |||
| pub use self::hole_punch::{UdpHolePunchMediator, UdpRendezvousClient}; | |||
There was a problem hiding this comment.
I'm not comfortable exposing stuff only from a crate only for the purposes of testing. Maybe the testing code should be designed in some other way but the crate should not become a "debug-library" where all the internals are exposed to the user and not for any non-trivial reason.
src/udp/rendezvous_server.rs
Outdated
|
|
||
| /// Boot the UDP rendezvous server and use the given UDP socket to listen for incoming | ||
| /// requests. | ||
| pub fn start_with_sock(sock: UdpSock, ifc: &mut Interface, poll: &Poll) -> ::Res<Token> { |
There was a problem hiding this comment.
Similar to another comment in this review. I'm not comfortable exposing stuff only from a crate only for the purposes of testing. The crate should not become a "debug-library" where all the internals are exposed to the user and not for any non-trivial reason. This function conflicts with the config file usage and the design does not make much sense when things can be tweaked in several different ways, each ignoring the other specifications. In this case for e.g., i would rather use an in-memory config file which can easily be created per test and different to other tests, to bind to some specific port etc.
tests/udp_rendezvous_server.rs
Outdated
|
|
||
| #[test] | ||
| fn it_responds_with_client_address() { | ||
| let server_sock = unwrap!(UdpSock::bind(&unwrap!("127.0.0.1:0".parse()))); |
There was a problem hiding this comment.
Check other review comments for reference. Having a config to configure the library and then designing the library to ignore that config doesn't seem nice and especially so if it's only needed by the test. Pls use in-mem config file if needed as explained in the prior review comment.
src/config.rs
Outdated
| @@ -17,7 +17,7 @@ pub const HOLE_PUNCH_WAIT_FOR_OTHER: bool = true; | |||
| /// | |||
| /// User can opt to provide this in a file, read from it and pass it when required. For optional | |||
| /// fields that are `None`, reasonable defaults will be used. | |||
| #[derive(Debug, Clone, Serialize, Deserialize)] | |||
| #[derive(Debug, Clone, Default, Serialize, Deserialize)] | |||
There was a problem hiding this comment.
A natural default doesn't really make sense here as it will be an unusable config (no punchers, no ip-echo servers etc.) and we should not be explicitly giving an API to construct one - maybe create a few minimalistic constructors if you want, which take at the very least
remote_tcp_rendezvous_servers(e.g.,Config::with_tcp_only(...))remote_udp_rendezvous_servers(e.g.Config::with_udp_only(...)and in this case create one entry forudp_hole_puncherswith naturalDefaultforUdpHolePuncher(so all values will be zero). Then wherever this is used if you get starting-ttl=0 stick to the default OS ttl (i.e., don't do anything with ttl) and if ttl_increment_delay_ms=0 then don't create have a timer started (i.e., no retransmissions). Document this behaviour below in config.rs too.
Ofc the user can always override everything later as all fields are public and put values in a manner where it's an unusable config, but that is a conscious choice at that point.
| @@ -277,11 +277,15 @@ extern crate serde_derive; | |||
| extern crate unwrap; | |||
|
|
|||
| extern crate bincode; | |||
| #[cfg(test)] | |||
| extern crate maidsafe_utilities; | |||
There was a problem hiding this comment.
If it's only for tests is it not better to put it as a dev dependency in Cargo.toml ?
There was a problem hiding this comment.
It is a dev dependency, see https://github.com/povilasb/p2p/blob/more-tests/Cargo.toml#L32 :)
povilasb
force-pushed
the
more-tests
branch
2 times, most recently
from
b1439eb to
65a8702
Compare
* move UdpRendezvousServer tests to lib itself, * make UdpRendezvousServer return it's listen address, * fixed UdpRendezvousServer on Windows where thhe client tried to connect with the server using 0.0.0.0 IP address.
We hit the rustfmt bug: rust-lang/rustfmt#1873 . Currently, rustfmt fails to format when path attribute has relitive paths. Usually, our code does not have too much windows specific code and in this case there's no such code at the moment. So, it's the easiest fix is to disable rustfmt on Windows for now hoping for the fix in the future.
Adds UdpRendezvousServer integration test. This is a smaller scope than
nat_traversaltest and will be a good example for such future tests. Also, it will keep us safer while doing safe_crypto integration.