GH-499 review 6 head

automap/src/control_layer/automap_control.rs

@@ -380,7 +380,7 @@ mod tests {
     use std::sync::{Arc, Mutex};
 
     #[test]
-    fn automap_config_default_implmentation_works_properly() {
+    fn automap_config_default_implementation_works_properly() {
         let subject = AutomapConfig::default();
 
         assert_eq!(subject.usual_protocol_opt, None);

masq_lib/src/shared_schema.rs

@@ -29,6 +29,14 @@ pub const CONSUMING_PRIVATE_KEY_HELP: &str = "The private key for the Ethereum w
      make sure you haven't already set up a consuming wallet with a derivation path, and make sure that you always \
      supply exactly the same private key every time you run the Node. A consuming private key is 64 case-insensitive \
      hexadecimal digits.";
+pub const CRASH_POINT_HELP: &str = "The Node is designed not to crash; however, unforeseen circumstances can strike. \
+     Therefore, it's important for things around the Node that depend upon it to be able to handle crashes, should \
+     they occur. But because it's been hardened against crashing, it's hard to make it crash on cue in order to \
+     test the crash-handling capabilities of those peripheral utilities. That's the purpose of --crash-point: to \
+     make the Node easier to crash. --crash-point None is the same as not specifying --crash-point at all. \
+     --crash-point Panic makes the Node panic on startup. --crash-point Error makes the Node log an ERROR \
+     message on startup and then panic. --crash-point Message does not make the Node panic on startup, but it \
+     makes the Node vulnerable to the \"crash\" message from a user interface via the MASQNode-UIv2 protocol.";
 pub const DATA_DIRECTORY_HELP: &str =
     "Directory in which the Node will store its persistent state, including at \
      least its database and by default its configuration file as well.";
@@ -45,6 +53,20 @@ pub const EARNING_WALLET_HELP: &str =
      (case-insensitive). If you already have a derivation-path earning wallet, don't supply this. \
      If you have supplied an earning wallet address before, either don't supply it again or be \
      careful to supply exactly the same one you supplied before.";
+pub const FAKE_PUBLIC_KEY_HELP: &str =
+    "Normally when you start a Node, it selects its own public/private key pairs. But when you start a Node \
+     for testing, it can be handy to be able to specify its keys yourself, and to have it use CryptDENull \
+     encryption rather than CryptDEReal encryption, so that you can intercept and read its communications. \
+     To do this, specify --fake-public-key with the public key you want the Node to use. That public key \
+     should be a valid Base64 string of whatever length you like; the Node will synthesize main and alias \
+     keypairs from that string. PLEASE NOTE: specifying --fake-public-key will DISABLE EFFECTIVE ENCRYPTION \
+     for the Node; it will not be able to communicate with other people's Nodes that use real encryption.";
+pub const FAKE_ROUTER_IP_HELP: &str =
+    "When Automap normally communicates with the router, it naturally expects that the router will be at the \
+     address identified by the system network stack as the network gateway. However, for some kinds of tests, \
+     you want Automap to conduct its router communications with a mock router you set up. In that case, you \
+     can start the Node using --fake-router-ip with an IP address, and Automap will assume that the router \
+     is at that address rather than the gateway address.";
 pub const IP_ADDRESS_HELP: &str = "The public IP address of your MASQ Node: that is, the IPv4 \
      address at which other Nodes can contact yours. If you're running your Node behind \
      a router, this will be the IP address of the router. If this IP address starts with 192.168 or 10.0, \
@@ -109,6 +131,9 @@ pub const REAL_USER_HELP: &str =
      run with root privilege after bootstrapping, you might want to use this if you start the Node as root, or if \
      you start the Node using pkexec or some other method that doesn't populate the SUDO_xxx variables. Use a value \
      like <uid>:<gid>:<home directory>.";
+pub const WINDOWS_REAL_USER_HELP: &str =
+    "--real-user doesn't really apply to Windows as long as Windows Nodes don't drop their privilege, so it does \
+     not appear in the help for Windows. However, if you wish, you can still specify it for testing purposes.";
 pub const SCANS_HELP: &str =
     "The Node, when running, performs various periodic scans, including scanning for payables that need to be paid, \
     for pending payables that have arrived (and are no longer pending), for incoming receivables that need to be \
@@ -283,6 +308,7 @@ pub fn real_user_arg<'a>() -> Arg<'a, 'a> {
         .required(false)
         .takes_value(true)
         .validator(common_validators::validate_real_user)
+        .help(WINDOWS_REAL_USER_HELP)
         .hidden(true)
 }
 
@@ -342,6 +368,7 @@ pub fn shared_app(head: App<'static, 'static>) -> App<'static, 'static> {
             .max_values(1)
             .possible_values(&CrashPoint::variants())
             .case_insensitive(true)
+            .help(CRASH_POINT_HELP)
             .hidden(true),
     )
     .arg(data_directory_arg())
@@ -365,6 +392,7 @@ pub fn shared_app(head: App<'static, 'static>) -> App<'static, 'static> {
             .value_name("FAKE-PUBLIC-KEY")
             .min_values(0)
             .max_values(1)
+            .help(FAKE_PUBLIC_KEY_HELP)
             .hidden(true),
     )
     .arg(
@@ -374,6 +402,7 @@ pub fn shared_app(head: App<'static, 'static>) -> App<'static, 'static> {
             .required(false)
             .min_values(0)
             .max_values(1)
+            .help(FAKE_ROUTER_IP_HELP)
             .hidden(true),
     )
     .arg(

multinode_integration_tests/src/main.rs

@@ -230,7 +230,7 @@ impl DataProbe {
     }
 
     fn usage(stderr: &mut dyn Write) -> u8 {
-        writeln! (stderr, "{DATAPROBE_USAGE}").unwrap ();
+        writeln!(stderr, "{DATAPROBE_USAGE}").unwrap();
         1
     }
 
@@ -466,7 +466,7 @@ mod tests {
 
         assert_eq!(result, 1);
         let stderr = holder.stderr;
-        assert_eq! (stderr.get_string (), format!("{DATAPROBE_USAGE}\n\n"));
+        assert_eq!(stderr.get_string(), format!("{DATAPROBE_USAGE}\n\n"));
     }
 
     #[test]
@@ -481,10 +481,7 @@ mod tests {
 
         assert_eq!(result, 1);
         let stderr = holder.stderr;
-        assert_eq!(
-            stderr.get_string(),
-            format!("{PROBE_TARGET_SYNTAX_MSG}\n")
-        );
+        assert_eq!(stderr.get_string(), format!("{PROBE_TARGET_SYNTAX_MSG}\n"));
     }
 
     #[test]

multinode_integration_tests/src/masq_mock_node.rs

@@ -507,8 +507,13 @@ impl MASQMockNode {
         let earning_wallet = make_wallet(format!("{}_earning", name).as_str());
         let consuming_wallet = Some(make_paying_wallet(format!("{}_consuming", name).as_bytes()));
         DataProbeUtils::clean_up_existing_container(&name[..]);
-        let mock_node_args = Self::make_mock_node_args(node_addr);
-        do_docker_run(&node_addr, host_node_parent_dir, &name, mock_node_args);
+        let mock_node_args = Self::make_mock_node_args(&node_addr);
+        do_docker_run(
+            node_addr.ip_addr(),
+            host_node_parent_dir,
+            &name,
+            mock_node_args,
+        );
         let wait_addr = SocketAddr::new(node_addr.ip_addr(), CONTROL_STREAM_PORT);
         let control_stream = RefCell::new(wait_for_startup(wait_addr, &name));
         let framer = RefCell::new(DataHunkFramer::new());

multinode_integration_tests/src/mock_router.rs

@@ -20,26 +20,27 @@ impl MockRouter for MockPcpRouter {
     }
 }
 
-impl Default for MockPcpRouter {
-    fn default() -> Self {
-        Self::new()
-    }
-}
-
 impl MockPcpRouter {
     pub fn new(port: u16) -> Self {
         let control_stream = Self::start(port);
-        Self {}
+        Self {
+            control_stream,
+            framer: DataHunkFramer::new(),
+        }
     }
 
     fn start(port: u16) -> TcpStream {
         let name = "pcp_router".to_string();
         DataProbeUtils::clean_up_existing_container(&name[..]);
         let mock_router_args = Self::make_mock_router_args(port);
-        do_docker_run(&node_addr, host_node_parent_dir, &name, mock_router_args);
+        do_docker_run(
+            node_addr.ip_addr(),
+            host_node_parent_dir,
+            &name,
+            mock_router_args,
+        );
         let wait_addr = SocketAddr::new(node_addr.ip_addr(), CONTROL_STREAM_PORT);
         let control_stream = wait_for_startup(wait_addr, &name);
-        let framer = RefCell::new(DataHunkFramer::new());
         control_stream
     }
 }

multinode_integration_tests/src/utils.rs

@@ -130,7 +130,7 @@ pub fn do_docker_run(
     ip_addr: IpAddr,
     host_node_parent_dir: Option<String>,
     name: &str,
-    program_args: Vec<String>,
+    data_probe_args: Vec<String>,
 ) {
     let root = match host_node_parent_dir {
         Some(dir) => dir,
@@ -153,7 +153,7 @@ pub fn do_docker_run(
         "test_node_image",
         "/node_root/node/mock_node", // TODO: Should be /node_root/node/data_probe
     ]);
-    docker_args.extend(program_args);
+    docker_args.extend(data_probe_args);
     let mut command = Command::new(docker_command, docker_args);
     command.stdout_or_stderr().unwrap();
 }

node/src/actor_system_factory.rs

@@ -36,7 +36,9 @@ use crate::sub_lib::ui_gateway::UiGatewaySubs;
 use actix::Arbiter;
 use actix::{Addr, Recipient};
 use automap_lib::comm_layer::AutomapError;
-use automap_lib::control_layer::automap_control::{AutomapChange, AutomapConfig, AutomapControl, AutomapControlReal, ChangeHandler};
+use automap_lib::control_layer::automap_control::{
+    AutomapChange, AutomapConfig, AutomapControl, AutomapControlReal, ChangeHandler,
+};
 use masq_lib::blockchains::chains::Chain;
 use masq_lib::crash_point::CrashPoint;
 use masq_lib::logger::prepare_log_recipient;
@@ -569,6 +571,7 @@ impl AutomapControlFactory for AutomapControlFactoryReal {
         automap_config: AutomapConfig,
         change_handler: ChangeHandler,
     ) -> Box<dyn AutomapControl> {
+        // Untested factory-pattern code: test by inspection
         Box::new(AutomapControlReal::new(automap_config, change_handler))
     }
 }
@@ -1627,7 +1630,8 @@ mod tests {
     }
 
     #[test]
-    fn handle_automap_error_delegates_correctly_to_factory() {
+    fn make_automap_control_initializes_properly() {
+        let test_name = "make_automap_control_initializes_properly";
         let make_params_arc = Arc::new(Mutex::new(vec![]));
         let automap_control = AutomapControlMock::new();
         let automap_control_factory = AutomapControlFactoryMock::new()
@@ -1639,34 +1643,33 @@ mod tests {
         subject.automap_control_factory = Box::new(automap_control_factory);
         let mapping_protocol = AutomapProtocol::Pcp;
         let fake_router_ip = IpAddr::from_str("1.5.2.4").unwrap();
+        let automap_config = AutomapConfig::new(Some(mapping_protocol), Some(fake_router_ip));
 
-        let _result = subject.make_automap_control(
-            AutomapConfig::new(Some(mapping_protocol), Some(fake_router_ip)),
-            vec![new_ip_recipient_sub],
-        );
+        let _result = subject.make_automap_control(automap_config, vec![new_ip_recipient_sub]);
 
         let mut make_params = make_params_arc.lock().unwrap();
         // Make sure the AutomapConfig was passed correctly
-        let (automap_config, change_handler) = make_params.remove(0);
-        assert_eq!(
-            automap_config,
-            AutomapConfig::new(Some(AutomapProtocol::Pcp), Some(fake_router_ip))
-        );
+        let (actual_automap_config, change_handler) = make_params.remove(0);
+        assert_eq!(actual_automap_config, automap_config);
         // Make sure the generated change_handler handles new IP addresses properly
         let new_ip = IpAddr::from_str("1.2.3.4").unwrap();
-        let system = System::new("handle_automap_error_delegates_correctly_to_factory");
+        let system = System::new(test_name);
         change_handler(AutomapChange::NewIp(new_ip));
         System::current().stop();
         system.run();
         let new_ip_recipient_recording = new_ip_recipient_recording_arc.lock().unwrap();
         assert_eq!(
             new_ip_recipient_recording.get_record::<NewPublicIp>(0),
-            &NewPublicIp {new_ip}
+            &NewPublicIp { new_ip }
         );
         // Make sure the generated change_handler handles errors properly
         init_test_logging();
-        change_handler(AutomapChange::Error(AutomapError::DeleteMappingError("handle_automap_error_delegates_correctly_to_factory".to_string())));
-        TestLogHandler::new().exists_log_containing("ERROR: Automap: Automap failure: DeleteMappingError(\"handle_automap_error_delegates_correctly_to_factory\")");
+        change_handler(AutomapChange::Error(AutomapError::DeleteMappingError(
+            test_name.to_string(),
+        )));
+        TestLogHandler::new().exists_log_containing(&format!(
+            "ERROR: Automap: Automap failure: DeleteMappingError(\"{test_name}\")"
+        ));
     }
 
     #[test]

node/src/database/db_migrations/db_migrator.rs

@@ -181,7 +181,6 @@ impl DbMigratorReal {
 #[cfg(test)]
 mod tests {
     use crate::database::connection_wrapper::{ConnectionWrapper, ConnectionWrapperReal};
-    use crate::database::test_utils::ConnectionWrapperMock;
     use crate::database::db_initializer::{ExternalData, CURRENT_SCHEMA_VERSION};
     use crate::database::db_migrations::db_migrator::{
         DatabaseMigration, DbMigrator, DbMigratorReal,
@@ -192,6 +191,7 @@ mod tests {
         DBMigratorInnerConfiguration,
     };
     use crate::database::db_migrations::test_utils::DBMigDeclaratorMock;
+    use crate::database::test_utils::ConnectionWrapperMock;
     use crate::test_utils::database_utils::make_external_data;
     use masq_lib::constants::TEST_DEFAULT_CHAIN;
     use masq_lib::logger::Logger;

node/src/database/mod.rs

@@ -4,5 +4,4 @@ pub mod config_dumper;
 pub mod connection_wrapper;
 pub mod db_initializer;
 pub mod db_migrations;
-#[cfg(test)]
 pub mod test_utils;

node/src/database/test_utils.rs

@@ -1,7 +1,6 @@
+#![cfg(test)]
 use crate::database::connection_wrapper::ConnectionWrapper;
-use crate::database::db_initializer::{
-    DbInitializationConfig, ExternalData, InitializationMode,
-};
+use crate::database::db_initializer::{DbInitializationConfig, ExternalData, InitializationMode};
 use crate::database::db_initializer::{DbInitializer, InitializationError};
 use crate::test_utils::unshared_test_utils::arbitrary_id_stamp::ArbitraryIdStamp;
 use crate::{arbitrary_id_stamp, set_arbitrary_id_stamp};
@@ -75,8 +74,7 @@ impl<'a: 'b, 'b> ConnectionWrapper for ConnectionWrapperMock<'a, 'b> {
 #[derive(Default)]
 pub struct DbInitializerMock {
     pub initialize_params: Arc<Mutex<Vec<(PathBuf, DbInitializationConfig)>>>,
-    pub initialize_results:
-    RefCell<Vec<Result<Box<dyn ConnectionWrapper>, InitializationError>>>,
+    pub initialize_results: RefCell<Vec<Result<Box<dyn ConnectionWrapper>, InitializationError>>>,
 }
 
 impl DbInitializer for DbInitializerMock {
@@ -92,17 +90,16 @@ impl DbInitializer for DbInitializerMock {
         self.initialize_results.borrow_mut().remove(0)
     }
 
-    #[allow(unused_variables)]
     fn initialize_to_version(
         &self,
-        path: &Path,
-        target_version: usize,
-        init_config: DbInitializationConfig,
+        _path: &Path,
+        _target_version: usize,
+        _init_config: DbInitializationConfig,
     ) -> Result<Box<dyn ConnectionWrapper>, InitializationError> {
         intentionally_blank!()
-        /*all existing test calls only initialize() in the mocked version,
+        /* all existing tests call only initialize() in the mocked version,
         but we need to call initialize_to_version() for the real object
-        in order to carry out some important tests too*/
+        in order to carry out some important tests too */
     }
 }
 

node/src/node_configurator/node_configurator_standard.rs

@@ -902,6 +902,7 @@ mod tests {
             .param("--blockchain-service-url", "http://127.0.0.1:8545")
             .param("--log-level", "trace")
             .param("--fake-public-key", "AQIDBA")
+            .param("--fake-router-ip", "4.5.6.7")
             .param("--db-password", "secret-db-password")
             .param(
                 "--earning-wallet",
@@ -946,6 +947,10 @@ mod tests {
             config.main_cryptde_null_opt.unwrap().public_key(),
             &PublicKey::new(&[1, 2, 3, 4]),
         );
+        assert_eq!(
+            config.automap_config.fake_router_ip_opt,
+            Some(IpAddr::from_str("4.5.6.7").unwrap())
+        );
         assert_eq!(
             config.real_user,
             RealUser::new(Some(999), Some(999), Some(PathBuf::from("/home/booga")))

node/src/test_utils/automap_mocks.rs

@@ -54,7 +54,6 @@ impl AutomapControlFactoryMock {
     }
 }
 
-#[derive(Clone)]
 pub struct AutomapControlMock {
     get_public_ip_results: RefCell<Vec<Result<IpAddr, AutomapError>>>,
     add_mapping_params: Arc<Mutex<Vec<u16>>>,