[TESTS] Add unit tests for monitoring

[cachebag]

Describe the issue
The wifi.rs module has no unit tests, we should add some similar to bluetooth.rs.

[Rifat-R]

Got a few questions, still learning 😅 :

Within wifi.rs,

pub(crate) async fn current_ssid(conn: &Connection) -> Option<String> {
    let nm = try_log!(NMProxy::new(conn).await, "Failed to create NM proxy");
    let devices = try_log!(nm.get_devices().await, "Failed to get devices");

    for dp in devices {
        let dev_builder = try_log!(
            NMDeviceProxy::builder(conn).path(dp.clone()),
            "Failed to create device proxy builder"
        );
        let dev = try_log!(dev_builder.build().await, "Failed to build device proxy");

        let dev_type = try_log!(dev.device_type().await, "Failed to get device type");
        if dev_type != device_type::WIFI {
            continue;
        }

try_log macro

/// Macro to convert Result to Option with error logging.
/// Usage: `try_log!(result, "context message")?`
#[macro_export]
macro_rules! try_log {
    ($result:expr, $context:expr) => {
        match $result {
            Ok(value) => value,
            Err(e) => {
                log::warn!("{}: {:?}", $context, e);
                return None;
            }
        }
    };
}

Here, if we fail to e.g. get device type, wouldn't it return from the entire function and therefore ending the search for a device early? Not sure if that's the behaviour we want. I ask this since we continue here if device isn't wifi

        if dev_type != device_type::WIFI {
            continue;
        }

And also, how would testing occur here? d-bus connection is used and coupled tightly within the functions so I'm not entirely sure how to extract logic here for testing or whether to mock live connection.

[cachebag]

Here, if we fail to e.g. get device type, wouldn't it return from the entire function and therefore ending the search for a device early? Not sure if that's the behaviour we want. I ask this since we continue here if device isn't wifi

Nice eye! Yeah it looks like we should use continue instead of return None for per-device errors...or perhaps a try_continue macro? Something like:

macro_rules! try_continue {
    ($result:expr, $context:expr) => {
        match $result {
            Ok(value) => value,
            Err(e) => {
                log::warn!("{}: {:?}", $context, e);
                continue; 
            }
        }
    };
}

And also, how would testing occur here? d-bus connection is used and coupled tightly within the functions so I'm not entirely sure how to extract logic here for testing or whether to mock live connection.

You can extract D-Bus operations behind traits that can be mocked. I have been meaning to do something like this:

#[async_trait]
trait DeviceProxy {
    async fn device_type(&self) -> Result<u32>;
    // ...etc
}

// Assuming we have NM available
struct NMDeviceProxyImpl { /* ... */ }

// Mock for testing
struct MockDeviceProxy {
    device_type_result: Result<u32>,
}

Truthfully, for our integration tests, we rely on a running instance of NetworkManager on Linux with physical devices. It's a limitation right now but development is scattered and niche so I haven't put effort into a good solution (especially for CI). I would suggest trying the Dockerfile if you don't have access to a Linux machine, which should simulate NM and DBus.

Feel free to let me know if you have any more questions or if what I said wasn't clear :)

[Rifat-R]

Thanks for the detailed explanation, it all made sense 😁

Will take a look at refactoring the functions within wifi.rs and also omit using try_log! within per device check loop.

You can extract D-Bus operations behind traits that can be mocked. I have been meaning to do something like this:

Ahh very nice, will spend some time learning about this and take a crack at it :)