GH-692: Self Review

.gitignore

@@ -65,6 +65,7 @@ Temporary Items
 **/.idea/**/dataSources.ids
 **/.idea/**/dataSources.xml
 **/.idea/**/dataSources.local.xml
+**/.idea/**/dbnavigator.xml
 **/.idea/**/sqlDataSources.xml
 **/.idea/**/dynamic.xml
 **/.idea/**/uiDesigner.xml

masq/src/commands/set_configuration_command.rs

@@ -3,8 +3,8 @@ use crate::commands::commands_common::{transaction, Command, CommandError};
 use clap::{App, Arg, ArgGroup, SubCommand};
 use masq_lib::implement_as_any;
 use masq_lib::messages::{UiSetConfigurationRequest, UiSetConfigurationResponse};
-use masq_lib::shared_schema::common_validators;
-use masq_lib::shared_schema::GAS_PRICE_HELP;
+use masq_lib::shared_schema::gas_price_arg;
+use masq_lib::shared_schema::min_hops_arg;
 use masq_lib::short_writeln;
 use masq_lib::utils::ExpectValue;
 #[cfg(test)]
@@ -66,15 +66,8 @@ const START_BLOCK_HELP: &str =
 pub fn set_configuration_subcommand() -> App<'static, 'static> {
     SubCommand::with_name("set-configuration")
         .about(SET_CONFIGURATION_ABOUT)
-        .arg(
-            Arg::with_name("gas-price")
-                .help(&GAS_PRICE_HELP)
-                .long("gas-price")
-                .value_name("GAS-PRICE")
-                .takes_value(true)
-                .required(false)
-                .validator(common_validators::validate_gas_price),
-        )
+        .arg(gas_price_arg())
+        .arg(min_hops_arg())
         .arg(
             Arg::with_name("start-block")
                 .help(START_BLOCK_HELP)
@@ -86,7 +79,7 @@ pub fn set_configuration_subcommand() -> App<'static, 'static> {
         )
         .group(
             ArgGroup::with_name("parameter")
-                .args(&["gas-price", "start-block"])
+                .args(&["gas-price", "min-hops", "start-block"])
                 .required(true),
         )
 }
@@ -135,16 +128,43 @@ mod tests {
 
     #[test]
     fn command_execution_works_all_fine() {
+        test_command_execution("--start-block", "123456");
+        test_command_execution("--gas-price", "123456");
+        test_command_execution("--min-hops", "6");
+    }
+
+    // TODO: This test only passes when we run through IDE - make it work even without it
+    #[test]
+    #[ignore]
+    fn set_configuration_command_throws_err_for_invalid_arg() {
+        let (invalid_arg, some_value) = ("--invalid-arg", "123");
+
+        let result = SetConfigurationCommand::new(&[
+            "set-configuration".to_string(),
+            invalid_arg.to_string(),
+            some_value.to_string(),
+        ]);
+
+        let err_msg = result.unwrap_err();
+        assert!(err_msg.contains(
+            "error: Found argument '--invalid-arg' \
+             which wasn't expected, or isn't valid in this context"
+        ));
+    }
+
+    fn test_command_execution(name: &str, value: &str) {
         let transact_params_arc = Arc::new(Mutex::new(vec![]));
         let mut context = CommandContextMock::new()
             .transact_params(&transact_params_arc)
             .transact_result(Ok(UiSetConfigurationResponse {}.tmb(4321)));
         let stdout_arc = context.stdout_arc();
         let stderr_arc = context.stderr_arc();
-        let subject = SetConfigurationCommand {
-            name: "start-block".to_string(),
-            value: "123456".to_string(),
-        };
+        let subject = SetConfigurationCommand::new(&[
+            "set-configuration".to_string(),
+            name.to_string(),
+            value.to_string(),
+        ])
+        .unwrap();
 
         let result = subject.execute(&mut context);
 
@@ -154,15 +174,15 @@ mod tests {
             *transact_params,
             vec![(
                 UiSetConfigurationRequest {
-                    name: "start-block".to_string(),
-                    value: "123456".to_string()
+                    name: name[2..].to_string(),
+                    value: value.to_string(),
                 }
                 .tmb(0),
                 1000
             )]
         );
         let stderr = stderr_arc.lock().unwrap();
-        assert_eq!(*stderr.get_string(), String::new());
+        assert_eq!(&stderr.get_string(), "");
         let stdout = stdout_arc.lock().unwrap();
         assert_eq!(&stdout.get_string(), "Parameter was successfully set\n");
     }

masq_lib/src/shared_schema.rs

@@ -218,6 +218,16 @@ lazy_static! {
 
 // These Args are needed in more than one clap schema. To avoid code duplication, they're defined here and referred
 // to from multiple places.
+pub fn chain_arg<'a>() -> Arg<'a, 'a> {
+    Arg::with_name("chain")
+        .long("chain")
+        .value_name("CHAIN")
+        .min_values(0)
+        .max_values(1)
+        .possible_values(official_chain_names())
+        .help(CHAIN_HELP)
+}
+
 pub fn config_file_arg<'a>() -> Arg<'a, 'a> {
     Arg::with_name("config-file")
         .long("config-file")
@@ -240,16 +250,7 @@ pub fn data_directory_arg<'a>() -> Arg<'a, 'a> {
         .help(DATA_DIRECTORY_HELP)
 }
 
-pub fn chain_arg<'a>() -> Arg<'a, 'a> {
-    Arg::with_name("chain")
-        .long("chain")
-        .value_name("CHAIN")
-        .min_values(0)
-        .max_values(1)
-        .possible_values(official_chain_names())
-        .help(CHAIN_HELP)
-}
-
+// TODO: Not an arg fn, move somewhere else
 pub fn official_chain_names() -> &'static [&'static str] {
     &[
         POLYGON_MAINNET_FULL_IDENTIFIER,
@@ -285,6 +286,27 @@ where
         .help(help)
 }
 
+pub fn gas_price_arg<'a>() -> Arg<'a, 'a> {
+    Arg::with_name("gas-price")
+        .long("gas-price")
+        .value_name("GAS-PRICE")
+        .min_values(0)
+        .max_values(1)
+        .validator(common_validators::validate_gas_price)
+        .help(&GAS_PRICE_HELP)
+}
+
+pub fn min_hops_arg<'a>() -> Arg<'a, 'a> {
+    Arg::with_name("min-hops")
+        .long("min-hops")
+        .value_name("MIN_HOPS")
+        .required(false)
+        .min_values(0)
+        .max_values(1)
+        .possible_values(&["1", "2", "3", "4", "5", "6"])
+        .help(MIN_HOPS_HELP)
+}
+
 #[cfg(not(target_os = "windows"))]
 pub fn real_user_arg<'a>() -> Arg<'a, 'a> {
     Arg::with_name("real-user")
@@ -389,15 +411,7 @@ pub fn shared_app(head: App<'static, 'static>) -> App<'static, 'static> {
             .max_values(1)
             .hidden(true),
     )
-    .arg(
-        Arg::with_name("gas-price")
-            .long("gas-price")
-            .value_name("GAS-PRICE")
-            .min_values(0)
-            .max_values(1)
-            .validator(common_validators::validate_gas_price)
-            .help(&GAS_PRICE_HELP),
-    )
+    .arg(gas_price_arg())
     .arg(
         Arg::with_name("ip")
             .long("ip")
@@ -427,16 +441,7 @@ pub fn shared_app(head: App<'static, 'static>) -> App<'static, 'static> {
             .case_insensitive(true)
             .help(MAPPING_PROTOCOL_HELP),
     )
-    .arg(
-        Arg::with_name("min-hops")
-            .long("min-hops")
-            .value_name("MIN_HOPS")
-            .required(false)
-            .min_values(0)
-            .max_values(1)
-            .possible_values(&["1", "2", "3", "4", "5", "6"])
-            .help(MIN_HOPS_HELP),
-    )
+    .arg(min_hops_arg())
     .arg(
         Arg::with_name("neighborhood-mode")
             .long("neighborhood-mode")

multinode_integration_tests/tests/data_routing_test.rs

@@ -70,50 +70,6 @@ fn http_end_to_end_routing_test() {
     );
 }
 
-fn assert_http_end_to_end_routing_test(min_hops: Hops) {
-    let mut cluster = MASQNodeCluster::start().unwrap();
-    let config = NodeStartupConfigBuilder::standard()
-        .min_hops(min_hops)
-        .chain(cluster.chain)
-        .consuming_wallet_info(make_consuming_wallet_info("first_node"))
-        .build();
-    let first_node = cluster.start_real_node(config);
-
-    let nodes_count = 2 * (min_hops as usize) + 1;
-    let nodes = (0..nodes_count)
-        .map(|_| {
-            cluster.start_real_node(
-                NodeStartupConfigBuilder::standard()
-                    .neighbor(first_node.node_reference())
-                    .chain(cluster.chain)
-                    .build(),
-            )
-        })
-        .collect::<Vec<MASQRealNode>>();
-
-    thread::sleep(Duration::from_millis(500 * (nodes.len() as u64)));
-
-    let mut client = first_node.make_client(8080, 5000);
-    client.send_chunk(b"GET / HTTP/1.1\r\nHost: www.example.com\r\n\r\n");
-    let response = client.wait_for_chunk();
-
-    assert_eq!(
-        index_of(&response, &b"<h1>Example Domain</h1>"[..]).is_some(),
-        true,
-        "Actual response:\n{}",
-        String::from_utf8(response).unwrap()
-    );
-}
-
-#[test]
-fn http_end_to_end_routing_test_with_different_min_hops() {
-    // This test fails sometimes due to a timeout: "Couldn't read chunk: Kind(TimedOut)"
-    // You may fix it by increasing the timeout for the client.
-    assert_http_end_to_end_routing_test(Hops::OneHop);
-    assert_http_end_to_end_routing_test(Hops::TwoHops);
-    assert_http_end_to_end_routing_test(Hops::SixHops);
-}
-
 #[test]
 fn http_end_to_end_routing_test_with_consume_and_originate_only_nodes() {
     let mut cluster = MASQNodeCluster::start().unwrap();

multinode_integration_tests/tests/min_hops_test.rs

@@ -0,0 +1,121 @@
+// Copyright (c) 2019, MASQ (https://masq.ai) and/or its affiliates. All rights reserved.
+
+use masq_lib::messages::{ToMessageBody, UiSetConfigurationRequest};
+use masq_lib::utils::{find_free_port, index_of};
+use multinode_integration_tests_lib::masq_node::MASQNode;
+use multinode_integration_tests_lib::masq_node_cluster::MASQNodeCluster;
+use multinode_integration_tests_lib::masq_real_node::{
+    make_consuming_wallet_info, MASQRealNode, NodeStartupConfigBuilder,
+};
+use node_lib::sub_lib::neighborhood::Hops;
+use std::thread;
+use std::time::Duration;
+
+#[test]
+fn http_end_to_end_routing_test_with_different_min_hops() {
+    // This test fails sometimes due to a timeout: "Couldn't read chunk: Kind(TimedOut)"
+    // You may fix it by increasing the timeout for the client.
+    assert_http_end_to_end_routing_test(Hops::OneHop);
+    assert_http_end_to_end_routing_test(Hops::TwoHops);
+    assert_http_end_to_end_routing_test(Hops::SixHops);
+}
+
+fn assert_http_end_to_end_routing_test(min_hops: Hops) {
+    let mut cluster = MASQNodeCluster::start().unwrap();
+    let config = NodeStartupConfigBuilder::standard()
+        .min_hops(min_hops)
+        .chain(cluster.chain)
+        .consuming_wallet_info(make_consuming_wallet_info("first_node"))
+        .build();
+    let first_node = cluster.start_real_node(config);
+
+    let nodes_count = 2 * (min_hops as usize) + 1;
+    let nodes = (0..nodes_count)
+        .map(|_| {
+            cluster.start_real_node(
+                NodeStartupConfigBuilder::standard()
+                    .neighbor(first_node.node_reference())
+                    .chain(cluster.chain)
+                    .build(),
+            )
+        })
+        .collect::<Vec<MASQRealNode>>();
+
+    thread::sleep(Duration::from_millis(500 * (nodes.len() as u64)));
+
+    let mut client = first_node.make_client(8080, 5000);
+    client.send_chunk(b"GET / HTTP/1.1\r\nHost: www.example.com\r\n\r\n");
+    let response = client.wait_for_chunk();
+
+    assert_eq!(
+        index_of(&response, &b"<h1>Example Domain</h1>"[..]).is_some(),
+        true,
+        "Actual response:\n{}",
+        String::from_utf8(response).unwrap()
+    );
+}
+
+#[test]
+fn min_hops_can_be_changed_during_runtime() {
+    let initial_min_hops = Hops::OneHop;
+    let new_min_hops = Hops::TwoHops;
+    let mut cluster = MASQNodeCluster::start().unwrap();
+    let ui_port = find_free_port();
+    let first_node_config = NodeStartupConfigBuilder::standard()
+        .min_hops(initial_min_hops)
+        .chain(cluster.chain)
+        .consuming_wallet_info(make_consuming_wallet_info("first_node"))
+        .ui_port(ui_port)
+        .build();
+    let first_node = cluster.start_real_node(first_node_config);
+    let ui_client = first_node.make_ui(ui_port);
+    let mut prev_node_reference = first_node.node_reference();
+
+    for _ in 0..initial_min_hops as u8 {
+        let new_node_config = NodeStartupConfigBuilder::standard()
+            .neighbor(prev_node_reference)
+            .chain(cluster.chain)
+            .build();
+        let new_node = cluster.start_real_node(new_node_config);
+        prev_node_reference = new_node.node_reference();
+    }
+    thread::sleep(Duration::from_millis(1000));
+
+    let mut client = first_node.make_client(8080, 5000);
+    client.send_chunk(b"GET / HTTP/1.1\r\nHost: www.example.com\r\n\r\n");
+    let response = client.wait_for_chunk();
+
+    // Client shutdown is necessary to re-initialize stream keys for old requests
+    client.shutdown();
+
+    assert_eq!(
+        index_of(&response, &b"<h1>Example Domain</h1>"[..]).is_some(),
+        true,
+        "Actual response:\n{}",
+        String::from_utf8(response).unwrap()
+    );
+
+    ui_client.send_request(
+        UiSetConfigurationRequest {
+            name: "min-hops".to_string(),
+            value: new_min_hops.to_string(),
+        }
+        .tmb(1),
+    );
+    let response = ui_client.wait_for_response(1, Duration::from_secs(2));
+    assert!(response.payload.is_ok());
+
+    let mut client = first_node.make_client(8080, 5000);
+    client.send_chunk(b"GET / HTTP/1.1\r\nHost: www.example.com\r\n\r\n");
+    let response = client.wait_for_chunk();
+    assert_eq!(
+        index_of(
+            &response,
+            &b"<h3>Subtitle: Can't find a route to www.example.com</h3>"[..]
+        )
+        .is_some(),
+        true,
+        "Actual response:\n{}",
+        String::from_utf8(response).unwrap()
+    );
+}