From 7386c05696cb61c3d914d9373222da8bd02908a8 Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Tue, 1 Aug 2023 09:40:22 +0530 Subject: [PATCH 1/9] GH-692: allow overall connection stage to regress --- masq_lib/src/shared_schema.rs | 1 - node/src/neighborhood/mod.rs | 1 - .../neighborhood/overall_connection_status.rs | 29 ++++++++++++------- 3 files changed, 18 insertions(+), 13 deletions(-) diff --git a/masq_lib/src/shared_schema.rs b/masq_lib/src/shared_schema.rs index 7c060e6d2..894724540 100644 --- a/masq_lib/src/shared_schema.rs +++ b/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, diff --git a/node/src/neighborhood/mod.rs b/node/src/neighborhood/mod.rs index b16d6185a..1c6c258fb 100644 --- a/node/src/neighborhood/mod.rs +++ b/node/src/neighborhood/mod.rs @@ -154,7 +154,6 @@ impl Handler 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 self.search_for_a_new_route(); } } diff --git a/node/src/neighborhood/overall_connection_status.rs b/node/src/neighborhood/overall_connection_status.rs index 04358f076..18e5b2a13 100644 --- a/node/src/neighborhood/overall_connection_status.rs +++ b/node/src/neighborhood/overall_connection_status.rs @@ -266,9 +266,8 @@ impl OverallConnectionStatus { node_to_ui_recipient: &Recipient, 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 )); } From a5ff355e2306ed1a2ed4bd42660f5438a38ec652 Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Tue, 1 Aug 2023 10:18:21 +0530 Subject: [PATCH 2/9] GH-692: regress stage of OverallConnectionStatus and send a msg to the UI --- node/src/neighborhood/mod.rs | 32 +++++++++++++++++++++++++------- 1 file changed, 25 insertions(+), 7 deletions(-) diff --git a/node/src/neighborhood/mod.rs b/node/src/neighborhood/mod.rs index 1c6c258fb..a00d21683 100644 --- a/node/src/neighborhood/mod.rs +++ b/node/src/neighborhood/mod.rs @@ -154,6 +154,16 @@ impl Handler for Neighborhood { } ConfigurationChange::UpdateMinHops(new_min_hops) => { self.set_min_hops_and_patch_size(new_min_hops); + 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(); } } @@ -3058,12 +3068,12 @@ 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 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(); subject_addr.try_send(BindMessage { peer_actors }).unwrap(); @@ -3076,19 +3086,27 @@ 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..." - )); + 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] From e34ddaebafd82fd666e75c772b25a1553061b40a Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Tue, 1 Aug 2023 11:37:12 +0530 Subject: [PATCH 3/9] GH-692: assert that the ui_gateway received a message for regression --- node/src/neighborhood/mod.rs | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/node/src/neighborhood/mod.rs b/node/src/neighborhood/mod.rs index a00d21683..569b32d30 100644 --- a/node/src/neighborhood/mod.rs +++ b/node/src/neighborhood/mod.rs @@ -1684,12 +1684,15 @@ mod tests { use super::*; use crate::accountant::test_utils::bc_from_earning_wallet; + use crate::match_every_type_id; use crate::neighborhood::overall_connection_status::ConnectionStageErrors::{ NoGossipResponseReceived, PassLoopFound, TcpConnectionFailed, }; use crate::neighborhood::overall_connection_status::{ ConnectionProgress, ConnectionStage, OverallConnectionStage, }; + use crate::test_utils::recorder_stop_conditions::StopCondition; + use crate::test_utils::recorder_stop_conditions::StopConditions; use crate::test_utils::unshared_test_utils::notify_handlers::NotifyLaterHandleMock; use masq_lib::test_utils::logging::{init_test_logging, TestLogHandler}; @@ -3070,12 +3073,13 @@ mod tests { 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 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 @@ -3100,6 +3104,18 @@ mod tests { .unwrap(); System::current().stop(); system.run(); + 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 fe94ef4cd824f0b25b93a97da1b370c26818a91b Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Tue, 1 Aug 2023 11:42:22 +0530 Subject: [PATCH 4/9] GH-692 : remove unnecessary warnings --- node/src/neighborhood/mod.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/node/src/neighborhood/mod.rs b/node/src/neighborhood/mod.rs index 569b32d30..b240c382d 100644 --- a/node/src/neighborhood/mod.rs +++ b/node/src/neighborhood/mod.rs @@ -1684,15 +1684,12 @@ mod tests { use super::*; use crate::accountant::test_utils::bc_from_earning_wallet; - use crate::match_every_type_id; use crate::neighborhood::overall_connection_status::ConnectionStageErrors::{ NoGossipResponseReceived, PassLoopFound, TcpConnectionFailed, }; use crate::neighborhood::overall_connection_status::{ ConnectionProgress, ConnectionStage, OverallConnectionStage, }; - use crate::test_utils::recorder_stop_conditions::StopCondition; - use crate::test_utils::recorder_stop_conditions::StopConditions; use crate::test_utils::unshared_test_utils::notify_handlers::NotifyLaterHandleMock; use masq_lib::test_utils::logging::{init_test_logging, TestLogHandler}; From 638057ade55d62eb6a030d24788e58006def815c Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Tue, 1 Aug 2023 11:48:33 +0530 Subject: [PATCH 5/9] GH-692: test hardening --- node/src/neighborhood/mod.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/node/src/neighborhood/mod.rs b/node/src/neighborhood/mod.rs index b240c382d..0c07cce12 100644 --- a/node/src/neighborhood/mod.rs +++ b/node/src/neighborhood/mod.rs @@ -4030,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." )); } From 48fb052fca7c0c831d64deae0fb6154804f2fb50 Mon Sep 17 00:00:00 2001 From: utkarshg6 Date: Tue, 1 Aug 2023 11:49:43 +0530 Subject: [PATCH 6/9] GH-692: reorder msg inside recorder.rs --- node/src/test_utils/recorder.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/node/src/test_utils/recorder.rs b/node/src/test_utils/recorder.rs index 1790daae2..7ec349aca 100644 --- a/node/src/test_utils/recorder.rs +++ b/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); From c07ef4a57df7e932098964ec42c2fe7eb1de48e9 Mon Sep 17 00:00:00 2001 From: Utkarsh Gupta Date: Tue, 1 Aug 2023 12:06:49 +0530 Subject: [PATCH 7/9] GH-692: minor improvements in multinode tests --- multinode_integration_tests/tests/min_hops_test.rs | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/multinode_integration_tests/tests/min_hops_test.rs b/multinode_integration_tests/tests/min_hops_test.rs index 037303434..e57910d62 100644 --- a/multinode_integration_tests/tests/min_hops_test.rs +++ b/multinode_integration_tests/tests/min_hops_test.rs @@ -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)); From 1338d7a9129e7411baa6d730e60e3d919ce4798b Mon Sep 17 00:00:00 2001 From: Utkarsh Gupta Date: Tue, 1 Aug 2023 12:09:21 +0530 Subject: [PATCH 8/9] GH-692: rename file --- .../tests/{min_hops_test.rs => min_hops_tests.rs} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename multinode_integration_tests/tests/{min_hops_test.rs => min_hops_tests.rs} (100%) diff --git a/multinode_integration_tests/tests/min_hops_test.rs b/multinode_integration_tests/tests/min_hops_tests.rs similarity index 100% rename from multinode_integration_tests/tests/min_hops_test.rs rename to multinode_integration_tests/tests/min_hops_tests.rs From 03cdd00705b02de9c34ab3d7c422fa3870a490b8 Mon Sep 17 00:00:00 2001 From: Utkarsh Gupta Date: Tue, 1 Aug 2023 12:10:27 +0530 Subject: [PATCH 9/9] GH-692: few fn renames --- multinode_integration_tests/tests/min_hops_tests.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/multinode_integration_tests/tests/min_hops_tests.rs b/multinode_integration_tests/tests/min_hops_tests.rs index e57910d62..af634444e 100644 --- a/multinode_integration_tests/tests/min_hops_tests.rs +++ b/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)