loggerd: add types to tests and use pathlib (#28943)

system/loggerd/config.py

@@ -5,7 +5,7 @@ from system.hardware import PC
 if os.environ.get('LOG_ROOT', False):
   ROOT = os.environ['LOG_ROOT']
 elif PC:
-  ROOT = os.path.join(str(Path.home()), ".comma", "media", "0", "realdata")
+  ROOT = str(Path.home() / ".comma" / "media" / "0" / "realdata")
 else:
   ROOT = '/data/media/0/realdata/'
 
@@ -16,7 +16,7 @@ SEGMENT_LENGTH = 60
 STATS_DIR_FILE_LIMIT = 10000
 STATS_SOCKET = "ipc:///tmp/stats"
 if PC:
-  STATS_DIR = os.path.join(str(Path.home()), ".comma", "stats")
+  STATS_DIR = str(Path.home() / ".comma" / "stats")
 else:
   STATS_DIR = "/data/stats/"
 STATS_FLUSH_TIME_S = 60

system/loggerd/tests/fill.py

@@ -1,7 +1,7 @@
 #!/usr/bin/env python3
-"""Script to fill up EON with fake data"""
+"""Script to fill up storage with fake data"""
 
-import os
+from pathlib import Path
 
 from system.loggerd.config import ROOT, get_available_percent
 from system.loggerd.tests.loggerd_tests_common import create_random_file
@@ -10,13 +10,13 @@ from system.loggerd.tests.loggerd_tests_common import create_random_file
 if __name__ == "__main__":
   segment_idx = 0
   while True:
-    seg_name = "1970-01-01--00-00-00--%d" % segment_idx
-    seg_path = os.path.join(ROOT, seg_name)
+    seg_name = f"1970-01-01--00-00-00--{segment_idx}"
+    seg_path = Path(ROOT) / seg_name
 
     print(seg_path)
 
-    create_random_file(os.path.join(seg_path, 'fcamera.hevc'), 36)
-    create_random_file(os.path.join(seg_path, 'rlog.bz2'), 2)
+    create_random_file(seg_path / "fcamera.hevc", 36)
+    create_random_file(seg_path / "rlog.bz2", 2)
 
     segment_idx += 1
 

system/loggerd/tests/loggerd_tests_common.py

@@ -4,29 +4,29 @@ import shutil
 import random
 import tempfile
 import unittest
+from pathlib import Path
+from typing import Optional
 
 import system.loggerd.uploader as uploader
 
-def create_random_file(file_path, size_mb, lock=False, xattr=None):
-  try:
-    os.mkdir(os.path.dirname(file_path))
-  except OSError:
-    pass
 
-  lock_path = file_path + ".lock"
+def create_random_file(file_path: Path, size_mb: float, lock: bool = False, xattr: Optional[bytes] = None) -> None:
+  file_path.parent.mkdir(parents=True, exist_ok=True)
+
   if lock:
+    lock_path = str(file_path) + ".lock"
     os.close(os.open(lock_path, os.O_CREAT | os.O_EXCL))
 
   chunks = 128
   chunk_bytes = int(size_mb * 1024 * 1024 / chunks)
   data = os.urandom(chunk_bytes)
 
-  with open(file_path, 'wb') as f:
+  with open(file_path, "wb") as f:
     for _ in range(chunks):
       f.write(data)
 
   if xattr is not None:
-    uploader.setxattr(file_path, uploader.UPLOAD_ATTR_NAME, xattr)
+    uploader.setxattr(str(file_path), uploader.UPLOAD_ATTR_NAME, xattr)
 
 class MockResponse():
   def __init__(self, text, status_code):
@@ -75,12 +75,18 @@ class MockParams():
 class UploaderTestCase(unittest.TestCase):
   f_type = "UNKNOWN"
 
+  root: Path
+  seg_num: int
+  seg_format: str
+  seg_format2: str
+  seg_dir: str
+
   def set_ignore(self):
     uploader.Api = MockApiIgnore
 
   def setUp(self):
-    self.root = tempfile.mkdtemp()
-    uploader.ROOT = self.root  # Monkey patch root dir
+    self.root = Path(tempfile.mkdtemp())
+    uploader.ROOT = str(self.root)  # Monkey patch root dir
     uploader.Api = MockApi
     uploader.Params = MockParams
     uploader.fake_upload = True
@@ -98,8 +104,9 @@ class UploaderTestCase(unittest.TestCase):
       if e.errno != errno.ENOENT:
         raise
 
-  def make_file_with_data(self, f_dir, fn, size_mb=.1, lock=False, xattr=None):
-    file_path = os.path.join(self.root, f_dir, fn)
+  def make_file_with_data(self, f_dir: str, fn: str, size_mb: float = .1, lock: bool = False,
+                          xattr: Optional[bytes] = None) -> Path:
+    file_path = self.root / f_dir / fn
     create_random_file(file_path, size_mb, lock, xattr)
 
     return file_path

system/loggerd/tests/test_deleter.py

@@ -1,5 +1,4 @@
 #!/usr/bin/env python3
-import os
 import time
 import threading
 import unittest
@@ -21,7 +20,7 @@ class TestDeleter(UploaderTestCase):
     super().setUp()
     self.fake_stats = Stats(f_bavail=0, f_blocks=10, f_frsize=4096)
     deleter.os.statvfs = self.fake_statvfs
-    deleter.ROOT = self.root
+    deleter.ROOT = str(self.root)
 
   def start_thread(self):
     self.end_event = threading.Event()
@@ -39,11 +38,11 @@ class TestDeleter(UploaderTestCase):
     self.start_thread()
 
     with Timeout(5, "Timeout waiting for file to be deleted"):
-      while os.path.exists(f_path):
+      while f_path.exists():
         time.sleep(0.01)
     self.join_thread()
 
-    self.assertFalse(os.path.exists(f_path), "File not deleted")
+    self.assertFalse(f_path.exists(), "File not deleted")
 
   def test_delete_files_in_create_order(self):
     f_path_1 = self.make_file_with_data(self.seg_dir, self.f_type)
@@ -55,14 +54,13 @@ class TestDeleter(UploaderTestCase):
     self.start_thread()
 
     with Timeout(5, "Timeout waiting for file to be deleted"):
-      while os.path.exists(f_path_1) and os.path.exists(f_path_2):
+      while f_path_1.exists() and f_path_2.exists():
         time.sleep(0.01)
 
     self.join_thread()
 
-    self.assertFalse(os.path.exists(f_path_1), "Older file not deleted")
-
-    self.assertTrue(os.path.exists(f_path_2), "Newer file deleted before older file")
+    self.assertFalse(f_path_1.exists(), "Older file not deleted")
+    self.assertTrue(f_path_2.exists(), "Newer file deleted before older file")
 
   def test_no_delete_when_available_space(self):
     f_path = self.make_file_with_data(self.seg_dir, self.f_type)
@@ -75,14 +73,14 @@ class TestDeleter(UploaderTestCase):
 
     try:
       with Timeout(2, "Timeout waiting for file to be deleted"):
-        while os.path.exists(f_path):
+        while f_path.exists():
           time.sleep(0.01)
     except TimeoutException:
       pass
     finally:
       self.join_thread()
 
-    self.assertTrue(os.path.exists(f_path), "File deleted with available space")
+    self.assertTrue(f_path.exists(), "File deleted with available space")
 
   def test_no_delete_with_lock_file(self):
     f_path = self.make_file_with_data(self.seg_dir, self.f_type, lock=True)
@@ -91,14 +89,14 @@ class TestDeleter(UploaderTestCase):
 
     try:
       with Timeout(2, "Timeout waiting for file to be deleted"):
-        while os.path.exists(f_path):
+        while f_path.exists():
           time.sleep(0.01)
     except TimeoutException:
       pass
     finally:
       self.join_thread()
 
-    self.assertTrue(os.path.exists(f_path), "File deleted when locked")
+    self.assertTrue(f_path.exists(), "File deleted when locked")
 
 
 if __name__ == "__main__":

system/loggerd/tests/test_uploader.py

@@ -5,6 +5,8 @@ import threading
 import unittest
 import logging
 import json
+from pathlib import Path
+from typing import List, Optional
 
 from system.swaglog import cloudlog
 from system.loggerd.uploader import uploader_fn, UPLOAD_ATTR_NAME, UPLOAD_ATTR_VALUE
@@ -50,8 +52,8 @@ class TestUploader(UploaderTestCase):
     self.end_event.set()
     self.up_thread.join()
 
-  def gen_files(self, lock=False, xattr=None, boot=True):
-    f_paths = list()
+  def gen_files(self, lock=False, xattr: Optional[bytes] = None, boot=True) -> List[Path]:
+    f_paths = []
     for t in ["qlog", "rlog", "dcamera.hevc", "fcamera.hevc"]:
       f_paths.append(self.make_file_with_data(self.seg_dir, t, 1, lock=lock, xattr=xattr))
 
@@ -59,7 +61,7 @@ class TestUploader(UploaderTestCase):
       f_paths.append(self.make_file_with_data("boot", f"{self.seg_dir}", 1, lock=lock, xattr=xattr))
     return f_paths
 
-  def gen_order(self, seg1, seg2, boot=True):
+  def gen_order(self, seg1: List[int], seg2: List[int], boot=True) -> List[str]:
     keys = []
     if boot:
       keys += [f"boot/{self.seg_format.format(i)}.bz2" for i in seg1]
@@ -82,7 +84,7 @@ class TestUploader(UploaderTestCase):
     self.assertFalse(len(log_handler.upload_order) < len(exp_order), "Some files failed to upload")
     self.assertFalse(len(log_handler.upload_order) > len(exp_order), "Some files were uploaded twice")
     for f_path in exp_order:
-      self.assertEqual(os.getxattr(os.path.join(self.root, f_path.replace('.bz2', '')), UPLOAD_ATTR_NAME), UPLOAD_ATTR_VALUE, "All files not uploaded")
+      self.assertEqual(os.getxattr((self.root / f_path).with_suffix(""), UPLOAD_ATTR_NAME), UPLOAD_ATTR_VALUE, "All files not uploaded")
 
     self.assertTrue(log_handler.upload_order == exp_order, "Files uploaded in wrong order")
 
@@ -100,7 +102,7 @@ class TestUploader(UploaderTestCase):
     self.assertFalse(len(log_handler.upload_order) < len(exp_order), "Some files failed to upload")
     self.assertFalse(len(log_handler.upload_order) > len(exp_order), "Some files were uploaded twice")
     for f_path in exp_order:
-      self.assertEqual(os.getxattr(os.path.join(self.root, f_path.replace('.bz2', '')), UPLOAD_ATTR_NAME), UPLOAD_ATTR_VALUE, "All files not uploaded")
+      self.assertEqual(os.getxattr((self.root / f_path).with_suffix(""), UPLOAD_ATTR_NAME), UPLOAD_ATTR_VALUE, "All files not uploaded")
 
     self.assertTrue(log_handler.upload_order == exp_order, "Files uploaded in wrong order")
 
@@ -119,7 +121,7 @@ class TestUploader(UploaderTestCase):
     self.assertFalse(len(log_handler.upload_ignored) < len(exp_order), "Some files failed to ignore")
     self.assertFalse(len(log_handler.upload_ignored) > len(exp_order), "Some files were ignored twice")
     for f_path in exp_order:
-      self.assertEqual(os.getxattr(os.path.join(self.root, f_path.replace('.bz2', '')), UPLOAD_ATTR_NAME), UPLOAD_ATTR_VALUE, "All files not ignored")
+      self.assertEqual(os.getxattr((self.root / f_path).with_suffix(""), UPLOAD_ATTR_NAME), UPLOAD_ATTR_VALUE, "All files not ignored")
 
     self.assertTrue(log_handler.upload_ignored == exp_order, "Files ignored in wrong order")
 
@@ -144,7 +146,7 @@ class TestUploader(UploaderTestCase):
     self.assertFalse(len(log_handler.upload_order) < len(exp_order), "Some files failed to upload")
     self.assertFalse(len(log_handler.upload_order) > len(exp_order), "Some files were uploaded twice")
     for f_path in exp_order:
-      self.assertEqual(os.getxattr(os.path.join(self.root, f_path.replace('.bz2', '')), UPLOAD_ATTR_NAME), UPLOAD_ATTR_VALUE, "All files not uploaded")
+      self.assertEqual(os.getxattr((self.root / f_path).with_suffix(""), UPLOAD_ATTR_NAME), UPLOAD_ATTR_VALUE, "All files not uploaded")
 
     self.assertTrue(log_handler.upload_order == exp_order, "Files uploaded in wrong order")
 
@@ -159,7 +161,7 @@ class TestUploader(UploaderTestCase):
     self.join_thread()
 
     for f_path in f_paths:
-      fn = f_path.replace('.bz2', '')
+      fn = f_path.with_suffix(f_path.suffix.replace(".bz2", ""))
       uploaded = UPLOAD_ATTR_NAME in os.listxattr(fn) and os.getxattr(fn, UPLOAD_ATTR_NAME) == UPLOAD_ATTR_VALUE
       self.assertFalse(uploaded, "File upload when locked")
 
@@ -180,7 +182,8 @@ class TestUploader(UploaderTestCase):
     self.join_thread()
 
     for f_path in f_paths:
-      self.assertFalse(os.path.isfile(f_path + ".lock"), "File lock not cleared on startup")
+      lock_path = f_path.with_suffix(f_path.suffix + ".lock")
+      self.assertFalse(lock_path.is_file(), "File lock not cleared on startup")
 
 
 if __name__ == "__main__":

system/loggerd/xattr_cache.py

@@ -1,6 +1,6 @@
 import os
 import errno
-from typing import Dict, Tuple, Optional
+from typing import Dict, Optional, Tuple
 
 _cached_attributes: Dict[Tuple, Optional[bytes]] = {}