Review: Fixed tests and made log_to_file to log_directory

.pylintrc

@@ -43,7 +43,9 @@ disable=
   too-many-statements,
   too-many-boolean-expressions,
   unused-argument,
-  wrong-import-order
+  wrong-import-order,
+  logging-not-lazy,
+  logging-fstring-interpolation
 # enable useless-suppression temporarily every now and then to clean them up
 enable=
   use-symbolic-message-instead

docs/xknx.md

@@ -28,7 +28,7 @@ xknx = XKNX(config='xknx.yaml',
             rate_limit=DEFAULT_RATE_LIMIT,
             multicast_group=DEFAULT_MCAST_GRP,
             multicast_port=DEFAULT_MCAST_PORT,
-            log_to_file=False)
+            log_directory=None)
 ```
 
 The constructor of the XKNX object takes several parameters:
@@ -45,7 +45,7 @@ The constructor of the XKNX object takes several parameters:
 * `rate_limit` in telegrams per second - can be used to limit the outgoing traffic to the KNX/IP interface. The default value is 20 packets per second.
 * `multicast_group` is the multicast IP address - can be used to override the default multicast address (`224.0.23.12`)
 * `multicast_port` is the multicast port - can be used to override the default multicast port (`3671`)
-* `log_to_file` is a boolean - when set to True we log to a dedicated file `xknx.log`
+* `log_directory` is the path to the log directory - when set to a valid directory we log to a dedicated file in this directory called `xknx.log`. The log files are rotated each night and will exist for 7 days. After that the oldest one will be deleted.
 
 # [](#header-2)Starting
 

test/xknx_test.py

@@ -2,14 +2,22 @@
 import asyncio
 import os
 import unittest
-from unittest.mock import patch
+from unittest.mock import MagicMock, patch
 
 from xknx import XKNX
 
-# pylint: disable=too-many-public-methods,invalid-name
+# pylint: disable=too-many-public-methods,invalid-name,useless-super-delegation,protected-access
 from xknx.io import ConnectionConfig, ConnectionType
 
 
+class AsyncMock(MagicMock):
+    """Async Mock."""
+
+    # pylint: disable=invalid-overridden-method
+    async def __call__(self, *args, **kwargs):
+        return super().__call__(*args, **kwargs)
+
+
 class TestXknxModule(unittest.TestCase):
     """Test class for XKNX."""
 
@@ -24,18 +32,23 @@ def tearDown(self):
 
     def test_log_to_file(self):
         """Test logging enable."""
-        xknx = XKNX(loop=self.loop, log_to_file=True)
+        XKNX(loop=self.loop, log_directory="/tmp/")
+
+        self.assertTrue(os.path.isfile("/tmp/xknx.log"))
+
+        os.remove("/tmp/xknx.log")
 
-        self.assertTrue(os.path.isfile("xknx.log"))
+    def test_log_to_file_when_dir_does_not_exist(self):
+        """Test logging enable with non existent directory."""
+        XKNX(loop=self.loop, log_directory="/xknx/is/fun")
 
-        os.remove("xknx.log")
+        self.assertFalse(os.path.isfile("/xknx/is/fun/xknx.log"))
 
     def test_register_telegram_cb(self):
         """Test register telegram callback."""
 
         async def telegram_received(telegram):
             """Telegram received."""
-            pass  # tested in telegram queue
 
         xknx = XKNX(loop=self.loop, telegram_received_cb=telegram_received)
 
@@ -46,24 +59,24 @@ def test_register_device_cb(self):
 
         async def device_update(device):
             """Device updated."""
-            pass  # tested in devices
 
         xknx = XKNX(loop=self.loop, device_updated_cb=device_update)
 
         self.assertEqual(len(xknx.devices.device_updated_cbs), 1)
 
-    @patch("xknx.io.KNXIPInterface.start")
+    @patch("xknx.io.KNXIPInterface.start", new_callable=AsyncMock)
     def test_xknx_start(self, start_mock):
         """Test xknx start."""
         xknx = XKNX(loop=self.loop)
 
         self.loop.run_until_complete(xknx.start(state_updater=True))
 
         start_mock.assert_called_once()
+        self.loop.run_until_complete(xknx.stop())
 
-    @patch("xknx.io.KNXIPInterface.start")
-    def test_xknx_start_with_dedicated_connection_config(self, start_mock):
-        """Test xknx start with connection config."""
+    @patch("xknx.io.KNXIPInterface.start", new_callable=AsyncMock)
+    def test_xknx_start_and_stop_with_dedicated_connection_config(self, start_mock):
+        """Test xknx start and stop with connection config."""
         xknx = XKNX(loop=self.loop)
 
         connection_config = ConnectionConfig(connection_type=ConnectionType.TUNNELING)
@@ -74,10 +87,7 @@ def test_xknx_start_with_dedicated_connection_config(self, start_mock):
         start_mock.assert_called_once()
         self.assertEqual(xknx.knxip_interface.connection_config, connection_config)
 
-        with patch("xknx.core.TelegramQueue.stop") as tq_stop, patch(
-            "xknx.core.StateUpdater.stop"
-        ) as updater_stop:
-            self.loop.run_until_complete(xknx.stop())
-            tq_stop.assert_called_once()
-            updater_stop.assert_called_once()
-            self.assertIsNone(xknx.knxip_interface)
+        self.loop.run_until_complete(xknx.stop())
+        self.assertIsNone(xknx.knxip_interface)
+        self.assertTrue(xknx.telegram_queue._consumer_task.done())
+        self.assertFalse(xknx.state_updater.started)

xknx/io/knxip_interface.py

@@ -173,7 +173,8 @@ async def send_telegram(self, telegram):
         """Send telegram to connected device (either Tunneling or Routing)."""
         await self.interface.send_telegram(telegram)
 
-    def find_local_ip(self, gateway_ip: str) -> str:
+    @staticmethod
+    def find_local_ip(gateway_ip: str) -> str:
         """Find local IP address on same subnet as gateway."""
 
         def _scan_interfaces(gateway: ipaddress.IPv4Address) -> str:

xknx/xknx.py

@@ -2,6 +2,7 @@
 import asyncio
 import logging
 from logging.handlers import TimedRotatingFileHandler
+import os
 import signal
 from sys import platform
 
@@ -39,7 +40,7 @@ def __init__(
         rate_limit=DEFAULT_RATE_LIMIT,
         multicast_group=DEFAULT_MCAST_GRP,
         multicast_port=DEFAULT_MCAST_PORT,
-        log_to_file=False,
+        log_directory=None,
     ):
         """Initialize XKNX class."""
         # pylint: disable=too-many-arguments
@@ -59,8 +60,8 @@ def __init__(
         self.connection_config = None
         self.version = VERSION
 
-        if log_to_file:
-            self.setup_logging()
+        if log_directory is not None:
+            self.setup_logging(log_directory)
 
         if config is not None:
             Config(self).read(config)
@@ -136,10 +137,17 @@ def sigint_handler():
         await self.sigint_received.wait()
 
     @staticmethod
-    def setup_logging():
+    def setup_logging(log_directory: str):
         """Configure logging to file."""
+        if not os.path.isdir(log_directory):
+            logger.warning("The provided log directory does not exist.")
+            return
+
         _handler = TimedRotatingFileHandler(
-            filename="xknx.log", when="midnight", backupCount=7, encoding="utf-8"
+            filename=f"{log_directory}{os.sep}xknx.log",
+            when="midnight",
+            backupCount=7,
+            encoding="utf-8",
         )
         _formatter = logging.Formatter(
             "%(asctime)s | %(name)s | %(levelname)s | %(message)s",