""" Tests for the check-silent-cleanup pre-commit hook. Ensures the hook correctly detects: 1. Silent exception handling in close() methods 2. Bare .close() calls in finally/except blocks """ import ast import sys from importlib import import_module from pathlib import Path # Add the pre-commit hooks directory to path HOOKS_DIR = Path(__file__).parent.parent.parent / ".pre-commit-hooks" sys.path.insert(0, str(HOOKS_DIR)) # Import the checker from the hook (must be after sys.path modification) hook_module = import_module("check-silent-cleanup") # noqa: E402 SilentCleanupChecker = hook_module.SilentCleanupChecker BareCloseInFinallyChecker = hook_module.BareCloseInFinallyChecker class TestSilentCleanupChecker: """Tests for the SilentCleanupChecker AST visitor.""" def _check_code(self, code: str, filename: str = "src/module.py") -> list: """Helper to check code and return errors.""" tree = ast.parse(code) checker = SilentCleanupChecker(filename) checker.visit(tree) return checker.errors # --- Detection: should flag --- def test_detects_except_exception_pass_in_close(self): """Should flag except Exception: pass inside close().""" code = """ class Foo: def close(self): try: self.client.close() except Exception: pass """ errors = self._check_code(code) assert len(errors) == 1 assert "Silent exception" in errors[0][1] def test_detects_bare_except_pass_in_close(self): """Should flag bare except: pass inside close().""" code = """ class Foo: def close(self): try: self.client.close() except: pass """ errors = self._check_code(code) assert len(errors) == 1 def test_detects_multiple_silent_blocks(self): """Should flag each silent block independently.""" code = """ class Foo: def close(self): try: self.session.close() except Exception: pass try: self.engine.close() except Exception: pass """ errors = self._check_code(code) assert len(errors) == 2 def test_reports_correct_line_number(self): """Should report the line of the except handler, not the try.""" code = """ class Foo: def close(self): x = 1 y = 2 try: self.client.close() except Exception: pass """ errors = self._check_code(code) assert len(errors) == 1 assert errors[0][0] == 8 # "except Exception:" is line 8 # --- Allowances: should NOT flag --- def test_allows_logger_warning_in_handler(self): """Should allow except blocks that log a warning.""" code = """ class Foo: def close(self): try: self.client.close() except Exception: logger.warning("Failed to close client") """ errors = self._check_code(code) assert len(errors) == 0 def test_allows_logger_exception_in_handler(self): """Should allow except blocks that use logger.exception.""" code = """ class Foo: def close(self): try: self.client.close() except Exception: logger.exception("Close failed") """ errors = self._check_code(code) assert len(errors) == 0 def test_allows_logger_debug_in_handler(self): """Should allow any logger.* call.""" code = """ class Foo: def close(self): try: self.client.close() except Exception: logger.debug("Ignoring close error") """ errors = self._check_code(code) assert len(errors) == 0 def test_allows_raise_in_handler(self): """Should allow except blocks that re-raise.""" code = """ class Foo: def close(self): try: self.client.close() except Exception: raise """ errors = self._check_code(code) assert len(errors) == 0 def test_allows_specific_exception_type(self): """Should not flag handlers for specific exception types (e.g. OSError).""" code = """ class Foo: def close(self): try: self.client.close() except OSError: pass """ errors = self._check_code(code) assert len(errors) == 0 def test_ignores_non_close_methods(self): """Should not inspect methods other than close().""" code = """ class Foo: def cleanup(self): try: self.client.close() except Exception: pass def shutdown(self): try: self.engine.stop() except Exception: pass """ errors = self._check_code(code) assert len(errors) == 0 def test_ignores_standalone_close_function(self): """A top-level function named close() should still be checked.""" code = """ def close(): try: something.close() except Exception: pass """ errors = self._check_code(code) assert len(errors) == 1 def test_close_with_no_try_except(self): """close() with no try/except should pass cleanly.""" code = """ class Foo: def close(self): self.client.close() """ errors = self._check_code(code) assert len(errors) == 0 def test_close_using_safe_close(self): """close() using safe_close() should pass.""" code = """ class Foo: def close(self): from utilities.resource_utils import safe_close safe_close(self.client, "client") """ errors = self._check_code(code) assert len(errors) == 0 def test_mixed_silent_and_logged(self): """Should flag only the silent block, not the logged one.""" code = """ class Foo: def close(self): try: self.session.close() except Exception: logger.warning("session close failed") try: self.engine.close() except Exception: pass """ errors = self._check_code(code) assert len(errors) == 1 # The flagged line should be the second except (line 10) assert errors[0][0] == 10 class TestFileExclusions: """Tests for file-level exclusion patterns via check_file().""" SILENT_CLOSE = """\ class Foo: def close(self): try: self.x.close() except Exception: pass """ def _write_and_check(self, tmp_path, subdir, filename): """Write SILENT_CLOSE to subdir/filename and run check_file.""" d = tmp_path / subdir d.mkdir(parents=True, exist_ok=True) f = d / filename f.write_text(self.SILENT_CLOSE) return hook_module.check_file(str(f)) def test_allows_test_directory(self, tmp_path): """Should skip files under tests/.""" assert self._write_and_check(tmp_path, "tests", "foo.py") is True def test_allows_test_prefix(self, tmp_path): """Should skip files starting with test_.""" assert ( self._write_and_check(tmp_path, "src", "test_something.py") is True ) def test_allows_test_suffix(self, tmp_path): """Should skip files ending with _test.py.""" assert ( self._write_and_check(tmp_path, "src", "something_test.py") is True ) def test_allows_llm_config(self, tmp_path): """Should skip config/llm_config.py (logging boundary).""" assert ( self._write_and_check(tmp_path, "config", "llm_config.py") is True ) def test_allows_rate_limiting_wrapper(self, tmp_path): """Should skip rate_limiting/llm/wrapper.py (logging boundary).""" assert ( self._write_and_check(tmp_path, "rate_limiting/llm", "wrapper.py") is True ) def test_flags_regular_source_file(self): """Should flag violations in normal source files.""" tree = ast.parse(self.SILENT_CLOSE) checker = SilentCleanupChecker("src/engine.py") checker.visit(tree) assert len(checker.errors) == 1 class TestCheckFileIntegration: """Integration tests for check_file().""" def _write_and_check(self, tmp_path, code, filename="module.py"): """Write code to a temp file and run check_file on it. Uses a subdirectory named 'src' to avoid matching exclusion patterns like 'test_' that can appear in pytest's tmp_path. We also monkeypatch check_file to use a clean path for pattern matching while still reading from the real file. """ src_dir = tmp_path / "src" src_dir.mkdir(exist_ok=True) filepath = src_dir / filename filepath.write_text(code) # check_file matches ALLOWED_PATTERNS against the full path. # pytest tmp dirs contain "pytest-" and "test_" which would match. # Use the checker directly to bypass path-based exclusions. import ast as _ast with open(filepath, "r", encoding="utf-8") as f: content = f.read() tree = _ast.parse(content, filename=str(filepath)) checker = SilentCleanupChecker("src/" + filename) checker.visit(tree) return len(checker.errors) == 0 def test_clean_file_passes(self, tmp_path): code = """ class Foo: def close(self): from utils import safe_close safe_close(self.client, "client") """ assert self._write_and_check(tmp_path, code) is True def test_violation_fails(self, tmp_path): code = """ class Foo: def close(self): try: self.client.close() except Exception: pass """ assert self._write_and_check(tmp_path, code) is False def test_non_python_file_passes(self, tmp_path): src_dir = tmp_path / "src" src_dir.mkdir() txt = src_dir / "readme.txt" txt.write_text("not python") assert hook_module.check_file(str(txt)) is True def test_syntax_error_passes(self, tmp_path): """Files with syntax errors are skipped (other tools handle them).""" src_dir = tmp_path / "src" src_dir.mkdir() bad = src_dir / "broken.py" bad.write_text("def close(self:\n pass") assert hook_module.check_file(str(bad)) is True def test_nonexistent_file_fails(self): assert hook_module.check_file("/nonexistent/path/module.py") is False def test_empty_file_passes(self, tmp_path): src_dir = tmp_path / "src" src_dir.mkdir() empty = src_dir / "empty.py" empty.write_text("") assert hook_module.check_file(str(empty)) is True class TestIsSilentHandler: """Unit tests for _is_silent_handler static method.""" def _make_handler(self, body_code): """Parse an except handler from code.""" code = f""" try: pass except Exception: {body_code} """ tree = ast.parse(code) for node in ast.walk(tree): if isinstance(node, ast.ExceptHandler): return node raise ValueError("No ExceptHandler found") def test_pass_only_is_silent(self): handler = self._make_handler(" pass") assert SilentCleanupChecker._is_silent_handler(handler) is True def test_multiple_pass_is_silent(self): handler = self._make_handler(" pass\n pass") assert SilentCleanupChecker._is_silent_handler(handler) is True def test_logger_call_is_not_silent(self): handler = self._make_handler(' logger.warning("fail")') assert SilentCleanupChecker._is_silent_handler(handler) is False def test_raise_is_not_silent(self): handler = self._make_handler(" raise") assert SilentCleanupChecker._is_silent_handler(handler) is False def test_assignment_is_not_silent(self): """An assignment (any non-pass statement) is not silent.""" handler = self._make_handler(" x = 1") assert SilentCleanupChecker._is_silent_handler(handler) is False def test_print_call_is_not_silent(self): """A print() call (non-logger, non-pass) is not silent.""" handler = self._make_handler(' print("error")') assert SilentCleanupChecker._is_silent_handler(handler) is False class TestBareCloseInFinallyChecker: """Tests for the BareCloseInFinallyChecker AST visitor.""" def _check_code(self, code: str, filename: str = "src/module.py") -> list: """Helper to check code and return errors.""" tree = ast.parse(code) checker = BareCloseInFinallyChecker(filename) checker.visit(tree) return checker.errors # --- Detection: should flag --- def test_detects_bare_close_in_finally(self): """Should flag obj.close() in a finally block.""" code = """ try: do_work() finally: resource.close() """ errors = self._check_code(code) assert len(errors) == 1 assert "Bare .close()" in errors[0][1] def test_detects_bare_close_in_except(self): """Should flag obj.close() in an except block.""" code = """ try: do_work() except Exception: conn.close() raise """ errors = self._check_code(code) assert len(errors) == 1 def test_detects_bare_close_in_if_inside_finally(self): """Should flag obj.close() inside if-guard in finally.""" code = """ try: do_work() finally: if resource is not None: resource.close() """ errors = self._check_code(code) assert len(errors) == 1 def test_detects_bare_close_in_else_inside_finally(self): """Should flag obj.close() in else branch of if in finally.""" code = """ try: do_work() finally: if skip: pass else: resource.close() """ errors = self._check_code(code) assert len(errors) == 1 def test_detects_multiple_bare_closes(self): """Should flag each bare .close() independently.""" code = """ try: do_work() finally: session.close() engine.close() """ errors = self._check_code(code) assert len(errors) == 2 def test_reports_correct_line_number(self): """Should report the line of the .close() call.""" code = """ try: do_work() finally: x = 1 resource.close() """ errors = self._check_code(code) assert len(errors) == 1 assert errors[0][0] == 6 # --- Allowances: should NOT flag --- def test_allows_close_in_nested_try_except(self): """Should not flag .close() that's protected by its own try/except.""" code = """ try: do_work() finally: try: resource.close() except Exception: logger.warning("close failed") """ errors = self._check_code(code) assert len(errors) == 0 def test_allows_safe_close(self): """Should not flag safe_close() calls.""" code = """ try: do_work() finally: safe_close(resource, "resource") """ errors = self._check_code(code) assert len(errors) == 0 def test_allows_close_outside_finally(self): """Should not flag .close() calls in normal code.""" code = """ resource = get_resource() resource.close() """ errors = self._check_code(code) assert len(errors) == 0 def test_allows_close_with_args(self): """Should not flag .close(arg) — only no-arg .close().""" code = """ try: do_work() finally: resource.close(force=True) """ errors = self._check_code(code) assert len(errors) == 0 def test_allows_non_close_method_in_finally(self): """Should not flag other method calls in finally.""" code = """ try: do_work() finally: resource.shutdown() """ errors = self._check_code(code) assert len(errors) == 0 def test_bare_except_handler_also_caught(self): """Should flag .close() in bare except: block.""" code = """ try: do_work() except: conn.close() """ errors = self._check_code(code) assert len(errors) == 1 def test_allows_plt_close(self): """Should not flag plt.close() — matplotlib figure cleanup, not a resource.""" code = """ try: make_chart() except Exception: plt.figure() plt.text(0.5, 0.5, "error") plt.savefig(path) plt.close() """ errors = self._check_code(code) assert len(errors) == 0 def test_allows_fig_close(self): """Should not flag fig.close() — matplotlib figure object.""" code = """ try: make_chart() finally: fig.close() """ errors = self._check_code(code) assert len(errors) == 0