MAID-1957 test/tunnel: tunnel client reconnect #1355
Conversation
|
r? @afck (maidsafe_highfive has picked a reviewer for you, use r? to override) |
maqi
force-pushed
the
tunnel_test_tryout
branch
2 times, most recently
from
4de7e70
to
1a61073
Compare
Viv-Rajkumar
changed the title
maqi
force-pushed
the
tunnel_test_tryout
branch
from
8d9719e
to
73fb313
Compare
Viv-Rajkumar
changed the title
There was a problem hiding this comment.
I have some general points for now:
- I'd rather we avoid the changes that have been made to node.rs, peer_manager.rs and states/node.rs. I think we could modify the mock crust framework to enable the blocking/dropping of tunnelled connections there, and just allow the production code to take care of handling the generated crust events. And for reconnecting, we could use the RT request mechanism (although I'm slightly less worried about the exposure of
send_connect_request()since it doesn't seem to be quite as intrusive as theremove_peer()function). - If we take this approach, then in the tests, when we remove a connection we can check for a
NodeLostevent, and likewise if the connection gets re-established we can check for aNodeAddedevent. Just doing the invariant check at each stage doesn't really tell us whether anything changed at all in the network during that stage.
maqi
force-pushed
the
tunnel_test_tryout
branch
3 times, most recently
from
72aca12
to
cdb9705
Compare
tests/mock_crust/tunnel.rs
Outdated
| endpoints | ||
| } | ||
|
|
||
| // Adds a pair of nodes with specified names into the nework. Returns the endpoints of the nodes. |
tests/mock_crust/tunnel.rs
Outdated
| // Adds a pair of nodes with specified names into the nework. Returns the endpoints of the nodes. | ||
| // | ||
| // Note: it's necessary to call `poll_all` afterwards, as this function doesn't call it for the | ||
| // second added node. |
There was a problem hiding this comment.
I'd find this more consistent if it called it for both. You could add a tunnel: bool option to the method to block the connection before calling poll_and_resend the second time?
tests/mock_crust/tunnel.rs
Outdated
|
|
||
| #[test] | ||
| fn tunnel_clients() { | ||
| let min_section_size = 5; |
There was a problem hiding this comment.
Maybe we could even use size 3, to speed up the test?
maqi
force-pushed
the
tunnel_test_tryout
branch
from
cdb9705
to
2372755
Compare
| .collect(); | ||
| section_indexes.iter() | ||
| .take(section_indexes.len() - min_section_size + 1) | ||
| .foreach(|index| { let _ = nodes.remove(*index); }); |
There was a problem hiding this comment.
You'd best sort the indices from highest to lowest if you want to remove them from the Vec like this.
There was a problem hiding this comment.
it was taken in a reversed order, https://github.com/maidsafe/routing/pull/1355/files#diff-b83639c52df93eae4d3e66ec59a8e66fR69
so I think we are ok here?
There was a problem hiding this comment.
I guess so. I don't much like this style of coding because it's easy to accidentally break it, but I guess it'll do.
tests/mock_crust/tunnel.rs
Outdated
| poll_and_resend(nodes, &mut []); | ||
| } | ||
|
|
||
| // Adds a pair of nodes with specified names into the network. Returns the endpoints of the nodes. |
There was a problem hiding this comment.
Also blocks direct connection between these two nodes if is_tunnel is set.
tests/mock_crust/utils.rs
Outdated
| mut prefix_lengths: Vec<usize>, | ||
| use_cache: bool) | ||
| -> Nodes { | ||
| pub fn create_connected_nodes_until_split_with_nodes(network: &Network, |
There was a problem hiding this comment.
Should be add_connected_nodes_until_split.
maqi
force-pushed
the
tunnel_test_tryout
branch
2 times, most recently
from
61b29e0
to
7d7f674
Compare
maqi
force-pushed
the
tunnel_test_tryout
branch
from
7d7f674
to
5932152
Compare
No description provided.