From eb907ad0f02dbfede273ba498b9aca3911ee5aa1 Mon Sep 17 00:00:00 2001
From: Nicolas Belouin <nicolas.belouin@suse.com>
Date: Thu, 13 Apr 2023 17:01:06 +0200
Subject: [PATCH] udev: Improve testability and tests

As indicated in #564 the new grouping behavior could get more test
coverage. This commit moves the grouping logic to a separate function
and adds relevant tests.

Signed-off-by: Nicolas Belouin <nicolas.belouin@suse.com>
---
 .../udev/src/discovery_handler.rs             | 17 +---
 discovery-handlers/udev/src/discovery_impl.rs | 91 ++++++++++++++++++-
 2 files changed, 92 insertions(+), 16 deletions(-)

diff --git a/discovery-handlers/udev/src/discovery_handler.rs b/discovery-handlers/udev/src/discovery_handler.rs
index c6bf4bde2..b14dfe899 100644
--- a/discovery-handlers/udev/src/discovery_handler.rs
+++ b/discovery-handlers/udev/src/discovery_handler.rs
@@ -1,5 +1,5 @@
 use super::{
-    discovery_impl::{do_parse_and_find, get_device_relatives, DeviceProperties},
+    discovery_impl::{do_parse_and_find, insert_device_with_relatives, DeviceProperties},
     wrappers::udev_enumerator,
 };
 use akri_discovery_utils::discovery::{
@@ -79,20 +79,7 @@ impl DiscoveryHandler for DiscoveryHandlerImpl {
                         if !discovery_handler_config.group_recursive {
                             devpaths.insert(path.0.clone(), vec![path]);
                         } else {
-                            match get_device_relatives(&path.0, devpaths.keys()) {
-                                (Some(parent), _) => devpaths.get_mut(&parent).unwrap().push(path),
-                                (None, children) => {
-                                    let id = path.0.clone();
-                                    let mut children_devices: Vec<DeviceProperties> = children
-                                        .into_iter()
-                                        .flat_map(|child| {
-                                            devpaths.remove(&child).unwrap().into_iter()
-                                        })
-                                        .collect();
-                                    children_devices.push(path);
-                                    let _ = devpaths.insert(id, children_devices);
-                                }
-                            }
+                            insert_device_with_relatives(&mut devpaths, path);
                         }
                     }
                 });
diff --git a/discovery-handlers/udev/src/discovery_impl.rs b/discovery-handlers/udev/src/discovery_impl.rs
index 2df740088..71fbb7fa5 100644
--- a/discovery-handlers/udev/src/discovery_impl.rs
+++ b/discovery-handlers/udev/src/discovery_impl.rs
@@ -514,7 +514,7 @@ fn device_or_parents_have_tag(device: &impl DeviceExt, value_regex: &Regex) -> b
 }
 
 /// Retrieve Parent or Children of a device using their sysfs path.
-pub fn get_device_relatives<'a>(
+fn get_device_relatives<'a>(
     device_path: &str,
     possible_relatives: impl Iterator<Item = &'a String>,
 ) -> (Option<String>, Vec<String>) {
@@ -531,6 +531,24 @@ pub fn get_device_relatives<'a>(
     (None, childrens)
 }
 
+pub fn insert_device_with_relatives(
+    devpaths: &mut std::collections::HashMap<String, Vec<DeviceProperties>>,
+    path: DeviceProperties,
+) {
+    match get_device_relatives(&path.0, devpaths.keys()) {
+        (Some(parent), _) => devpaths.get_mut(&parent).unwrap().push(path),
+        (None, children) => {
+            let id = path.0.clone();
+            let mut children_devices: Vec<DeviceProperties> = children
+                .into_iter()
+                .flat_map(|child| devpaths.remove(&child).unwrap().into_iter())
+                .collect();
+            children_devices.push(path);
+            let _ = devpaths.insert(id, children_devices);
+        }
+    }
+}
+
 #[cfg(test)]
 mod discovery_tests {
     use super::super::wrappers::udev_enumerator::{create_enumerator, MockEnumerator};
@@ -1340,4 +1358,75 @@ mod discovery_tests {
         assert_eq!(parent_4, Some(parent_path[0].clone()));
         assert_eq!(childrens_4, empty);
     }
+
+    #[test]
+    fn test_insert_device_with_relatives() {
+        let mut devpaths: HashMap<String, Vec<DeviceProperties>> = HashMap::default();
+        let related_devices = vec![
+            ("/sys/device/parent".to_string(), None),
+            (
+                "/sys/device/parent/child1".to_string(),
+                Some("/dev/dev1".to_string()),
+            ),
+            (
+                "/sys/device/parent/child1/child2".to_string(),
+                Some("/dev/dev2".to_string()),
+            ),
+        ];
+        let unrelated_device = (
+            "/sys/device/other".to_string(),
+            Some("/dev/other".to_string()),
+        );
+
+        // Add first device
+        insert_device_with_relatives(&mut devpaths, related_devices[1].clone());
+        assert_eq!(
+            devpaths,
+            HashMap::from([(
+                related_devices[1].0.clone(),
+                vec![related_devices[1].clone()]
+            )])
+        );
+
+        // Add its child
+        insert_device_with_relatives(&mut devpaths, related_devices[2].clone());
+        assert_eq!(
+            devpaths,
+            HashMap::from([(
+                related_devices[1].0.clone(),
+                vec![related_devices[1].clone(), related_devices[2].clone()]
+            )])
+        );
+
+        // Add its parent
+        insert_device_with_relatives(&mut devpaths, related_devices[0].clone());
+        assert_eq!(
+            devpaths,
+            HashMap::from([(
+                related_devices[0].0.clone(),
+                vec![
+                    related_devices[1].clone(),
+                    related_devices[2].clone(),
+                    related_devices[0].clone()
+                ]
+            )])
+        );
+
+        // Add a completely unrelated device
+        insert_device_with_relatives(&mut devpaths, unrelated_device.clone());
+        assert_eq!(
+            devpaths,
+            HashMap::from([
+                (
+                    related_devices[0].0.clone(),
+                    vec![
+                        related_devices[1].clone(),
+                        related_devices[2].clone(),
+                        related_devices[0].clone()
+                    ]
+                ),
+                (unrelated_device.0.clone(), vec![unrelated_device]),
+            ])
+        );
+    }
 }