Refactor helper functions and more

* Move helper functions from SyncHelper to utils.py
* Obtain default exporter port constant from config.py
* Refactor parse_metrics to simplify logic
* Change test_config_collector_enabled so that it passes only when all
  enabled collectors for the hardware are provided

tests/functional/conftest.py

@@ -1,34 +1,10 @@
 import logging
 
 import pytest
-from async_lru import alru_cache
 
 log = logging.getLogger(__name__)
 
 
-class SyncHelper:
-    """Helper class for running juju async function."""
-
-    @staticmethod
-    async def run_command_on_unit(ops_test, unit_name, command):
-        complete_command = ["exec", "--unit", unit_name, "--", *command.split()]
-        return_code, stdout, _ = await ops_test.juju(*complete_command)
-        results = {
-            "return-code": return_code,
-            "stdout": stdout,
-        }
-        return results
-
-    @staticmethod
-    @alru_cache
-    async def get_metrics_output(ops_test, unit_name):
-        """Return prometheus metric output from endpoint on unit."""
-        port = 10200
-        command = f"curl localhost:{port}"
-        results = await SyncHelper.run_command_on_unit(ops_test, unit_name, command)
-        return results
-
-
 def pytest_addoption(parser):
     parser.addoption(
         "--series",
@@ -64,7 +40,7 @@ def series(request):
 
 @pytest.fixture(scope="module")
 def provided_collectors(request):
-    return request.config.getoption("collectors")
+    return set(request.config.getoption("collectors"))
 
 
 def pytest_configure(config):
@@ -83,11 +59,6 @@ def pytest_collection_modifyitems(config, items):
             item.add_marker(skip_real_hw)
 
 
-@pytest.fixture(scope="module")
-def sync_helper():
-    return SyncHelper
-
-
 @pytest.fixture()
 def app(ops_test):
     return ops_test.model.applications["hardware-observer"]

tests/functional/test_charm.py

@@ -6,16 +6,13 @@
 import inspect
 import logging
 import os
-import re
-from collections import defaultdict
-from dataclasses import dataclass
 from enum import Enum
 from pathlib import Path
-from typing import Optional
 
 import pytest
 import yaml
 from pytest_operator.plugin import OpsTest
+from utils import get_metrics_output, parse_metrics, run_command_on_unit
 
 logger = logging.getLogger(__name__)
 
@@ -27,116 +24,12 @@
 TIMEOUT = 600
 
 
-@dataclass
-class Metric:
-    """Class for metric data."""
-
-    name: str
-    labels: Optional[str]
-    value: float
-
-
 def get_this_script_dir() -> Path:
     filename = inspect.getframeinfo(inspect.currentframe()).filename  # type: ignore[arg-type]
     path = os.path.dirname(os.path.abspath(filename))
     return Path(path)
 
 
-def _parse_single_metric(metric: str) -> Optional[Metric]:
-    """Return a Metric object parsed from a single metric string."""
-    # ignore blank lines or comments
-    if not metric or metric.startswith("#"):
-        return None
-
-    # The regex pattern below uses named capturing groups to extract aspects of the metric.
-    # (?P<name>[^\s{]+) captures the metric name and matches one or more chars that are not
-    # whitespace or opening curly brace '{'
-
-    # (?:{(?P<label>[^}]*)})? handles optional labels It uses a non-capturing group (?:...)
-    # to make the entire portion optional. Inside this group:
-    #   {               : Matches an opening curly brace.
-    #   (?P<label>[^}]*): Another named capturing group (label) that matches zero or more
-    #                     characters that are not a closing curly brace ([^}]*).
-    #   }               : Matches a closing curly brace.
-
-    # (?P<value>\d+\.\d+|\d+): This part captures the numeric value.
-    # It uses a named capturing group (value) and matches either:
-    #   \d+\.\d+  : For obtaining floating point values containing a dot.
-    #   |\d+      : OR, one or more digits for integer values.
-
-    pattern = re.compile(r"(?P<name>[^\s{]+)(?:\{(?P<labels>[^}]*)\})?\s+(?P<value>\d+\.\d+|\d+)")
-    match = pattern.match(metric)
-
-    if match:
-        name = match.group("name")
-        labels = match.group("labels") or None
-        value = float(match.group("value"))
-        return Metric(name=name, labels=labels, value=value)
-    else:
-        return None
-
-
-def parse_metrics(metrics_input: str) -> dict[str, list[Metric]]:
-    """Parse raw metrics and return dictionary of parsed Metric objects for each collector.
-
-    For example, parsing this metrics_input,
-        # HELP ipmi_temperature_celsius Temperature measure from temperature sensors
-        # TYPE ipmi_temperature_celsius gauge
-        ipmi_temperature_celsius{name="Exhaust Temp",state="Nominal",unit="C"} 52.0
-
-        # HELP ipmi_power_watts Power measure from power sensors
-        # TYPE ipmi_power_watts gauge
-        ipmi_power_watts{name="Sys Fan Pwr",state="Nominal",unit="W"} 20.0
-
-        # HELP megaraid_virtual_drives Number of virtual drives
-        # TYPE megaraid_virtual_drives gauge
-        megaraid_virtual_drives{controller_id="0"} 1.0
-
-        # HELP redfish_call_success Indicates if call to the redfish API succeeded or not.
-        # TYPE redfish_call_success gauge
-        redfish_call_success 0.0
-
-    would return,
-        {
-          "ipmi_sensors": [
-                            Metric(name='ipmi_temperature_celsius',
-                            labels='name="Exhaust Temp",state="Nominal",unit="C"',
-                            value=52.0), Metric(name='ipmi_power_watts',
-                            labels='name="Sys Fan Pwr",state="Nominal",unit="W"',
-                            value=20.0)
-                          ],
-          "mega_raid":    [
-                            Metric(name='megaraid_virtual_drives',
-                            labels='controller_id="0"',
-                            value=1.0)
-                          ],
-          "redfish":      [Metric(name='redfish_call_success', labels=None, value=0.0)],
-        }
-    """
-    metrics_list: Optional[list[Metric]] = []
-
-    for line in metrics_input.split("\n"):
-        if metric := _parse_single_metric(line):
-            metrics_list.append(metric)
-
-    parsed_metrics = defaultdict(list)
-    for metric in metrics_list:
-        name = metric.name
-        if name.startswith("redfish"):
-            parsed_metrics["redfish"].append(metric)
-        elif name.startswith("ipmi_dcmi"):
-            parsed_metrics["ipmi_dcmi"].append(metric)
-        elif name.startswith("ipmi_sel"):
-            parsed_metrics["ipmi_sel"].append(metric)
-        elif name.startswith("ipmi"):
-            parsed_metrics["ipmi_sensors"].append(metric)
-        elif name.startswith("poweredgeraid"):
-            parsed_metrics["poweredge_raid"].append(metric)
-        elif name.startswith("megaraid") or name.startswith("storcli"):
-            parsed_metrics["mega_raid"].append(metric)
-    return parsed_metrics
-
-
 class AppStatus(str, Enum):
     """Various workload status messages for the app."""
 
@@ -150,7 +43,7 @@ class AppStatus(str, Enum):
 
 @pytest.mark.abort_on_fail
 @pytest.mark.skip_if_deployed
-async def test_build_and_deploy(ops_test: OpsTest, series, sync_helper):
+async def test_build_and_deploy(ops_test: OpsTest, series):
     """Build the charm-under-test and deploy it together with related charms.
 
     Assert on the unit status before any relations/configurations take place.
@@ -209,7 +102,7 @@ async def test_build_and_deploy(ops_test: OpsTest, series, sync_helper):
 
     # Test without cos-agent relation
     for unit in ops_test.model.applications[APP_NAME].units:
-        results = await sync_helper.run_command_on_unit(ops_test, unit.name, check_active_cmd)
+        results = await run_command_on_unit(ops_test, unit.name, check_active_cmd)
         assert results.get("return-code") > 0
         assert results.get("stdout").strip() == "inactive"
 
@@ -227,7 +120,7 @@ async def test_build_and_deploy(ops_test: OpsTest, series, sync_helper):
 
     # Test with cos-agent relation
     for unit in ops_test.model.applications[APP_NAME].units:
-        results = await sync_helper.run_command_on_unit(ops_test, unit.name, check_active_cmd)
+        results = await run_command_on_unit(ops_test, unit.name, check_active_cmd)
         assert results.get("return-code") == 0
         assert results.get("stdout").strip() == "active"
         assert unit.workload_status_message == AppStatus.READY
@@ -237,20 +130,24 @@ async def test_build_and_deploy(ops_test: OpsTest, series, sync_helper):
 class TestCharmWithHW:
     """Run functional tests that require specific hardware."""
 
-    async def test_config_collector_enabled(
-        self, app, unit, sync_helper, ops_test, provided_collectors
-    ):
+    async def test_config_collector_enabled(self, app, unit, ops_test, provided_collectors):
         """Test whether provided collectors are present in exporter config."""
         cmd = "cat /etc/hardware-exporter-config.yaml"
-        results = await sync_helper.run_command_on_unit(ops_test, unit.name, cmd)
+        results = await run_command_on_unit(ops_test, unit.name, cmd)
         assert results.get("return-code") == 0
         config = yaml.safe_load(results.get("stdout").strip())
-        for collector_name in provided_collectors:
-            assert f"collector.{collector_name}" in config.get("enable_collectors")
+        collectors_in_config = {
+            collector.replace("collector.", "") for collector in config.get("enable_collectors")
+        }
+        error_msg = (
+            f"Provided collectors {provided_collectors} are different from"
+            f" enabled collectors in config {collectors_in_config}"
+        )
+        assert provided_collectors == collectors_in_config, error_msg
 
-    async def test_metrics_available(self, app, unit, sync_helper, ops_test):
+    async def test_metrics_available(self, app, unit, ops_test):
         """Test if metrics are available at the expected endpoint on unit."""
-        results = await sync_helper.get_metrics_output(ops_test, unit.name)
+        results = await get_metrics_output(ops_test, unit.name)
         assert results.get("return-code") == 0, "Metrics output not available"
 
     @pytest.mark.parametrize(
@@ -267,22 +164,22 @@ async def test_metrics_available(self, app, unit, sync_helper, ops_test):
         ],
     )
     async def test_collector_specific_metrics_available(
-        self, ops_test, app, unit, sync_helper, provided_collectors, collector
+        self, ops_test, app, unit, provided_collectors, collector
     ):
         """Test if metrics specific to provided collectors are present."""
         if collector not in provided_collectors:
             pytest.skip(f"{collector} not in provided collectors, skipping test")
 
         # collector is available, proceed to run collector specific tests
-        results = await sync_helper.get_metrics_output(ops_test, unit.name)
+        results = await get_metrics_output(ops_test, unit.name)
         parsed_metrics = parse_metrics(results.get("stdout").strip())
         assert parsed_metrics.get(collector), f"{collector} specific metrics are not available."
 
 
 class TestCharm:
     """Perform basic functional testing of the charm without having the actual hardware."""
 
-    async def test_config_changed_port(self, app, unit, sync_helper, ops_test):
+    async def test_config_changed_port(self, app, unit, ops_test):
         """Test changing the config option: exporter-port."""
         new_port = "10001"
         await asyncio.gather(
@@ -291,14 +188,14 @@ async def test_config_changed_port(self, app, unit, sync_helper, ops_test):
         )
 
         cmd = "cat /etc/hardware-exporter-config.yaml"
-        results = await sync_helper.run_command_on_unit(ops_test, unit.name, cmd)
+        results = await run_command_on_unit(ops_test, unit.name, cmd)
         assert results.get("return-code") == 0
         config = yaml.safe_load(results.get("stdout").strip())
         assert config["port"] == int(new_port)
 
         await app.reset_config(["exporter-port"])
 
-    async def test_config_changed_log_level(self, app, unit, sync_helper, ops_test):
+    async def test_config_changed_log_level(self, app, unit, ops_test):
         """Test changing the config option: exporter-log-level."""
         new_log_level = "DEBUG"
         await asyncio.gather(
@@ -307,14 +204,14 @@ async def test_config_changed_log_level(self, app, unit, sync_helper, ops_test):
         )
 
         cmd = "cat /etc/hardware-exporter-config.yaml"
-        results = await sync_helper.run_command_on_unit(ops_test, unit.name, cmd)
+        results = await run_command_on_unit(ops_test, unit.name, cmd)
         assert results.get("return-code") == 0
         config = yaml.safe_load(results.get("stdout").strip())
         assert config["level"] == new_log_level
 
         await app.reset_config(["exporter-log-level"])
 
-    async def test_start_and_stop_exporter(self, app, unit, sync_helper, ops_test):
+    async def test_start_and_stop_exporter(self, app, unit, ops_test):
         """Test starting and stopping the exporter results in correct charm status."""
         # Stop the exporter, and the exporter should auto-restart after update status fire.
         stop_cmd = "systemctl stop hardware-exporter"
@@ -325,7 +222,7 @@ async def test_start_and_stop_exporter(self, app, unit, sync_helper, ops_test):
             )
             assert unit.workload_status_message == AppStatus.READY
 
-    async def test_exporter_failed(self, app, unit, sync_helper, ops_test):
+    async def test_exporter_failed(self, app, unit, ops_test):
         """Test failure in the exporter results in correct charm status."""
         # Setting incorrect log level will crash the exporter
         async with ops_test.fast_forward():
@@ -342,7 +239,7 @@ async def test_exporter_failed(self, app, unit, sync_helper, ops_test):
             )
             assert unit.workload_status_message == AppStatus.READY
 
-    async def test_on_remove_event(self, app, sync_helper, ops_test):
+    async def test_on_remove_event(self, app, ops_test):
         """Test _on_remove event cleans up the service on the host machine."""
         await asyncio.gather(
             app.remove_relation(f"{APP_NAME}:general-info", f"{PRINCIPAL_APP_NAME}:juju-info"),
@@ -358,11 +255,11 @@ async def test_on_remove_event(self, app, sync_helper, ops_test):
         )
 
         cmd = "ls /etc/hardware-exporter-config.yaml"
-        results = await sync_helper.run_command_on_unit(ops_test, principal_unit.name, cmd)
+        results = await run_command_on_unit(ops_test, principal_unit.name, cmd)
         assert results.get("return-code") > 0
 
         cmd = "ls /etc/systemd/system/hardware-exporter.service"
-        results = await sync_helper.run_command_on_unit(ops_test, principal_unit.name, cmd)
+        results = await run_command_on_unit(ops_test, principal_unit.name, cmd)
         assert results.get("return-code") > 0
 
         await asyncio.gather(