fix(components): correctly pass vis to component unload method

Author: roflcoopter

tests/components/test__init__.py

@@ -3,7 +3,7 @@
 from __future__ import annotations
 
 import logging
-from typing import Literal
+from typing import TYPE_CHECKING, Literal, cast
 from unittest.mock import ANY, Mock, call, patch
 
 import pytest
@@ -23,7 +23,11 @@
 from viseron.exceptions import ComponentNotReady
 
 from tests.common import MockComponent, MockComponentModule
-from tests.conftest import MockViseron
+
+if TYPE_CHECKING:
+    from types import ModuleType
+
+    from tests.conftest import MockViseron
 
 
 class TestSetupComponents:
@@ -163,6 +167,7 @@ def test_add_domain_to_setup(
         self, vis: MockViseron, caplog: pytest.LogCaptureFixture
     ) -> None:
         """Test add_domain_to_setup."""
+        caplog.set_level(logging.DEBUG)
         registry = vis.domain_registry
 
         component1 = Component(vis, "component1", "component1", {})
@@ -180,8 +185,8 @@ def test_add_domain_to_setup(
         assert entry is not None
         assert entry.component_name == "component1"
         assert (
-            "Domain object_detector with identifier identifier1 already in setup "
-            "queue. Skipping setup of domain object_detector with identifier "
+            "Domain object_detector with identifier identifier1 already pending setup. "
+            "Skipping setup of domain object_detector with identifier "
             "identifier1 for component component2" in caplog.text
         )
         caplog.clear()
@@ -196,7 +201,7 @@ def test_get_component_success(self, vis: MockViseron) -> None:
             result = component.get_component()
 
             mock_import.assert_called_once_with("viseron.components.test")
-            assert result == mock_module
+            assert result == cast("ModuleType", mock_module)
 
 
 class TestComponentConfigValidation:
@@ -218,7 +223,7 @@ def test_validate_config_valid_schema(self, vis: MockViseron) -> None:
         """Test validation with valid CONFIG_SCHEMA."""
         validated_config: dict[str, bool] = {"validated": True}
 
-        def schema(_config) -> dict[str, bool]:
+        def schema(_config: dict[str, bool]) -> dict[str, bool]:
             return validated_config
 
         mock_module = MockComponentModule(config_schema=schema)

viseron/components/__init__.py

@@ -5,10 +5,9 @@
 import importlib
 import logging
 import time
-import traceback
 from concurrent.futures import ThreadPoolExecutor, as_completed
 from timeit import default_timer as timer
-from typing import TYPE_CHECKING, Any
+from typing import TYPE_CHECKING, Any, ClassVar
 
 import voluptuous as vol
 from voluptuous.humanize import humanize_error
@@ -30,6 +29,8 @@
 from viseron.watchdog.thread_watchdog import RestartableThread
 
 if TYPE_CHECKING:
+    from types import ModuleType
+
     from viseron import Viseron
     from viseron.domains import OptionalDomain, RequireDomain
     from viseron.viseron_types import SupportedDomains
@@ -51,7 +52,7 @@
 class Component:
     """Represents a Viseron component."""
 
-    retry_timers: dict[str, NamedTimer] = {}
+    retry_timers: ClassVar[dict[str, NamedTimer]] = {}
 
     def __init__(
         self,
@@ -79,7 +80,7 @@ def path(self) -> str:
         """Return component path."""
         return self._path
 
-    def get_component(self):
+    def get_component(self) -> ModuleType:
         """Return component module."""
         return importlib.import_module(self._path)
 
@@ -92,20 +93,21 @@ def validate_component_config(self) -> dict | bool | None:
             except vol.Invalid as ex:
                 LOGGER.exception(
                     f"Error validating config for component {self.name}: "
-                    f"{humanize_error(self._config, ex)}"
+                    f"{humanize_error(self._config, ex)}"  # noqa: TRY401
                 )
                 return None
             except Exception:  # pylint: disable=broad-except
                 LOGGER.exception("Unknown error calling %s CONFIG_SCHEMA", self.name)
                 return None
         return True
 
-    def setup_component(self, tries: int = 1, domains_only=False) -> bool:
+    def setup_component(self, tries: int = 1, *, domains_only: bool = False) -> bool:
         """Set up component."""
         LOGGER.info(
-            "Setting up component %s%s",
+            "Setting up component %s%s%s",
             self.name,
             (f", attempt {tries}" if tries > 1 else ""),
+            (" (domains only)" if domains_only else ""),
         )
         slow_setup_warning = NamedTimer(
             SLOW_SETUP_WARNING,
@@ -131,15 +133,13 @@ def setup_component(self, tries: int = 1, domains_only=False) -> bool:
                 # setup() is optional for stateless components
                 if hasattr(component_module, "setup") and not domains_only:
                     result = component_module.setup(self._vis, config)
+                # No setup function, assume success if setup_domains exists
+                elif hasattr(component_module, "setup_domains"):
+                    result = True
                 else:
-                    # No setup function, assume success if setup_domains exists
-                    if hasattr(component_module, "setup_domains"):
-                        result = True
-                    else:
-                        LOGGER.error(
-                            f"Component {self.name} has neither setup() "
-                            "nor setup_domains()"
-                        )
+                    LOGGER.error(
+                        f"Component {self.name} has neither setup() nor setup_domains()"
+                    )
             except ComponentNotReady as error:
                 if self._vis.shutdown_event.is_set():
                     LOGGER.warning(
@@ -153,7 +153,7 @@ def setup_component(self, tries: int = 1, domains_only=False) -> bool:
                 LOGGER.error(
                     f"Component {self.name} is not ready. "
                     f"Retrying in {wait_time} seconds in the background. "
-                    f"Error: {str(error)}"
+                    f"Error: {error!s}"
                 )
                 retry_timer = NamedTimer(
                     wait_time,
@@ -178,11 +178,8 @@ def cancel_retry_timer() -> None:
                     VISERON_SIGNAL_SHUTDOWN, cancel_retry_timer
                 )
                 retry_timer.start()
-            except Exception as ex:  # pylint: disable=broad-except
-                LOGGER.error(
-                    f"Uncaught exception setting up component {self.name}: {ex}\n"
-                    f"{traceback.format_exc()}"
-                )
+            except Exception:  # pylint: disable=broad-except
+                LOGGER.exception(f"Uncaught exception setting up component {self.name}")
             finally:
                 slow_setup_warning.cancel()
 
@@ -192,10 +189,9 @@ def cancel_retry_timer() -> None:
             if hasattr(component_module, "setup_domains"):
                 try:
                     component_module.setup_domains(self._vis, config)
-                except Exception as ex:  # pylint: disable=broad-except
-                    LOGGER.error(
-                        f"Uncaught exception in setup_domains for component "
-                        f"{self.name}: {ex}\n{traceback.format_exc()}"
+                except Exception:  # pylint: disable=broad-except
+                    LOGGER.exception(
+                        f"Uncaught exception in setup_domains for component {self.name}"
                     )
                     return False
             LOGGER.info(
@@ -238,11 +234,14 @@ def add_domain_to_setup(
         # Check if already registered (any state)
         existing = registry.get(domain, identifier)
         if existing:
-            LOGGER.warning(
-                f"Domain {domain} with identifier {identifier} already in setup queue. "
-                f"Skipping setup of domain {domain} with identifier {identifier} for "
-                f"component {self.name}",
-            )
+            if existing.state == DomainState.PENDING:
+                LOGGER.warning(
+                    f"Domain {domain} with identifier {identifier} "
+                    "already pending setup. "
+                    f"Skipping setup of domain {domain} "
+                    f"with identifier {identifier} for "
+                    f"component {self.name}",
+                )
             return None
 
         return registry.register(
@@ -274,7 +273,7 @@ def get_component(
 
 
 def setup_component(
-    vis: Viseron, component: Component, tries: int = 1, domains_only=False
+    vis: Viseron, component: Component, tries: int = 1, *, domains_only: bool = False
 ) -> None:
     """Set up single component."""
     # When tries is larger than one, it means we are in a retry loop.
@@ -340,9 +339,9 @@ def unload_component(vis: Viseron, component: str) -> set[str] | None:
     component_module = component_instance.get_component()
     if hasattr(component_module, "unload"):
         try:
-            component_module.unload()
-        except Exception as ex:  # pylint: disable=broad-except
-            LOGGER.error(f"Error unloading component {component}: {ex}")
+            component_module.unload(vis)
+        except Exception:  # pylint: disable=broad-except
+            LOGGER.exception(f"Error unloading component {component}")
     else:
         LOGGER.debug(f"Component {component} has no unload method")
 
@@ -359,7 +358,7 @@ class CriticalComponentsConfigStore:
     Used to store the last known good config for critical components.
     """
 
-    def __init__(self, vis) -> None:
+    def __init__(self, vis: Viseron) -> None:
         self._vis = vis
         self._store = Storage(vis, STORAGE_KEY)
 
@@ -448,27 +447,26 @@ def setup_components(
         return
 
     # Setup components in parallel
-    setup_threads = []
-    for component in (
-        components_to_setup
-        - set(LOGGING_COMPONENTS)
-        - set(CORE_COMPONENTS)
-        - set(DEFAULT_COMPONENTS)
-    ):
-        setup_threads.append(
-            RestartableThread(
-                target=setup_component,
-                args=(vis, get_component(vis, component, config)),
-                kwargs={"domains_only": domains_only},
-                name=f"{component}_setup",
-                daemon=True,
-                register=False,
-            )
+    setup_threads = [
+        RestartableThread(
+            target=setup_component,
+            args=(vis, get_component(vis, component, config)),
+            kwargs={"domains_only": domains_only},
+            name=f"{component}_setup",
+            daemon=True,
+            register=False,
+        )
+        for component in (
+            components_to_setup
+            - set(LOGGING_COMPONENTS)
+            - set(CORE_COMPONENTS)
+            - set(DEFAULT_COMPONENTS)
         )
+    ]
     for thread in setup_threads:
         thread.start()
 
-    def join(thread) -> None:
+    def join(thread: RestartableThread) -> None:
         thread.join(timeout=30)
         time.sleep(0.5)  # Wait for thread to exit properly
         if thread.is_alive():