Add tests for main hook implementation

.gitignore

@@ -158,3 +158,5 @@ cython_debug/
 #  and can be added to the global gitignore or merged into this file.  For a more nuclear
 #  option (not recommended) you can uncomment the following to ignore the entire idea folder.
 #.idea/
+
+.vscode

checkpatch_hook/__init__.py

@@ -14,7 +14,6 @@
 
 logging.basicConfig()
 logger = getLogger(__name__)
-logger.setLevel(logging.INFO)
 
 checkpatch_script_path = importlib.resources.files("checkpatch_hook.scripts").joinpath(
     "checkpatch.pl"
@@ -55,6 +54,7 @@ def _parse_args() -> dict:
 class ConfigFile:
     DEFAULT_DIR_KEY = "__default__"
     mandatory_keys = ["DIR_CONFIGS"]
+    # TODO: with YAML anchors there wouldn't really be a need for this approach
     magic_error_keys = {"errors_enabled": "ERRORS_COMMON", "errors_ignored": "IGNORES_COMMON"}
     optional_keys = ["IGNORED_FILES"]
     # TODO: Use schema based validation e.g., cfgv
@@ -114,6 +114,7 @@ def pre_commit_hook(config_file: Path, commit_files: list[Path], fix_inplace: bo
     config = config_file_obj.load_config()
     logger.debug(f"Config file @ {config_file.resolve()}")
 
+    result = True
     errors: DefaultDict[str, list] = defaultdict(list)
     # accumulate checkpatch errors for each configured dir
     for config_dir, dconfig in config["DIR_CONFIGS"].items():
@@ -126,24 +127,39 @@ def pre_commit_hook(config_file: Path, commit_files: list[Path], fix_inplace: bo
         patch_files = [
             p
             for p in commit_files
-            if p.is_relative_to(config_dir)
+            if check_config_applicable(config, config_dir, p)
             and p not in config.get("IGNORED_FILES", [])
             and not p.is_symlink()
         ]
         if patch_files:
-            _run_checkpatch(patch_files, fix_inplace, errors, config_dir, **post_dconfig)
+            result_dir = _run_checkpatch(
+                patch_files, fix_inplace, errors, config_dir, **post_dconfig
+            )
+            if not result_dir:
+                result = result_dir
         else:
             logger.debug(f"No files to check in {config_dir}")
 
-    # print errors
-    for filename, f_errors in errors.items():
-        logger.error(f"{filename}:")
-        for error in f_errors:
-            logger.error(f'  {error["line"]}: {error["message"]}')
-    if errors:
+    # log errors
+    for filename, file_errors in errors.items():
+        for error in file_errors:
+            logger.error("%s:%s: %s", filename, error["line"], error["message"])
+    if not result:
         sys.exit(1)
 
 
+def check_config_applicable(config: dict, config_dir: Path, path: Path) -> bool:
+    if str(config_dir) != ".":
+        return path.is_relative_to(config_dir)
+    for other_config_dir in config["DIR_CONFIGS"].keys():
+        if other_config_dir == ConfigFile.DEFAULT_DIR_KEY:
+            continue
+        other_config_path = Path(other_config_dir)
+        if path.is_relative_to(other_config_path):
+            return False
+    return True
+
+
 def _run_checkpatch(
     patch_files: list[Path],
     fix_inplace: bool,
@@ -152,7 +168,7 @@ def _run_checkpatch(
     errors_enabled: list,
     errors_ignored: list,
     max_line_length: int | str | None,
-) -> None:
+) -> bool:
     cmd = [
         str(checkpatch_script_path),
         "--strict",  # Be more annoying
@@ -183,6 +199,8 @@ def _run_checkpatch(
 
     if result.returncode != 0:
         _process_checkpatch_errors(result.stdout.decode(), errors)
+        return False
+    return True
 
 
 def _process_checkpatch_errors(output: str, errors: DefaultDict[str, list]) -> None:
@@ -203,9 +221,10 @@ def main() -> None:
     Entry point to be used when run as hook script.
     """
     args = _parse_args()
-    logger.debug(f"Running with args: {args}")
     if args["verbose"]:
         logger.setLevel(logging.DEBUG)
+    else:
+        logger.setLevel(logging.INFO)
     pre_commit_hook(args["config_file"], args["files"], args["fix_inplace"])
 
 

checkpatch_hook/data/checkpatch.yaml

@@ -33,13 +33,5 @@ DIR_CONFIGS:
   __default__:
     errors_ignored:
       - IGNORES_COMMON
-      - VOLATILE
-      - NEW_TYPEDEFS
-      - AVOID_EXTERNS
-      - MACRO_ARG_REUSE
-      - PREFER_KERNEL_TYPES
-      - PREFER_PACKED
-      - SSCANF_TO_KSTRTO
-    max_line_length: 120
 
 IGNORED_FILES: []

pyproject.toml

@@ -16,6 +16,7 @@ dev-dependencies = [
     "pytest>=8.3.2",
     "coverage>=7.6.1",
     "pytest-cov>=5.0.0",
+    "pytest-subprocess>=1.5.2",
 ]
 
 [tool.pytest.ini_options]

tests/test_hook.py

@@ -0,0 +1,150 @@
+import logging
+from pathlib import Path
+from textwrap import dedent
+
+from pytest import LogCaptureFixture, MonkeyPatch, raises
+from pytest_subprocess import FakeProcess
+
+from checkpatch_hook import logger, main
+
+
+def _register_pass(fp: FakeProcess) -> None:
+    fp.register([fp.any()], returncode=0)
+
+
+def _register_fail(fp: FakeProcess, stdout: str | None = None) -> None:
+    fp.register([fp.any()], returncode=1, stdout=stdout)
+
+
+def _invoke_hook(monkeypatch: MonkeyPatch, args: list[str]) -> None:
+    monkeypatch.setattr("sys.argv", ["checkpatch"] + args)
+    main()
+
+
+def test_default_config_is_applied_pass(fp: FakeProcess, monkeypatch: MonkeyPatch) -> None:
+    args = ["--verbose", "test1.c"]
+    common_ignores = [
+        "BAD_SIGN_OFF",
+        "SPDX_LICENSE_TAG",
+        "FILE_PATH_CHANGES",
+        "NOT_UNIFIED_DIFF",
+        "LINUX_VERSION_CODE",
+        "CONSTANT_COMPARISON",
+        "OPEN_ENDED_LINE",
+        "UNNECESSARY_PARENTHESES",
+        "GERRIT_CHANGE_ID",
+        "COMMIT_LOG_LONG_LINE",
+        "EMAIL_SUBJECT",
+        "GIT_COMMIT_ID",
+    ]
+
+    _register_pass(fp)
+
+    _invoke_hook(monkeypatch, args)
+
+    invocation = " ".join(fp.calls[0])
+
+    for ignore in common_ignores:
+        assert ignore in invocation
+
+    assert "--types" not in invocation
+    assert "--fix-inplace" not in invocation
+    assert "--max-line-length" not in invocation
+    assert "--file test1.c" in invocation
+
+
+def test_default_config_fix_inplace_fail(fp: FakeProcess, monkeypatch: MonkeyPatch) -> None:
+    args = ["--fix-inplace", "test1.c"]
+
+    _register_fail(fp)
+
+    with raises(SystemExit):
+        _invoke_hook(monkeypatch, args)
+
+    invocation = " ".join(fp.calls[0])
+
+    assert "--fix-inplace" in invocation
+    assert "--file test1.c" in invocation
+
+
+def test_default_config_no_files_pass(fp: FakeProcess, monkeypatch: MonkeyPatch) -> None:
+    _register_pass(fp)
+
+    _invoke_hook(monkeypatch, [])
+
+    assert len(fp.calls) == 0
+
+
+def test_specified_config_is_applied_pass(
+    fp: FakeProcess, monkeypatch: MonkeyPatch, tmp_path: Path
+) -> None:
+    config_path = tmp_path / "test_config.yaml"
+    config_text = dedent(
+        """\
+    DIR_CONFIGS:
+        __default__:
+            errors_ignored:
+            - UNNECESSARY_PARENTHESES
+            max_line_length: '120'
+        dir2:
+            errors_enabled:
+            - GERRIT_CHANGE_ID
+            max_line_length: '100'
+    """
+    )
+    config_path.write_text(config_text)
+    args = ["--config-file", str(config_path), "test1.c", "dir2/test2.h"]
+
+    _register_pass(fp)
+    _register_pass(fp)
+
+    _invoke_hook(monkeypatch, args)
+
+    assert len(fp.calls) == 2
+
+    invocation_default = " ".join(fp.calls[0])
+    invocation_dir2 = " ".join(fp.calls[1])
+
+    assert "--ignore UNNECESSARY_PARENTHESES" in invocation_default
+    assert "--types" not in invocation_default
+    assert "--fix-inplace" not in invocation_default
+    assert "--max-line-length=120" in invocation_default
+    assert "--file test1.c" in invocation_default
+    assert "test2.h" not in invocation_default
+
+    assert "--ignore" not in invocation_dir2
+    assert "--types GERRIT_CHANGE_ID" in invocation_dir2
+    assert "--fix-inplace" not in invocation_dir2
+    assert "--max-line-length=100" in invocation_dir2
+    assert "--file dir2/test2.h" in invocation_dir2
+    assert "test1.c" not in invocation_dir2
+
+
+def test_errors_returned(
+    fp: FakeProcess, monkeypatch: MonkeyPatch, tmp_path: Path, caplog: LogCaptureFixture
+) -> None:
+    config_path = tmp_path / "test_config.yaml"
+    config_text = dedent(
+        """\
+    DIR_CONFIGS:
+        __default__:
+            errors_ignored:
+            - UNNECESSARY_PARENTHESES
+            max_line_length: '120'
+        dir2:
+            errors_enabled:
+            - GERRIT_CHANGE_ID
+            max_line_length: '100'
+    """
+    )
+    config_path.write_text(config_text)
+    args = ["--config-file", str(config_path), "test1.c", "dir2/test2.h"]
+
+    _register_fail(fp, "test1.c:101: checkpatch: WARNING: struct  should normally be const")
+    _register_pass(fp)
+
+    with caplog.at_level(logging.ERROR, logger.name):
+        with raises(SystemExit):
+            _invoke_hook(monkeypatch, args)
+
+        assert "WARNING: struct  should normally be const" in caplog.records[0].message