refactor: [#1589] improve readability of UDP performance metrics race condition test

Restructures the race condition test to follow clear Arrange-Act-Assert pattern
and eliminates code duplication through helper function extraction.

The test maintains identical functionality while being more maintainable,
readable, and following DRY principles. All 200 concurrent tasks still
validate thread safety and mathematical correctness of the metrics system.

Author: josecelano

packages/udp-tracker-server/src/statistics/repository.rs

@@ -565,205 +565,211 @@ mod tests {
         assert!(result.is_ok());
     }
 
-    #[tokio::test]
-    #[allow(clippy::too_many_lines)]
-    async fn it_should_handle_race_conditions_when_updating_udp_performance_metrics_in_parallel() {
-        // Number of concurrent requests per server
-        const REQUESTS_PER_SERVER: usize = 100;
+    mod race_conditions {
 
-        let repo = Repository::new();
-        let now = CurrentClock::now();
+        use core::f64;
+        use std::time::Duration;
 
-        // Define labels for two different UDP servers
-        let server1_labels = LabelSet::from([
-            ("request_kind", "connect"),
-            ("server_binding_address_ip_family", "inet"),
-            ("server_port", "6868"),
-        ]);
-        let server2_labels = LabelSet::from([
-            ("request_kind", "connect"),
-            ("server_binding_address_ip_family", "inet"),
-            ("server_port", "6969"),
-        ]);
+        use tokio::task::JoinHandle;
+        use torrust_tracker_clock::clock::Time;
+        use torrust_tracker_metrics::metric_name;
 
-        let mut handles = vec![];
+        use super::*;
+        use crate::CurrentClock;
 
-        // Spawn tasks for server 1
-        for i in 0..REQUESTS_PER_SERVER {
-            let repo_clone = repo.clone();
-            let labels = server1_labels.clone();
-            let handle = tokio::spawn(async move {
-                // Simulate varying processing times (1000ns to 5000ns)
-                let processing_time_ns = 1000 + (i % 5) * 1000;
-                let processing_time = Duration::from_nanos(processing_time_ns as u64);
+        #[tokio::test]
+        async fn it_should_handle_race_conditions_when_updating_udp_performance_metrics_in_parallel() {
+            const REQUESTS_PER_SERVER: usize = 100;
 
-                repo_clone
-                    .recalculate_udp_avg_processing_time_ns(processing_time, &labels, now)
-                    .await
-            });
-            handles.push(handle);
-        }
+            // ** Set up test data and environment **
 
-        // Spawn tasks for server 2
-        for i in 0..REQUESTS_PER_SERVER {
-            let repo_clone = repo.clone();
-            let labels = server2_labels.clone();
-            let handle = tokio::spawn(async move {
-                // Simulate different processing times (2000ns to 6000ns)
-                let processing_time_ns = 2000 + (i % 5) * 1000;
-                let processing_time = Duration::from_nanos(processing_time_ns as u64);
+            let repo = Repository::new();
+            let now = CurrentClock::now();
 
-                repo_clone
-                    .recalculate_udp_avg_processing_time_ns(processing_time, &labels, now)
-                    .await
-            });
-            handles.push(handle);
-        }
+            let server1_labels = create_server_metric_labels("6868");
+            let server2_labels = create_server_metric_labels("6969");
 
-        // Collect all the results
-        let mut server1_results = Vec::new();
-        let mut server2_results = Vec::new();
+            // ** Execute concurrent metric updates **
 
-        for (i, handle) in handles.into_iter().enumerate() {
-            let result = handle.await.unwrap();
-            if i < REQUESTS_PER_SERVER {
-                server1_results.push(result);
-            } else {
-                server2_results.push(result);
-            }
-        }
+            // Spawn concurrent tasks for server 1 with processing times [1000, 2000, 3000, 4000, 5000] ns
+            let server1_handles = spawn_server_tasks(&repo, &server1_labels, 1000, now, REQUESTS_PER_SERVER);
 
-        // Verify that all tasks completed successfully
-        assert_eq!(server1_results.len(), REQUESTS_PER_SERVER);
-        assert_eq!(server2_results.len(), REQUESTS_PER_SERVER);
+            // Spawn concurrent tasks for server 2 with processing times [2000, 3000, 4000, 5000, 6000] ns
+            let server2_handles = spawn_server_tasks(&repo, &server2_labels, 2000, now, REQUESTS_PER_SERVER);
 
-        // Verify that all results are finite and positive
-        for result in &server1_results {
-            assert!(result.is_finite(), "Server 1 result should be finite: {result}");
-            assert!(*result > 0.0, "Server 1 result should be positive: {result}");
-        }
+            // Wait for both servers' results
+            let (server1_results, server2_results) = tokio::join!(
+                collect_concurrent_task_results(server1_handles),
+                collect_concurrent_task_results(server2_handles)
+            );
+
+            // ** Verify results and metrics **
+
+            // Verify correctness of concurrent operations
+            assert_server_results_are_valid(&server1_results, "Server 1", REQUESTS_PER_SERVER);
+            assert_server_results_are_valid(&server2_results, "Server 2", REQUESTS_PER_SERVER);
+
+            let stats = repo.get_stats().await;
 
-        for result in &server2_results {
-            assert!(result.is_finite(), "Server 2 result should be finite: {result}");
-            assert!(*result > 0.0, "Server 2 result should be positive: {result}");
+            // Verify each server's metrics individually
+            let server1_avg = assert_server_metrics_are_correct(&stats, &server1_labels, "Server 1", REQUESTS_PER_SERVER, 3000.0);
+            let server2_avg = assert_server_metrics_are_correct(&stats, &server2_labels, "Server 2", REQUESTS_PER_SERVER, 4000.0);
+
+            // Verify relationship between servers
+            assert_server_metrics_relationship(server1_avg, server2_avg);
+
+            // Verify each server's result consistency individually
+            assert_server_result_matches_stored_average(&server1_results, &stats, &server1_labels, "Server 1");
+            assert_server_result_matches_stored_average(&server2_results, &stats, &server2_labels, "Server 2");
+
+            // Verify metric collection integrity
+            assert_metric_collection_integrity(&stats);
         }
 
-        // Get final stats and verify metrics integrity
-        let stats = repo.get_stats().await;
+        // Test helper functions to hide implementation details
 
-        // Verify that the processed requests counters are correct for each server
-        #[allow(clippy::cast_sign_loss)]
-        #[allow(clippy::cast_possible_truncation)]
-        let server1_processed = stats
-            .metric_collection
-            .get_counter_value(
-                &metric_name!(UDP_TRACKER_SERVER_PERFORMANCE_AVG_PROCESSED_REQUESTS_TOTAL),
-                &server1_labels,
-            )
-            .unwrap()
-            .value();
+        fn create_server_metric_labels(port: &str) -> LabelSet {
+            LabelSet::from([
+                ("request_kind", "connect"),
+                ("server_binding_address_ip_family", "inet"),
+                ("server_port", port),
+            ])
+        }
 
-        #[allow(clippy::cast_sign_loss)]
-        #[allow(clippy::cast_possible_truncation)]
-        let server2_processed = stats
-            .metric_collection
-            .get_counter_value(
-                &metric_name!(UDP_TRACKER_SERVER_PERFORMANCE_AVG_PROCESSED_REQUESTS_TOTAL),
-                &server2_labels,
-            )
-            .unwrap()
-            .value();
+        fn spawn_server_tasks(
+            repo: &Repository,
+            labels: &LabelSet,
+            base_processing_time_ns: usize,
+            now: DurationSinceUnixEpoch,
+            requests_per_server: usize,
+        ) -> Vec<JoinHandle<f64>> {
+            let mut handles = vec![];
+
+            for i in 0..requests_per_server {
+                let repo_clone = repo.clone();
+                let labels_clone = labels.clone();
+                let handle = tokio::spawn(async move {
+                    let processing_time_ns = base_processing_time_ns + (i % 5) * 1000;
+                    let processing_time = Duration::from_nanos(processing_time_ns as u64);
+                    repo_clone
+                        .recalculate_udp_avg_processing_time_ns(processing_time, &labels_clone, now)
+                        .await
+                });
+                handles.push(handle);
+            }
 
-        assert_eq!(
-            server1_processed, REQUESTS_PER_SERVER as u64,
-            "Server 1 should have processed {REQUESTS_PER_SERVER} requests",
-        );
-        assert_eq!(
-            server2_processed, REQUESTS_PER_SERVER as u64,
-            "Server 2 should have processed {REQUESTS_PER_SERVER} requests",
-        );
+            handles
+        }
 
-        // Verify that the final average processing times are reasonable
-        #[allow(clippy::cast_sign_loss)]
-        #[allow(clippy::cast_possible_truncation)]
-        let server1_final_avg = stats
-            .metric_collection
-            .get_gauge_value(
-                &metric_name!(UDP_TRACKER_SERVER_PERFORMANCE_AVG_PROCESSING_TIME_NS),
-                &server1_labels,
-            )
-            .unwrap()
-            .value();
+        async fn collect_concurrent_task_results(handles: Vec<tokio::task::JoinHandle<f64>>) -> Vec<f64> {
+            let mut server_results = Vec::new();
 
-        #[allow(clippy::cast_sign_loss)]
-        #[allow(clippy::cast_possible_truncation)]
-        let server2_final_avg = stats
-            .metric_collection
-            .get_gauge_value(
-                &metric_name!(UDP_TRACKER_SERVER_PERFORMANCE_AVG_PROCESSING_TIME_NS),
-                &server2_labels,
-            )
-            .unwrap()
-            .value();
+            for handle in handles {
+                let result = handle.await.unwrap();
+                server_results.push(result);
+            }
 
-        // Server 1: 100 requests cycling through [1000, 2000, 3000, 4000, 5000] ns
-        // Expected average: (20×1000 + 20×2000 + 20×3000 + 20×4000 + 20×5000) / 100 = 3000 ns
-        // Note: Moving average with concurrent updates may have small deviations due to order dependency
-        assert!(
-            (server1_final_avg - 3000.0).abs() < 50.0,
-            "Server 1 final average should be close to 3000ns (±50ns), got {server1_final_avg}ns"
-        );
+            server_results
+        }
 
-        // Server 2: 100 requests cycling through [2000, 3000, 4000, 5000, 6000] ns
-        // Expected average: (20×2000 + 20×3000 + 20×4000 + 20×5000 + 20×6000) / 100 = 4000 ns
-        // Note: Moving average with concurrent updates may have small deviations due to order dependency
-        assert!(
-            (server2_final_avg - 4000.0).abs() < 50.0,
-            "Server 2 final average should be close to 4000ns (±50ns), got {server2_final_avg}ns"
-        );
+        fn assert_server_results_are_valid(results: &[f64], server_name: &str, expected_count: usize) {
+            // Verify all tasks completed
+            assert_eq!(
+                results.len(),
+                expected_count,
+                "{server_name} should have {expected_count} results"
+            );
+
+            // Verify all results are valid numbers
+            for result in results {
+                assert!(result.is_finite(), "{server_name} result should be finite: {result}");
+                assert!(*result > 0.0, "{server_name} result should be positive: {result}");
+            }
+        }
 
-        // Verify that the two servers have different averages (they should since they have different processing time ranges)
-        assert!(
-            (server1_final_avg - server2_final_avg).abs() > 950.0,
-            "Server 1 and Server 2 should have different average processing times"
-        );
+        fn assert_server_metrics_are_correct(
+            stats: &Metrics,
+            labels: &LabelSet,
+            server_name: &str,
+            expected_request_count: usize,
+            expected_avg_ns: f64,
+        ) -> f64 {
+            // Verify request count
+            let processed_requests = get_processed_requests_count(stats, labels);
+            assert_eq!(
+                processed_requests, expected_request_count as u64,
+                "{server_name} should have processed {expected_request_count} requests"
+            );
+
+            // Verify average processing time is within expected range
+            let avg_processing_time = get_average_processing_time(stats, labels);
+            assert!(
+                (avg_processing_time - expected_avg_ns).abs() < 50.0,
+                "{server_name} average should be ~{expected_avg_ns}ns (±50ns), got {avg_processing_time}ns"
+            );
+
+            avg_processing_time
+        }
 
-        // Server 2 should generally have higher averages since its processing times are higher
-        assert!(
-            server2_final_avg > server1_final_avg,
-            "Server 2 average ({server2_final_avg}) should be higher than Server 1 average ({server1_final_avg})"
-        );
+        fn assert_server_metrics_relationship(server1_avg: f64, server2_avg: f64) {
+            const MIN_DIFFERENCE_NS: f64 = 950.0;
 
-        // Verify that the moving average calculation maintains consistency
-        // The last result for each server should match the final stored average
-        let server1_last_result = server1_results.last().copied().unwrap();
-        let server2_last_result = server2_results.last().copied().unwrap();
+            assert_averages_are_significantly_different(server1_avg, server2_avg, MIN_DIFFERENCE_NS);
+            assert_server_ordering_is_correct(server1_avg, server2_avg);
+        }
 
-        // Note: Due to race conditions, the last result might not exactly match the final stored average
-        // but it should be in a reasonable range. We'll check that they're in the same ballpark.
-        let server1_diff = (server1_last_result - server1_final_avg).abs();
-        let server2_diff = (server2_last_result - server2_final_avg).abs();
+        fn assert_averages_are_significantly_different(avg1: f64, avg2: f64, min_difference: f64) {
+            let difference = (avg1 - avg2).abs();
+            assert!(
+                difference > min_difference,
+                "Server averages should differ by more than {min_difference}ns, but difference was {difference}ns"
+            );
+        }
 
-        assert!(
-            server1_diff <= 0.0,
-            "Server 1 last result ({server1_last_result}) should be equal to final average ({server1_final_avg}), diff: {server1_diff}",
+        fn assert_server_ordering_is_correct(server1_avg: f64, server2_avg: f64) {
+            // Server 2 should have higher average since it has higher processing times [2000-6000] vs [1000-5000]
+            assert!(
+            server2_avg > server1_avg,
+            "Server 2 average ({server2_avg}ns) should be higher than Server 1 ({server1_avg}ns) due to higher processing time ranges"
         );
+        }
 
-        assert!(
-            server2_diff <= 0.0,
-            "Server 2 last result ({server2_last_result}) should be equal to final average ({server2_final_avg}), diff: {server2_diff}",
-        );
+        fn assert_server_result_matches_stored_average(results: &[f64], stats: &Metrics, labels: &LabelSet, server_name: &str) {
+            let final_avg = get_average_processing_time(stats, labels);
+            let last_result = results.last().copied().unwrap();
 
-        // Verify that the metric collection contains the expected metrics for both servers
-        assert!(stats
-            .metric_collection
-            .contains_gauge(&metric_name!(UDP_TRACKER_SERVER_PERFORMANCE_AVG_PROCESSING_TIME_NS)));
-        assert!(stats
-            .metric_collection
-            .contains_counter(&metric_name!(UDP_TRACKER_SERVER_PERFORMANCE_AVG_PROCESSED_REQUESTS_TOTAL)));
+            assert!(
+                (last_result - final_avg).abs() <= f64::EPSILON,
+                "{server_name} last result ({last_result}) should match final average ({final_avg}) exactly"
+            );
+        }
 
-        println!(
-            "Race condition test completed successfully:\n  Server 1: {server1_processed} requests, final avg: {server1_final_avg}ns\n  Server 2: {server2_processed} requests, final avg: {server2_final_avg}ns"
-        );
+        fn assert_metric_collection_integrity(stats: &Metrics) {
+            assert!(stats
+                .metric_collection
+                .contains_gauge(&metric_name!(UDP_TRACKER_SERVER_PERFORMANCE_AVG_PROCESSING_TIME_NS)));
+            assert!(stats
+                .metric_collection
+                .contains_counter(&metric_name!(UDP_TRACKER_SERVER_PERFORMANCE_AVG_PROCESSED_REQUESTS_TOTAL)));
+        }
+
+        fn get_processed_requests_count(stats: &Metrics, labels: &LabelSet) -> u64 {
+            stats
+                .metric_collection
+                .get_counter_value(
+                    &metric_name!(UDP_TRACKER_SERVER_PERFORMANCE_AVG_PROCESSED_REQUESTS_TOTAL),
+                    labels,
+                )
+                .unwrap()
+                .value()
+        }
+
+        fn get_average_processing_time(stats: &Metrics, labels: &LabelSet) -> f64 {
+            stats
+                .metric_collection
+                .get_gauge_value(&metric_name!(UDP_TRACKER_SERVER_PERFORMANCE_AVG_PROCESSING_TIME_NS), labels)
+                .unwrap()
+                .value()
+        }
     }
 }