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/vault: use real routing for the integration test, with multiple vaults #910

Merged
merged 2 commits into from Dec 19, 2019

Conversation

@maqi
Copy link
Member

maqi commented Dec 11, 2019

This PR contains the work to make vault use real routing (with mock_parsec) during the integration test.
The testing environment contains a network of vaults, and the client requests are handled by the network, instead of just one vault.

@maqi maqi requested a review from ustulation as a code owner Dec 11, 2019
@maqi maqi requested review from nbaksalyar, jeanphilippeD and octol Dec 11, 2019
@maqi maqi force-pushed the maqi:real_routing branch 2 times, most recently from 8133030 to be045a3 Dec 11, 2019
src/config_handler.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved

#[cfg(feature = "mock-parsec")]
{
processed = self.routing_node.borrow_mut().poll();

This comment has been minimized.

Copy link
@jeanphilippeD

jeanphilippeD Dec 11, 2019

Contributor

Why is this needed, why is handle_selected_operation, and should we make it sufficient somehow if not?

This comment has been minimized.

Copy link
@maqi

maqi Dec 11, 2019

Author Member

only poll checks the timer advancing, handle_selected_operation doesnt

This comment has been minimized.

Copy link
@jeanphilippeD

jeanphilippeD Dec 11, 2019

Contributor

What do you mean by check the timer advancing?
And why is handle_selected_operation not, should it?

This comment has been minimized.

Copy link
@maqi

maqi Dec 11, 2019

Author Member

the routing::Node::poll eventually calls routing::StateMachine::try_step, which checks the timed_out_tokens
However, the handle_selected_operation doesn't.
I don't think it shall check the timer, as it is only for handle received action ?.

src/vault.rs Outdated Show resolved Hide resolved
src/vault.rs Outdated Show resolved Hide resolved
tests/common/mod.rs Outdated Show resolved Hide resolved
tests/common/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

jeanphilippeD left a comment

Left a number of comment, it does not feel quite completely ready.

@maqi maqi added the in progress label Dec 11, 2019
@maqi maqi changed the title test/vault: use real routing for the integration test WIP test/vault: use real routing for the integration test Dec 11, 2019
@maqi maqi changed the title WIP test/vault: use real routing for the integration test WIP test/vault: use real routing for the integration test, with multiple vaults Dec 11, 2019
@maqi maqi force-pushed the maqi:real_routing branch 10 times, most recently from 8f6fbe7 to 8d4a895 Dec 12, 2019
@maqi maqi changed the title WIP test/vault: use real routing for the integration test, with multiple vaults test/vault: use real routing for the integration test, with multiple vaults Dec 18, 2019
@maqi maqi removed the in progress label Dec 18, 2019
Cargo.toml Show resolved Hide resolved
src/vault.rs Outdated Show resolved Hide resolved
tests/common/mod.rs Outdated Show resolved Hide resolved
@maqi maqi force-pushed the maqi:real_routing branch from 8d4a895 to cd5774a Dec 18, 2019
@@ -189,9 +209,13 @@ impl Vault {
/// Processes any outstanding network events and returns. Does not block.
/// Returns whether at least one event was processed.
pub fn poll(&mut self) -> bool {
let mut processed = false;

let mut _processed = false;

This comment has been minimized.

Copy link
@jeanphilippeD

jeanphilippeD Dec 19, 2019

Contributor

why the _

This comment has been minimized.

Copy link
@maqi

maqi Dec 19, 2019

Author Member

just trying to limit the scope of rule, as mentioned at: #910 (comment)

Without _, we will have to use #[allow(unused_assignments)] above the function

This comment has been minimized.

Copy link
@jeanphilippeD

jeanphilippeD Dec 19, 2019

Contributor

I don't get it, we return the value.
Your problem is that you probably should just have:

let mut processed;

This comment has been minimized.

Copy link
@maqi

maqi Dec 19, 2019

Author Member

I tried let mut processed;
there is a complain of value assigned to processed is never read
I think it's because the returning is not considered as a read, as there is an initial value assigned, however may not be used before being set to true ?

Copy link
Contributor

jeanphilippeD left a comment

No sure everything is right, but if everyone that work on this code is happy, I'm ok.

@octol
octol approved these changes Dec 19, 2019
Copy link
Member

nbaksalyar left a comment

Looking good 👍 Just a few small nitpicks from me :)

Feel free to address these in a separate PR if it's easier.

@@ -1555,7 +1582,7 @@ impl ClientHandler {
let valid = if let Some(signature) = signature {
self.is_valid_client_signature(public_id, request, &message_id, &signature)
} else {
warn!(
println!(

This comment has been minimized.

Copy link
@nbaksalyar

nbaksalyar Dec 19, 2019

Member

Is there a reason to change from one of the log macros to println!?

This comment has been minimized.

Copy link
@maqi

maqi Dec 19, 2019

Author Member

Ah, missed this one. a left over from the debuggings. will change it back.

loop {
#[cfg(feature = "mock_parsec")]

This comment has been minimized.

Copy link
@nbaksalyar

nbaksalyar Dec 19, 2019

Member

Probably something like

if cfg!(feature = "mock_parsec") {
  ...
}

would be more idiomatic?

This comment has been minimized.

Copy link
@maqi

maqi Dec 19, 2019

Author Member

Cannot do that.

Although the code won't be executed when compile with mock, the compiler will still looking for routing_node having a poll function, which is not provided (needed) by mock_routing.

self.vaults
.iter_mut()
.for_each(|vault| processed = processed || vault.inner.poll());
FakeClock::advance_time(1001);

This comment has been minimized.

Copy link
@nbaksalyar

nbaksalyar Dec 19, 2019

Member

1001 looks like an arbitrary magic number, so maybe put it into a constant or add a comment?

This comment has been minimized.

Copy link
@maqi

maqi Dec 19, 2019

Author Member

just used the same as in routing. will put a comment in

Ok(Event::ConnectedTo {
peer: Peer::Node { ref node_info },
}) if node_info == conn_info => return,
// _ => (),

This comment has been minimized.

Copy link
@nbaksalyar

nbaksalyar Dec 19, 2019

Member

Looks like dead code

This comment has been minimized.

Copy link
@maqi

maqi Dec 19, 2019

Author Member

forgot to remove, will do

@maqi maqi force-pushed the maqi:real_routing branch from cd5774a to 8e228cf Dec 19, 2019
@maqi maqi force-pushed the maqi:real_routing branch from 8e228cf to 236aba3 Dec 19, 2019
@nbaksalyar nbaksalyar merged commit 6794039 into maidsafe:vNext Dec 19, 2019
4 checks passed
4 checks passed
Rustfmt-Clippy
Details
Test (ubuntu-latest)
Details
Test (windows-latest)
Details
Test (macOS-latest)
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.