GH-692: Self Review 2

masq_lib/src/shared_schema.rs

@@ -250,7 +250,6 @@ pub fn data_directory_arg<'a>() -> Arg<'a, 'a> {
         .help(DATA_DIRECTORY_HELP)
 }
 
-// TODO: Not an arg fn, move somewhere else
 pub fn official_chain_names() -> &'static [&'static str] {
     &[
         POLYGON_MAINNET_FULL_IDENTIFIER,

multinode_integration_tests/tests/min_hops_test.rs → multinode_integration_tests/tests/min_hops_tests.rs

@@ -12,15 +12,15 @@ use std::thread;
 use std::time::Duration;
 
 #[test]
-fn http_end_to_end_routing_test_with_different_min_hops() {
+fn data_can_be_routed_using_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);
+    assert_http_end_to_end_routing(Hops::OneHop);
+    assert_http_end_to_end_routing(Hops::TwoHops);
+    assert_http_end_to_end_routing(Hops::SixHops);
 }
 
-fn assert_http_end_to_end_routing_test(min_hops: Hops) {
+fn assert_http_end_to_end_routing(min_hops: Hops) {
     let mut cluster = MASQNodeCluster::start().unwrap();
     let config = NodeStartupConfigBuilder::standard()
         .min_hops(min_hops)
@@ -69,15 +69,14 @@ fn min_hops_can_be_changed_during_runtime() {
         .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();
+        cluster.start_real_node(
+            NodeStartupConfigBuilder::standard()
+                .neighbor(first_node.node_reference())
+                .chain(cluster.chain)
+                .build(),
+        );
     }
     thread::sleep(Duration::from_millis(1000));
 

node/src/neighborhood/mod.rs

@@ -154,7 +154,16 @@ impl Handler<ConfigurationChangeMessage> for Neighborhood {
             }
             ConfigurationChange::UpdateMinHops(new_min_hops) => {
                 self.set_min_hops_and_patch_size(new_min_hops);
-                // TODO: Should we make the stage transition for OverallConnectionStatus from RouteFound to ConnectedToNeighbor before we search for a new route
+                let node_to_ui_recipient = self
+                    .node_to_ui_recipient_opt
+                    .as_ref()
+                    .expect("UI gateway is dead");
+                self.overall_connection_status
+                    .update_ocs_stage_and_send_message_to_ui(
+                        OverallConnectionStage::ConnectedToNeighbor,
+                        node_to_ui_recipient,
+                        &self.logger,
+                    );
                 self.search_for_a_new_route();
             }
         }
@@ -3059,14 +3068,15 @@ mod tests {
     fn can_update_min_hops_with_configuration_change_msg() {
         init_test_logging();
         let test_name = "can_update_min_hops_with_configuration_change_msg";
+        let new_min_hops = Hops::FourHops;
         let system = System::new(test_name);
+        let (ui_gateway, _, ui_gateway_recording) = make_recorder();
         let mut subject = make_standard_subject();
         subject.min_hops = Hops::TwoHops;
         subject.logger = Logger::new(test_name);
         subject.overall_connection_status.stage = OverallConnectionStage::RouteFound;
-        let new_min_hops = Hops::FourHops;
         let subject_addr = subject.start();
-        let peer_actors = peer_actors_builder().build();
+        let peer_actors = peer_actors_builder().ui_gateway(ui_gateway).build();
         subject_addr.try_send(BindMessage { peer_actors }).unwrap();
 
         subject_addr
@@ -3077,19 +3087,39 @@ mod tests {
 
         subject_addr
             .try_send(AssertionsMessage {
-                assertions: Box::new(move |actor: &mut Neighborhood| {
+                assertions: Box::new(move |neighborhood: &mut Neighborhood| {
                     let expected_db_patch_size =
                         Neighborhood::calculate_db_patch_size(new_min_hops);
-                    assert_eq!(actor.min_hops, new_min_hops);
-                    assert_eq!(actor.db_patch_size, expected_db_patch_size);
+                    assert_eq!(neighborhood.min_hops, new_min_hops);
+                    assert_eq!(neighborhood.db_patch_size, expected_db_patch_size);
+                    assert_eq!(
+                        neighborhood.overall_connection_status.stage,
+                        OverallConnectionStage::ConnectedToNeighbor
+                    );
                 }),
             })
             .unwrap();
         System::current().stop();
         system.run();
-        TestLogHandler::new().exists_log_containing(&format!(
-            "DEBUG: {test_name}: Searching for a {new_min_hops}-hop route..."
-        ));
+        let recording = ui_gateway_recording.lock().unwrap();
+        let message: &NodeToUiMessage = recording.get_record(0);
+        assert_eq!(
+            message,
+            &NodeToUiMessage {
+                target: MessageTarget::AllClients,
+                body: UiConnectionChangeBroadcast {
+                    stage: UiConnectionStage::ConnectedToNeighbor
+                }
+                .tmb(0),
+            }
+        );
+        TestLogHandler::new().assert_logs_contain_in_order(vec![
+            &format!(
+                "DEBUG: {test_name}: The stage of OverallConnectionStatus has been changed \
+                from RouteFound to ConnectedToNeighbor. A message to the UI was also sent."
+            ),
+            &format!("DEBUG: {test_name}: Searching for a 4-hop route..."),
+        ]);
     }
 
     #[test]
@@ -4000,9 +4030,12 @@ mod tests {
         let ui_recording = ui_gateway_arc.lock().unwrap();
         assert_eq!(ui_recording.len(), 0);
         assert_eq!(subject.overall_connection_status.can_make_routes(), false);
-        TestLogHandler::new().exists_log_containing(&format!(
-            "DEBUG: {}: The connectivity check still can't find a good route.",
-            test_name
+        let tlh = TestLogHandler::new();
+        tlh.exists_log_containing(&format!(
+            "DEBUG: {test_name}: The connectivity check still can't find a good route.",
+        ));
+        tlh.exists_no_log_containing(&format!(
+            "DEBUG: {test_name}: The connectivity check has found a 5-hop route."
         ));
     }
 

node/src/neighborhood/overall_connection_status.rs

@@ -266,9 +266,8 @@ impl OverallConnectionStatus {
         node_to_ui_recipient: &Recipient<NodeToUiMessage>,
         logger: &Logger,
     ) {
-        // TODO: Modify this fn when you're implementing the regressing transitions
         let prev_stage = self.stage;
-        if new_stage as usize > prev_stage as usize {
+        if new_stage != prev_stage {
             self.stage = new_stage;
             OverallConnectionStatus::send_message_to_ui(self.stage.into(), node_to_ui_recipient);
             debug!(
@@ -909,9 +908,9 @@ mod tests {
     }
 
     #[test]
-    fn doesn_t_send_message_to_the_ui_in_case_stage_hasn_t_updated() {
+    fn does_not_send_message_to_the_ui_in_case_the_stage_has_not_updated() {
         init_test_logging();
-        let test_name = "doesn_t_send_message_to_the_ui_in_case_stage_hasn_t_updated";
+        let test_name = "does_not_send_message_to_the_ui_in_case_the_stage_has_not_updated";
         let initial_stage = OverallConnectionStage::ConnectedToNeighbor;
         let new_stage = initial_stage;
 
@@ -928,21 +927,29 @@ mod tests {
     }
 
     #[test]
-    fn doesn_t_send_a_message_to_ui_in_case_connection_drops_from_three_hops_to_connected_to_neighbor(
-    ) {
+    fn sends_a_message_to_ui_in_case_connection_drops_from_three_hops_to_connected_to_neighbor() {
         init_test_logging();
-        let test_name = "doesn_t_send_a_message_to_ui_in_case_connection_drops_from_three_hops_to_connected_to_neighbor";
+        let test_name = "sends_a_message_to_ui_in_case_connection_drops_from_three_hops_to_connected_to_neighbor";
         let initial_stage = OverallConnectionStage::RouteFound;
         let new_stage = OverallConnectionStage::ConnectedToNeighbor;
 
         let (stage, message_opt) =
             assert_stage_and_node_to_ui_message(initial_stage, new_stage, test_name);
 
-        assert_eq!(stage, initial_stage);
-        assert_eq!(message_opt, None);
+        assert_eq!(stage, new_stage);
+        assert_eq!(
+            message_opt,
+            Some(NodeToUiMessage {
+                target: MessageTarget::AllClients,
+                body: UiConnectionChangeBroadcast {
+                    stage: new_stage.into()
+                }
+                .tmb(0)
+            })
+        );
         TestLogHandler::new().exists_log_containing(&format!(
-            "TRACE: {}: There was an attempt to update the stage of OverallConnectionStatus \
-            from {:?} to {:?}. The request has been discarded.",
+            "DEBUG: {}: The stage of OverallConnectionStatus has been changed \
+                from {:?} to {:?}. A message to the UI was also sent.",
             test_name, initial_stage, new_stage
         ));
     }

node/src/test_utils/recorder.rs

@@ -97,6 +97,7 @@ recorder_message_handler!(AddReturnRouteMessage);
 recorder_message_handler!(AddRouteMessage);
 recorder_message_handler!(AddStreamMsg);
 recorder_message_handler!(BindMessage);
+recorder_message_handler!(ConfigurationChangeMessage);
 recorder_message_handler!(CrashNotification);
 recorder_message_handler!(DaemonBindMessage);
 recorder_message_handler!(DispatcherNodeQueryMessage);
@@ -127,7 +128,6 @@ recorder_message_handler!(ScanError);
 recorder_message_handler!(ConsumingWalletBalancesAndQualifiedPayables);
 recorder_message_handler!(SentPayables);
 recorder_message_handler!(RequestBalancesToPayPayables);
-recorder_message_handler!(ConfigurationChangeMessage);
 recorder_message_handler!(StartMessage);
 recorder_message_handler!(StreamShutdownMsg);
 recorder_message_handler!(TransmitDataMsg);