From c3a05861d7dbecc12d12e24847f4fa91018d360b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Manuel=20Barroso=20Galindo?= Date: Mon, 28 Aug 2023 19:55:24 +0200 Subject: [PATCH] Simplifying FsCache, covering firmware bug scenario in UT. --- src/downloader/file_system.py | 41 ++++++++----------- src/test/fake_file_system_factory.py | 34 +++++++++++---- .../online_importer/test_online_importer.py | 6 +++ 3 files changed, 49 insertions(+), 32 deletions(-) diff --git a/src/downloader/file_system.py b/src/downloader/file_system.py index c67bd10..1a1621f 100644 --- a/src/downloader/file_system.py +++ b/src/downloader/file_system.py @@ -42,7 +42,7 @@ def __init__(self, config: Dict[str, Any], path_dictionary: Dict[str, str], logg self._logger = logger self._unique_temp_filenames: Set[Optional[str]] = set() self._unique_temp_filenames.add(None) - self._fs_cache = _FsCache() + self._fs_cache = FsCache() def create_for_system_scope(self) -> 'FileSystem': return self.create_for_config(self._config) @@ -159,7 +159,7 @@ class FolderCreationError(Exception): class _FileSystem(FileSystem): - def __init__(self, config: Dict[str, Any], path_dictionary: Dict[str, str], logger: Logger, unique_temp_filenames: Set[Optional[str]], fs_cache: '_FsCache'): + def __init__(self, config: Dict[str, Any], path_dictionary: Dict[str, str], logger: Logger, unique_temp_filenames: Set[Optional[str]], fs_cache: 'FsCache'): self._config = config self._path_dictionary = path_dictionary self._logger = logger @@ -179,15 +179,14 @@ def resolve(self, path: str) -> str: return str(Path(path).resolve()) def is_file(self, path: str) -> bool: - if self._fs_cache.contains_file(path): + full_path = self._path(path) + if self._fs_cache.contains_file(full_path): self._quick_hit += 1 return True - else: - full_path = self._path(path) - if os.path.isfile(full_path): - self._slow_hit += 1 - self._fs_cache.add_file(path, full_path) - return True + elif os.path.isfile(full_path): + self._slow_hit += 1 + self._fs_cache.add_file(full_path) + return True return False @@ -205,7 +204,7 @@ def precache_is_file_with_folders(self, folders: List[str]) -> None: continue files = [f.path for f in os.scandir(full_folder_path) if f.is_file()] - self._fs_cache.add_many_files(files, base_path) + self._fs_cache.add_many_files(files) def read_file_contents(self, path: str) -> str: with open(self._path(path), 'r') as f: @@ -225,14 +224,14 @@ def move(self, source: str, target: str) -> None: self._logger.debug(f'Moving "{source}" to "{target}". {full_source} -> {full_target}') self._logger.debug(self._path_dictionary) os.replace(full_source, full_target) - self._fs_cache.remove_file(source, full_source) - self._fs_cache.add_file(target, full_target) + self._fs_cache.remove_file(full_source) + self._fs_cache.add_file(full_target) def copy(self, source: str, target: str) -> None: full_source = self._path(source) full_target = self._path(target) shutil.copyfile(full_source, full_target) - self._fs_cache.add_file(target, full_target) + self._fs_cache.add_file(full_target) def copy_fast(self, source: str, target: str) -> None: full_source = self._path(source) @@ -240,7 +239,7 @@ def copy_fast(self, source: str, target: str) -> None: with open(full_source, 'rb') as fsource: with open(full_target, 'wb') as ftarget: shutil.copyfileobj(fsource, ftarget, length=1024 * 1024 * 4) - self._fs_cache.add_file(target, full_target) + self._fs_cache.add_file(full_target) def hash(self, path: str) -> str: return hash_file(self._path(path)) @@ -358,7 +357,7 @@ def _unlink(self, path: str, verbose: bool) -> bool: self._logger.print(f'Removing {path} ({full_path})') try: Path(full_path).unlink() - self._fs_cache.remove_file(path, full_path) + self._fs_cache.remove_file(full_path) return True except FileNotFoundError as _: return False @@ -417,19 +416,15 @@ def _load_json(file_path: str) -> Dict[str, Any]: return json.loads(f.read()) -class _FsCache: +class FsCache: def __init__(self): self._files: Set[str] = set() def contains_file(self, path: str) -> bool: return path in self._files - def add_many_files(self, paths: List[str], base_path: Optional[str]) -> None: + def add_many_files(self, paths: List[str]) -> None: self._files.update(paths) - if base_path is not None: - self._files.update(f.replace(base_path + '/', '') for f in paths) - def add_file(self, path: str, full_path: str) -> None: + def add_file(self, path: str) -> None: self._files.add(path) - if len(path) != len(full_path): self._files.add(full_path) - def remove_file(self, path: str, full_path: str) -> None: + def remove_file(self, path: str) -> None: if path in self._files: self._files.remove(path) - if len(path) != len(full_path) and full_path in self._files: self._files.remove(full_path) diff --git a/src/test/fake_file_system_factory.py b/src/test/fake_file_system_factory.py index 37e2656..ff411e3 100644 --- a/src/test/fake_file_system_factory.py +++ b/src/test/fake_file_system_factory.py @@ -20,7 +20,7 @@ from downloader.constants import K_BASE_PATH, STORAGE_PATHS_PRIORITY_SEQUENCE from downloader.file_system import FileSystemFactory as ProductionFileSystemFactory, FileSystem as ProductionFileSystem, \ - absolute_parent_folder, is_windows, FolderCreationError + absolute_parent_folder, is_windows, FolderCreationError, FsCache from downloader.other import ClosableValue, UnreachableException from test.fake_importer_implicit_inputs import FileSystemState from downloader.logger import NoLogger @@ -45,15 +45,16 @@ def __init__(self, state=None, write_records=None, config=None): self._state = state if state is not None else FileSystemState(config=config) self._fake_failures = {} self._write_records = write_records if write_records is not None else [] + self._fs_cache = FsCache() def set_create_folders_buggy(self): self._fake_failures['create_folders'] = True def create_for_config(self, config): - return FakeFileSystem(self._state, config, self._fake_failures, self._write_records) + return FakeFileSystem(self._state, config, self._fake_failures, self._write_records, self._fs_cache) def create_for_system_scope(self): - return FakeFileSystem(self._state, self._state.config, self._fake_failures, self._write_records) + return FakeFileSystem(self._state, self._state.config, self._fake_failures, self._write_records, self._fs_cache) @property def data(self): @@ -74,12 +75,13 @@ def from_state(files=None, folders=None, config=None, base_path=None, path_dicti class FakeFileSystem(ProductionFileSystem): unique_temp_filename_index = 0 - def __init__(self, state, config, fake_failures, write_records): + def __init__(self, state, config, fake_failures, write_records, fs_cache: FsCache): self.state = state self._config = config self._fake_failures = fake_failures self._write_records = write_records self._current_temp_file_index = 0 + self._fs_cache = fs_cache @property def write_records(self): @@ -108,7 +110,15 @@ def resolve(self, path): return self._path(path) def is_file(self, path): - return self._path(path) in self.state.files + full_path = self._path(path) + if self._fs_cache.contains_file(full_path): + return True + + if full_path in self.state.files: + self._fs_cache.add_file(full_path) + return True + + return False def print_debug(self): pass @@ -146,9 +156,13 @@ def set_copy_buggy(self): def move(self, source, target): source_file = self._path(source) target_file = self._path(target) + if source_file not in self.state.files: + raise Exception('Source file "%s" does not exist!' % source_file) self.state.files[target_file] = self.state.files[source_file] self.state.files.pop(source_file) self._write_records.append(_Record('move', (source_file, target_file))) + self._fs_cache.add_file(target_file) + self._fs_cache.remove_file(source_file) def copy(self, source, target): source_file = self._path(source) @@ -159,6 +173,7 @@ def copy(self, source, target): self.state.files[target_file] = source_description self._write_records.append(_Record('copy', (source_file, target_file))) + self._fs_cache.add_file(target_file) def copy_fast(self, source, target): self.copy(source, target) @@ -218,10 +233,11 @@ def download_target_path(self, path): return self._path(path) def unlink(self, path, verbose=True): - file = self._path(path) - if file in self.state.files: - self.state.files.pop(file) - self._write_records.append(_Record('unlink', file)) + full_path = self._path(path) + if full_path in self.state.files: + self.state.files.pop(full_path) + self._write_records.append(_Record('unlink', full_path)) + self._fs_cache.remove_file(full_path) return True else: return False diff --git a/src/test/unit/online_importer/test_online_importer.py b/src/test/unit/online_importer/test_online_importer.py index ad79803..c186173 100644 --- a/src/test/unit/online_importer/test_online_importer.py +++ b/src/test/unit/online_importer/test_online_importer.py @@ -193,6 +193,12 @@ def test_download_distribution_mister_with_mister___on_empty_store___needs_reboo ]), sut.fs_records) self.assertReports(sut, [FILE_MiSTer], needs_reboot=True) + def test_two_databases_one_with_mister___on_empty_store___needs_reboot_and_does_not_throw_due_to_bad_fs_cache_handling(self): + self.assertReports(OnlineImporter() + .add_db(db_distribution_mister(files={FILE_MiSTer: file_mister_descr()}), empty_test_store()) + .add_db(db_test_being_empty_descr(), empty_test_store()) + .download(False), [FILE_MiSTer], needs_reboot=True) + def test_download_distribution_mister_with_pdfviewer___on_empty_store_and_fs___needs_reboot(self): sut = OnlineImporter() store = empty_test_store()