From 7cb6cf2f2a46436117c370ce303dda2055fc6e38 Mon Sep 17 00:00:00 2001 From: Jonathan Swoboda <154711427+swoboda1337@users.noreply.github.com> Date: Wed, 17 Jun 2026 21:12:39 -0400 Subject: [PATCH] [ci] Replace clang-tidy hash with direct config-file diff check (#17019) --- .clang-tidy.hash | 1 - .github/workflows/ci-clang-tidy-hash.yml | 76 ----- .github/workflows/ci.yml | 38 +-- .pre-commit-config.yaml | 9 +- script/ci-custom.py | 2 +- script/clang_tidy_hash.py | 208 +++----------- script/determine-jobs.py | 65 ++--- tests/script/test_clang_tidy_hash.py | 351 +++-------------------- tests/script/test_determine_jobs.py | 48 ++-- 9 files changed, 124 insertions(+), 674 deletions(-) delete mode 100644 .clang-tidy.hash delete mode 100644 .github/workflows/ci-clang-tidy-hash.yml mode change 100755 => 100644 script/clang_tidy_hash.py diff --git a/.clang-tidy.hash b/.clang-tidy.hash deleted file mode 100644 index 591ce70a62..0000000000 --- a/.clang-tidy.hash +++ /dev/null @@ -1 +0,0 @@ -6765760d573967b853b1f790f0f5478135d12f2b15ffa8bee9b0314090b582ee diff --git a/.github/workflows/ci-clang-tidy-hash.yml b/.github/workflows/ci-clang-tidy-hash.yml deleted file mode 100644 index 73c437467b..0000000000 --- a/.github/workflows/ci-clang-tidy-hash.yml +++ /dev/null @@ -1,76 +0,0 @@ -name: Clang-tidy Hash CI - -on: - pull_request: - paths: - - ".clang-tidy" - - "platformio.ini" - - "requirements_dev.txt" - - "sdkconfig.defaults" - - ".clang-tidy.hash" - - "script/clang_tidy_hash.py" - - ".github/workflows/ci-clang-tidy-hash.yml" - -permissions: - contents: read # actions/checkout for the PR head - pull-requests: write # pulls.createReview / listReviews / dismissReview when the clang-tidy hash is out of date - -jobs: - verify-hash: - name: Verify clang-tidy hash - runs-on: ubuntu-latest - steps: - - name: Checkout - uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3 - - - name: Set up Python - uses: actions/setup-python@a309ff8b426b58ec0e2a45f0f869d46889d02405 # v6.2.0 - with: - python-version: "3.11" - - - name: Verify hash - run: | - python script/clang_tidy_hash.py --verify - - - if: failure() - name: Show hash details - run: | - python script/clang_tidy_hash.py - echo "## Job Failed" | tee -a $GITHUB_STEP_SUMMARY - echo "You have modified clang-tidy configuration but have not updated the hash." | tee -a $GITHUB_STEP_SUMMARY - echo "Please run 'script/clang_tidy_hash.py --update' and commit the changes." | tee -a $GITHUB_STEP_SUMMARY - - - if: failure() && github.event.pull_request.head.repo.full_name == github.repository - name: Request changes - uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 - with: - script: | - await github.rest.pulls.createReview({ - pull_number: context.issue.number, - owner: context.repo.owner, - repo: context.repo.repo, - event: 'REQUEST_CHANGES', - body: 'You have modified clang-tidy configuration but have not updated the hash.\nPlease run `script/clang_tidy_hash.py --update` and commit the changes.' - }) - - - if: success() && github.event.pull_request.head.repo.full_name == github.repository - name: Dismiss review - uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 - with: - script: | - let reviews = await github.rest.pulls.listReviews({ - pull_number: context.issue.number, - owner: context.repo.owner, - repo: context.repo.repo - }); - for (let review of reviews.data) { - if (review.user.login === 'github-actions[bot]' && review.state === 'CHANGES_REQUESTED') { - await github.rest.pulls.dismissReview({ - pull_number: context.issue.number, - owner: context.repo.owner, - repo: context.repo.repo, - review_id: review.id, - message: 'Clang-tidy hash now matches configuration.' - }); - } - } diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index deeec72095..1b1032bcde 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -537,15 +537,12 @@ jobs: id: check_full_scan run: | . venv/bin/activate - # determine-jobs.clang-tidy-full-scan is true when core C++ changed - # OR the ci-run-all label forced --force-all. Independent of the - # hash check, both must produce a full scan in the job itself. + # determine-jobs.clang-tidy-full-scan is true when core C++ or a + # clang-tidy-relevant config file changed, or the ci-run-all label + # forced --force-all. if [ "${{ needs.determine-jobs.outputs.clang-tidy-full-scan }}" = "true" ]; then echo "full_scan=true" >> $GITHUB_OUTPUT echo "reason=determine_jobs" >> $GITHUB_OUTPUT - elif python script/clang_tidy_hash.py --check; then - echo "full_scan=true" >> $GITHUB_OUTPUT - echo "reason=hash_changed" >> $GITHUB_OUTPUT else echo "full_scan=false" >> $GITHUB_OUTPUT echo "reason=normal" >> $GITHUB_OUTPUT @@ -607,15 +604,12 @@ jobs: id: check_full_scan run: | . venv/bin/activate - # determine-jobs.clang-tidy-full-scan is true when core C++ changed - # OR the ci-run-all label forced --force-all. Independent of the - # hash check, both must produce a full scan in the job itself. + # determine-jobs.clang-tidy-full-scan is true when core C++ or a + # clang-tidy-relevant config file changed, or the ci-run-all label + # forced --force-all. if [ "${{ needs.determine-jobs.outputs.clang-tidy-full-scan }}" = "true" ]; then echo "full_scan=true" >> $GITHUB_OUTPUT echo "reason=determine_jobs" >> $GITHUB_OUTPUT - elif python script/clang_tidy_hash.py --check; then - echo "full_scan=true" >> $GITHUB_OUTPUT - echo "reason=hash_changed" >> $GITHUB_OUTPUT else echo "full_scan=false" >> $GITHUB_OUTPUT echo "reason=normal" >> $GITHUB_OUTPUT @@ -691,15 +685,12 @@ jobs: id: check_full_scan run: | . venv/bin/activate - # determine-jobs.clang-tidy-full-scan is true when core C++ changed - # OR the ci-run-all label forced --force-all. Independent of the - # hash check, both must produce a full scan in the job itself. + # determine-jobs.clang-tidy-full-scan is true when core C++ or a + # clang-tidy-relevant config file changed, or the ci-run-all label + # forced --force-all. if [ "${{ needs.determine-jobs.outputs.clang-tidy-full-scan }}" = "true" ]; then echo "full_scan=true" >> $GITHUB_OUTPUT echo "reason=determine_jobs" >> $GITHUB_OUTPUT - elif python script/clang_tidy_hash.py --check; then - echo "full_scan=true" >> $GITHUB_OUTPUT - echo "reason=hash_changed" >> $GITHUB_OUTPUT else echo "full_scan=false" >> $GITHUB_OUTPUT echo "reason=normal" >> $GITHUB_OUTPUT @@ -779,15 +770,12 @@ jobs: id: check_full_scan run: | . venv/bin/activate - # determine-jobs.clang-tidy-full-scan is true when core C++ changed - # OR the ci-run-all label forced --force-all. Independent of the - # hash check, both must produce a full scan in the job itself. + # determine-jobs.clang-tidy-full-scan is true when core C++ or a + # clang-tidy-relevant config file changed, or the ci-run-all label + # forced --force-all. if [ "${{ needs.determine-jobs.outputs.clang-tidy-full-scan }}" = "true" ]; then echo "full_scan=true" >> $GITHUB_OUTPUT echo "reason=determine_jobs" >> $GITHUB_OUTPUT - elif python script/clang_tidy_hash.py --check; then - echo "full_scan=true" >> $GITHUB_OUTPUT - echo "reason=hash_changed" >> $GITHUB_OUTPUT else echo "full_scan=false" >> $GITHUB_OUTPUT echo "reason=normal" >> $GITHUB_OUTPUT @@ -1049,7 +1037,7 @@ jobs: cache-key: ${{ needs.common.outputs.cache-key }} - uses: esphome/pre-commit-action@43cd1109c09c544d97196f7730ee5b2e0cc6d81e # v3.0.1 fork with pinned actions/cache env: - SKIP: pylint,clang-tidy-hash,ci-custom + SKIP: pylint,ci-custom - uses: pre-commit-ci/lite-action@5d6cc0eb514c891a40562a58a8e71576c5c7fb43 # v1.1.0 if: always() diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 3b6278e6b5..ba74aff07c 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -6,7 +6,7 @@ ci: autoupdate_commit_msg: 'pre-commit: autoupdate' autoupdate_schedule: off # Disabled until ruff versions are synced between deps and pre-commit # Skip hooks that have issues in pre-commit CI environment - skip: [pylint, clang-tidy-hash] + skip: [pylint] repos: - repo: https://github.com/astral-sh/ruff-pre-commit @@ -59,13 +59,6 @@ repos: language: system types: [python] files: ^esphome/.+\.py$ - - id: clang-tidy-hash - name: Update clang-tidy hash - entry: python script/clang_tidy_hash.py --update-if-changed - language: python - files: ^(\.clang-tidy|platformio\.ini|requirements_dev\.txt|sdkconfig\.defaults|esphome/idf_component\.yml)$ - pass_filenames: false - additional_dependencies: [] - id: ci-custom name: ci-custom entry: python script/run-in-env.py script/ci-custom.py diff --git a/script/ci-custom.py b/script/ci-custom.py index 78ff6cf781..cbc54ce55d 100755 --- a/script/ci-custom.py +++ b/script/ci-custom.py @@ -276,7 +276,7 @@ def lint_newline(fname, line, col, content): return "File contains Windows newline. Please set your editor to Unix newline mode." -@lint_content_check(exclude=["*.svg", ".clang-tidy.hash"]) +@lint_content_check(exclude=["*.svg"]) def lint_end_newline(fname, content): if content and not content.endswith("\n"): return "File does not end with a newline, please add an empty line at the end of the file." diff --git a/script/clang_tidy_hash.py b/script/clang_tidy_hash.py old mode 100755 new mode 100644 index 62f76246b4..00bcaf45b0 --- a/script/clang_tidy_hash.py +++ b/script/clang_tidy_hash.py @@ -1,66 +1,32 @@ -#!/usr/bin/env python3 -"""Calculate and manage hash for clang-tidy configuration.""" +"""Files that affect clang-tidy results, and a content hash over them. + +``CLANG_TIDY_GLOBAL_FILES`` (plus ``SDKCONFIG_DEFAULTS_PREFIX``) is the single +source of truth for which files influence clang-tidy output. A change to any of +them can surface warnings in source files a PR didn't touch, so: + +* ``script/determine-jobs.py`` runs a full clang-tidy scan when one changes, and +* ``calculate_clang_tidy_hash()`` folds them into the idedata cache key used by + ``script/helpers.py`` (a content hash, unlike an mtime check, stays correct + across git checkouts). +""" from __future__ import annotations -import argparse import hashlib from pathlib import Path -import re -import sys -# Add the script directory to path to import helpers -script_dir = Path(__file__).parent -sys.path.insert(0, str(script_dir)) +# Root-relative paths whose contents affect clang-tidy results. +CLANG_TIDY_GLOBAL_FILES = ( + ".clang-tidy", + "platformio.ini", + "requirements_dev.txt", + "esphome/idf_component.yml", +) - -def read_file_lines(path: Path) -> list[str]: - """Read lines from a file.""" - with path.open() as f: - return f.readlines() - - -def parse_requirement_line(line: str) -> tuple[str, str] | None: - """Parse a requirement line and return (package, original_line) or None. - - Handles formats like: - - package==1.2.3 - - package==1.2.3 # comment - - package>=1.2.3,<2.0.0 - """ - original_line = line.strip() - - # Extract the part before any comment for parsing - parse_line = line - if "#" in parse_line: - parse_line = parse_line[: parse_line.index("#")] - - parse_line = parse_line.strip() - if not parse_line: - return None - - # Use regex to extract package name - # This matches package names followed by version operators - match = re.match(r"^([a-zA-Z0-9_-]+)(==|>=|<=|>|<|!=|~=)(.+)$", parse_line) - if match: - return (match.group(1), original_line) # Return package name and original line - - return None - - -def get_clang_tidy_version_from_requirements(repo_root: Path | None = None) -> str: - """Get clang-tidy version from requirements_dev.txt""" - repo_root = _ensure_repo_root(repo_root) - requirements_path = repo_root / "requirements_dev.txt" - lines = read_file_lines(requirements_path) - - for line in lines: - parsed = parse_requirement_line(line) - if parsed and parsed[0] == "clang-tidy": - # Return the original line (preserves comments) - return parsed[1] - - return "clang-tidy version not found" +# sdkconfig.defaults and per-target sdkconfig.defaults. files flip the +# CONFIG flags that decide which variant code paths clang-tidy sees. Matched by +# this prefix at the repo root. +SDKCONFIG_DEFAULTS_PREFIX = "sdkconfig.defaults" def read_file_bytes(path: Path) -> bytes: @@ -80,130 +46,20 @@ def _ensure_repo_root(repo_root: Path | None) -> Path: def calculate_clang_tidy_hash(repo_root: Path | None = None) -> str: - """Calculate hash of clang-tidy configuration and version""" + """Calculate a hash of the files that affect clang-tidy results.""" repo_root = _ensure_repo_root(repo_root) hasher = hashlib.sha256() - # Hash .clang-tidy file - clang_tidy_path = repo_root / ".clang-tidy" - content = read_file_bytes(clang_tidy_path) - hasher.update(content) + for name in CLANG_TIDY_GLOBAL_FILES: + path = repo_root / name + if path.exists(): + hasher.update(read_file_bytes(path)) - # Hash clang-tidy version from requirements_dev.txt - version = get_clang_tidy_version_from_requirements(repo_root) - hasher.update(version.encode()) - - # Hash the entire platformio.ini file - platformio_path = repo_root / "platformio.ini" - platformio_content = read_file_bytes(platformio_path) - hasher.update(platformio_content) - - # Hash sdkconfig.defaults and any per-target sdkconfig.defaults.: - # the per-target files flip CONFIG flags that change which variant code - # paths clang-tidy sees. Include the filename so a rename is detected. - for sdkconfig_path in sorted(repo_root.glob("sdkconfig.defaults*")): - hasher.update(sdkconfig_path.name.encode()) - hasher.update(read_file_bytes(sdkconfig_path)) - - # Hash esphome/idf_component.yml: its managed deps drive the ESP-IDF - # build's include set, which clang-tidy analyzes. - idf_component_path = repo_root / "esphome" / "idf_component.yml" - if idf_component_path.exists(): - hasher.update(read_file_bytes(idf_component_path)) + # Hash each sdkconfig.defaults* file. Include the filename so adding or + # renaming a per-target variant is detected, not just content edits. + for path in sorted(repo_root.glob(f"{SDKCONFIG_DEFAULTS_PREFIX}*")): + hasher.update(path.name.encode()) + hasher.update(read_file_bytes(path)) return hasher.hexdigest() - - -def read_stored_hash(repo_root: Path | None = None) -> str | None: - """Read the stored hash from file""" - repo_root = _ensure_repo_root(repo_root) - hash_file = repo_root / ".clang-tidy.hash" - if hash_file.exists(): - lines = read_file_lines(hash_file) - return lines[0].strip() if lines else None - return None - - -def write_file_content(path: Path, content: str) -> None: - """Write content to a file.""" - with path.open("w") as f: - f.write(content) - - -def write_hash(hash_value: str, repo_root: Path | None = None) -> None: - """Write hash to file""" - repo_root = _ensure_repo_root(repo_root) - hash_file = repo_root / ".clang-tidy.hash" - # Strip any trailing newlines to ensure consistent formatting - write_file_content(hash_file, hash_value.strip() + "\n") - - -def main() -> None: - parser = argparse.ArgumentParser(description="Manage clang-tidy configuration hash") - parser.add_argument( - "--check", - action="store_true", - help="Check if full scan needed (exit 0 if needed)", - ) - parser.add_argument("--update", action="store_true", help="Update the hash file") - parser.add_argument( - "--update-if-changed", - action="store_true", - help="Update hash only if configuration changed (for pre-commit)", - ) - parser.add_argument( - "--verify", action="store_true", help="Verify hash matches (for CI)" - ) - - args = parser.parse_args() - - current_hash = calculate_clang_tidy_hash() - stored_hash = read_stored_hash() - - if args.check: - # Check if hash changed OR if .clang-tidy.hash was updated in this PR - # This is used in CI to determine if a full clang-tidy scan is needed - hash_changed = current_hash != stored_hash - - # Lazy import to avoid requiring dependencies that aren't needed for other modes - from helpers import changed_files # noqa: E402 - - hash_file_updated = ".clang-tidy.hash" in changed_files() - - # Exit 0 if full scan needed - sys.exit(0 if (hash_changed or hash_file_updated) else 1) - - elif args.verify: - # Verify that hash file is up to date with current configuration - # This is used in pre-commit and CI checks to ensure hash was updated - if current_hash != stored_hash: - print("ERROR: Clang-tidy configuration has changed but hash not updated!") - print(f"Expected: {current_hash}") - print(f"Found: {stored_hash}") - print("\nPlease run: script/clang_tidy_hash.py --update") - sys.exit(1) - print("Hash verification passed") - - elif args.update: - write_hash(current_hash) - print(f"Hash updated: {current_hash}") - - elif args.update_if_changed: - if current_hash != stored_hash: - write_hash(current_hash) - print(f"Clang-tidy hash updated: {current_hash}") - # Exit 0 so pre-commit can stage the file - sys.exit(0) - else: - print("Clang-tidy hash unchanged") - sys.exit(0) - - else: - print(f"Current hash: {current_hash}") - print(f"Stored hash: {stored_hash}") - print(f"Match: {current_hash == stored_hash}") - - -if __name__ == "__main__": - main() diff --git a/script/determine-jobs.py b/script/determine-jobs.py index 94a78e8423..4904883ca9 100755 --- a/script/determine-jobs.py +++ b/script/determine-jobs.py @@ -55,10 +55,10 @@ from functools import cache import json import os from pathlib import Path -import subprocess import sys from typing import Any +from clang_tidy_hash import CLANG_TIDY_GLOBAL_FILES, SDKCONFIG_DEFAULTS_PREFIX from helpers import ( CPP_FILE_EXTENSIONS, ESPHOME_TESTS_COMPONENTS_PATH, @@ -280,23 +280,22 @@ def determine_integration_tests(branch: str | None = None) -> tuple[bool, list[s @cache -def _is_clang_tidy_full_scan() -> bool: - """Check if clang-tidy configuration changed (requires full scan). +def _is_clang_tidy_full_scan(branch: str | None = None) -> bool: + """Check if a clang-tidy-relevant config file changed (requires full scan). + + A change to a file that affects clang-tidy globally can surface warnings in + source files the PR didn't touch, so the entire codebase must be re-scanned. Returns: - True if full scan is needed (hash changed), False otherwise. + True if full scan is needed, False otherwise. """ - try: - result = subprocess.run( - [str(Path(root_path) / "script" / "clang_tidy_hash.py"), "--check"], - capture_output=True, - check=False, - ) - # Exit 0 means hash changed (full scan needed) - return result.returncode == 0 - except Exception: # noqa: BLE001 - # If hash check fails, run full scan to be safe - return True + for file in changed_files(branch): + if file in CLANG_TIDY_GLOBAL_FILES: + return True + # Root-level sdkconfig.defaults and per-target sdkconfig.defaults. + if "/" not in file and file.startswith(SDKCONFIG_DEFAULTS_PREFIX): + return True + return False def should_run_clang_tidy(branch: str | None = None) -> bool: @@ -307,13 +306,12 @@ def should_run_clang_tidy(branch: str | None = None) -> bool: Clang-tidy will run when ANY of the following conditions are met: - 1. Clang-tidy configuration changed - - The hash of .clang-tidy configuration file has changed - - The hash includes the .clang-tidy file, clang-tidy version from requirements_dev.txt, - and relevant platformio.ini sections - - When configuration changes, a full scan is needed to ensure all code complies - with the new rules - - Detected by script/clang_tidy_hash.py --check returning exit code 0 + 1. A clang-tidy-relevant config file changed (full scan needed) + - Any file in CLANG_TIDY_GLOBAL_FILES (.clang-tidy, platformio.ini, + requirements_dev.txt, esphome/idf_component.yml) or a root-level + sdkconfig.defaults* file + - These affect clang-tidy results globally, so all code must be re-checked + to ensure it still complies 2. Any C++ source files changed - Any file with C++ extensions: .cpp, .h, .hpp, .cc, .cxx, .c, .tcc @@ -321,27 +319,14 @@ def should_run_clang_tidy(branch: str | None = None) -> bool: - This ensures all C++ code is checked, including tests, examples, etc. - Examples: esphome/core/component.cpp, tests/custom/my_component.h - 3. The .clang-tidy.hash file itself changed - - This indicates the configuration has been updated and clang-tidy should run - - Ensures that PRs updating the clang-tidy configuration are properly validated - - If the hash check fails for any reason, clang-tidy runs as a safety measure to ensure - code quality is maintained. - Args: branch: Branch to compare against. If None, uses default. Returns: True if clang-tidy should run, False otherwise. """ - # First check if clang-tidy configuration changed (full scan needed) - if _is_clang_tidy_full_scan(): - return True - - # Check if .clang-tidy.hash file itself was changed - # This handles the case where the hash was properly updated in the PR - files = changed_files(branch) - if ".clang-tidy.hash" in files: + # First check if a clang-tidy-relevant config file changed (full scan needed) + if _is_clang_tidy_full_scan(branch): return True return _any_changed_file_endswith(branch, CPP_FILE_EXTENSIONS) @@ -1276,9 +1261,9 @@ def main() -> None: # Determine clang-tidy mode based on actual files that will be checked is_full_scan = False if run_clang_tidy: - # Full scan needed if: hash changed OR core files changed - # (is_core_change is forced True under --force-all) - is_full_scan = _is_clang_tidy_full_scan() or is_core_change + # Full scan needed if: a clang-tidy-relevant config file changed OR + # core files changed (is_core_change is forced True under --force-all) + is_full_scan = _is_clang_tidy_full_scan(args.branch) or is_core_change if is_full_scan: # Full scan checks all files - always use split mode for efficiency diff --git a/tests/script/test_clang_tidy_hash.py b/tests/script/test_clang_tidy_hash.py index 194926a5df..b5a9d8ebe9 100644 --- a/tests/script/test_clang_tidy_hash.py +++ b/tests/script/test_clang_tidy_hash.py @@ -1,9 +1,7 @@ """Unit tests for script/clang_tidy_hash.py module.""" -import hashlib from pathlib import Path import sys -from unittest.mock import Mock, patch import pytest @@ -11,76 +9,45 @@ import pytest sys.path.insert(0, str(Path(__file__).parent.parent.parent / "script")) import clang_tidy_hash # noqa: E402 +from clang_tidy_hash import CLANG_TIDY_GLOBAL_FILES # noqa: E402 -@pytest.mark.parametrize( - ("file_content", "expected"), - [ - ( - "clang-tidy==18.1.5 # via -r requirements_dev.in\n", - "clang-tidy==18.1.5 # via -r requirements_dev.in", - ), - ( - "other-package==1.0\nclang-tidy==17.0.0\nmore-packages==2.0\n", - "clang-tidy==17.0.0", - ), - ( - "# comment\nclang-tidy==16.0.0 # some comment\n", - "clang-tidy==16.0.0 # some comment", - ), - ("no-clang-tidy-here==1.0\n", "clang-tidy version not found"), - ], -) -def test_get_clang_tidy_version_from_requirements( - file_content: str, expected: str +def _populate(repo_root: Path) -> None: + """Create every clang-tidy global file plus a base sdkconfig.defaults.""" + for name in CLANG_TIDY_GLOBAL_FILES: + path = repo_root / name + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(f"contents of {name}\n") + (repo_root / "sdkconfig.defaults").write_text("CONFIG_BASE=y\n") + + +def test_calculate_clang_tidy_hash_is_deterministic(tmp_path: Path) -> None: + """Same inputs must produce the same hash.""" + _populate(tmp_path) + assert clang_tidy_hash.calculate_clang_tidy_hash( + repo_root=tmp_path + ) == clang_tidy_hash.calculate_clang_tidy_hash(repo_root=tmp_path) + + +@pytest.mark.parametrize("filename", CLANG_TIDY_GLOBAL_FILES) +def test_calculate_clang_tidy_hash_changes_with_each_global_file( + tmp_path: Path, filename: str ) -> None: - """Test extracting clang-tidy version from various file formats.""" - # Mock read_file_lines to return our test content - with patch("clang_tidy_hash.read_file_lines") as mock_read: - mock_read.return_value = file_content.splitlines(keepends=True) + """Editing any global file must change the hash.""" + _populate(tmp_path) + before = clang_tidy_hash.calculate_clang_tidy_hash(repo_root=tmp_path) - result = clang_tidy_hash.get_clang_tidy_version_from_requirements() + (tmp_path / filename).write_text("changed\n") + after = clang_tidy_hash.calculate_clang_tidy_hash(repo_root=tmp_path) - assert result == expected - - -def test_calculate_clang_tidy_hash_with_sdkconfig(tmp_path: Path) -> None: - """Test calculating hash from all configuration sources including sdkconfig.defaults.""" - clang_tidy_content = b"Checks: '-*,readability-*'\n" - requirements_version = "clang-tidy==18.1.5" - platformio_content = b"[env:esp32]\nplatform = espressif32\n" - sdkconfig_content = b"" - requirements_content = "clang-tidy==18.1.5\n" - - # Create temporary files - (tmp_path / ".clang-tidy").write_bytes(clang_tidy_content) - (tmp_path / "platformio.ini").write_bytes(platformio_content) - (tmp_path / "sdkconfig.defaults").write_bytes(sdkconfig_content) - (tmp_path / "requirements_dev.txt").write_text(requirements_content) - - # Expected hash calculation - expected_hasher = hashlib.sha256() - expected_hasher.update(clang_tidy_content) - expected_hasher.update(requirements_version.encode()) - expected_hasher.update(platformio_content) - expected_hasher.update(b"sdkconfig.defaults") - expected_hasher.update(sdkconfig_content) - expected_hash = expected_hasher.hexdigest() - - result = clang_tidy_hash.calculate_clang_tidy_hash(repo_root=tmp_path) - - assert result == expected_hash + assert after != before def test_calculate_clang_tidy_hash_includes_per_target_sdkconfig( tmp_path: Path, ) -> None: """Per-target sdkconfig.defaults. files must be part of the hash.""" - (tmp_path / ".clang-tidy").write_bytes(b"Checks: '-*'\n") - (tmp_path / "platformio.ini").write_bytes(b"[env:esp32]\n") - (tmp_path / "requirements_dev.txt").write_text("clang-tidy==18.1.5\n") - (tmp_path / "sdkconfig.defaults").write_bytes(b"CONFIG_BASE=y\n") - + _populate(tmp_path) before = clang_tidy_hash.calculate_clang_tidy_hash(repo_root=tmp_path) # Adding a per-target file must change the hash. @@ -95,230 +62,14 @@ def test_calculate_clang_tidy_hash_includes_per_target_sdkconfig( assert after_edit != after_add -def test_calculate_clang_tidy_hash_without_sdkconfig(tmp_path: Path) -> None: - """Test calculating hash without sdkconfig.defaults file.""" - clang_tidy_content = b"Checks: '-*,readability-*'\n" - requirements_version = "clang-tidy==18.1.5" - platformio_content = b"[env:esp32]\nplatform = espressif32\n" - requirements_content = "clang-tidy==18.1.5\n" - - # Create temporary files (without sdkconfig.defaults) - (tmp_path / ".clang-tidy").write_bytes(clang_tidy_content) - (tmp_path / "platformio.ini").write_bytes(platformio_content) - (tmp_path / "requirements_dev.txt").write_text(requirements_content) - - # Expected hash calculation (no sdkconfig) - expected_hasher = hashlib.sha256() - expected_hasher.update(clang_tidy_content) - expected_hasher.update(requirements_version.encode()) - expected_hasher.update(platformio_content) - expected_hash = expected_hasher.hexdigest() - +def test_calculate_clang_tidy_hash_handles_missing_optional_files( + tmp_path: Path, +) -> None: + """Hash calculation must not fail when files are absent.""" + # Only .clang-tidy present; everything else missing. + (tmp_path / ".clang-tidy").write_text("Checks: '-*'\n") result = clang_tidy_hash.calculate_clang_tidy_hash(repo_root=tmp_path) - - assert result == expected_hash - - -def test_read_stored_hash_exists(tmp_path: Path) -> None: - """Test reading hash when file exists.""" - stored_hash = "abc123def456" - hash_file = tmp_path / ".clang-tidy.hash" - hash_file.write_text(f"{stored_hash}\n") - - result = clang_tidy_hash.read_stored_hash(repo_root=tmp_path) - - assert result == stored_hash - - -def test_read_stored_hash_not_exists(tmp_path: Path) -> None: - """Test reading hash when file doesn't exist.""" - result = clang_tidy_hash.read_stored_hash(repo_root=tmp_path) - - assert result is None - - -def test_write_hash(tmp_path: Path) -> None: - """Test writing hash to file.""" - hash_value = "abc123def456" - hash_file = tmp_path / ".clang-tidy.hash" - - clang_tidy_hash.write_hash(hash_value, repo_root=tmp_path) - - assert hash_file.exists() - assert hash_file.read_text() == hash_value.strip() + "\n" - - -@pytest.mark.parametrize( - ("args", "current_hash", "stored_hash", "hash_file_in_changed", "expected_exit"), - [ - (["--check"], "abc123", "abc123", False, 1), # Hashes match, no scan needed - (["--check"], "abc123", "def456", False, 0), # Hashes differ, scan needed - (["--check"], "abc123", None, False, 0), # No stored hash, scan needed - ( - ["--check"], - "abc123", - "abc123", - True, - 0, - ), # Hash file updated in PR, scan needed - ], -) -def test_main_check_mode( - args: list[str], - current_hash: str, - stored_hash: str | None, - hash_file_in_changed: bool, - expected_exit: int, -) -> None: - """Test main function in check mode.""" - changed = [".clang-tidy.hash"] if hash_file_in_changed else [] - - # Create a mock module that can be imported - mock_helpers = Mock() - mock_helpers.changed_files = Mock(return_value=changed) - - with ( - patch("sys.argv", ["clang_tidy_hash.py"] + args), - patch("clang_tidy_hash.calculate_clang_tidy_hash", return_value=current_hash), - patch("clang_tidy_hash.read_stored_hash", return_value=stored_hash), - patch.dict("sys.modules", {"helpers": mock_helpers}), - pytest.raises(SystemExit) as exc_info, - ): - clang_tidy_hash.main() - - assert exc_info.value.code == expected_exit - - -def test_main_update_mode(capsys: pytest.CaptureFixture[str]) -> None: - """Test main function in update mode.""" - current_hash = "abc123" - - with ( - patch("sys.argv", ["clang_tidy_hash.py", "--update"]), - patch("clang_tidy_hash.calculate_clang_tidy_hash", return_value=current_hash), - patch("clang_tidy_hash.write_hash") as mock_write, - ): - clang_tidy_hash.main() - - mock_write.assert_called_once_with(current_hash) - captured = capsys.readouterr() - assert f"Hash updated: {current_hash}" in captured.out - - -@pytest.mark.parametrize( - ("current_hash", "stored_hash"), - [ - ("abc123", "def456"), # Hash changed, should update - ("abc123", None), # No stored hash, should update - ], -) -def test_main_update_if_changed_mode_update( - current_hash: str, stored_hash: str | None, capsys: pytest.CaptureFixture[str] -) -> None: - """Test main function in update-if-changed mode when update is needed.""" - with ( - patch("sys.argv", ["clang_tidy_hash.py", "--update-if-changed"]), - patch("clang_tidy_hash.calculate_clang_tidy_hash", return_value=current_hash), - patch("clang_tidy_hash.read_stored_hash", return_value=stored_hash), - patch("clang_tidy_hash.write_hash") as mock_write, - pytest.raises(SystemExit) as exc_info, - ): - clang_tidy_hash.main() - - assert exc_info.value.code == 0 - mock_write.assert_called_once_with(current_hash) - captured = capsys.readouterr() - assert "Clang-tidy hash updated" in captured.out - - -def test_main_update_if_changed_mode_no_update( - capsys: pytest.CaptureFixture[str], -) -> None: - """Test main function in update-if-changed mode when no update is needed.""" - current_hash = "abc123" - stored_hash = "abc123" - - with ( - patch("sys.argv", ["clang_tidy_hash.py", "--update-if-changed"]), - patch("clang_tidy_hash.calculate_clang_tidy_hash", return_value=current_hash), - patch("clang_tidy_hash.read_stored_hash", return_value=stored_hash), - patch("clang_tidy_hash.write_hash") as mock_write, - pytest.raises(SystemExit) as exc_info, - ): - clang_tidy_hash.main() - - assert exc_info.value.code == 0 - mock_write.assert_not_called() - captured = capsys.readouterr() - assert "Clang-tidy hash unchanged" in captured.out - - -def test_main_verify_mode_success(capsys: pytest.CaptureFixture[str]) -> None: - """Test main function in verify mode when verification passes.""" - current_hash = "abc123" - stored_hash = "abc123" - - with ( - patch("sys.argv", ["clang_tidy_hash.py", "--verify"]), - patch("clang_tidy_hash.calculate_clang_tidy_hash", return_value=current_hash), - patch("clang_tidy_hash.read_stored_hash", return_value=stored_hash), - ): - clang_tidy_hash.main() - captured = capsys.readouterr() - assert "Hash verification passed" in captured.out - - -@pytest.mark.parametrize( - ("current_hash", "stored_hash"), - [ - ("abc123", "def456"), # Hashes differ, verification fails - ("abc123", None), # No stored hash, verification fails - ], -) -def test_main_verify_mode_failure( - current_hash: str, stored_hash: str | None, capsys: pytest.CaptureFixture[str] -) -> None: - """Test main function in verify mode when verification fails.""" - with ( - patch("sys.argv", ["clang_tidy_hash.py", "--verify"]), - patch("clang_tidy_hash.calculate_clang_tidy_hash", return_value=current_hash), - patch("clang_tidy_hash.read_stored_hash", return_value=stored_hash), - pytest.raises(SystemExit) as exc_info, - ): - clang_tidy_hash.main() - - assert exc_info.value.code == 1 - captured = capsys.readouterr() - assert "ERROR: Clang-tidy configuration has changed" in captured.out - - -def test_main_default_mode(capsys: pytest.CaptureFixture[str]) -> None: - """Test main function in default mode (no arguments).""" - current_hash = "abc123" - stored_hash = "def456" - - with ( - patch("sys.argv", ["clang_tidy_hash.py"]), - patch("clang_tidy_hash.calculate_clang_tidy_hash", return_value=current_hash), - patch("clang_tidy_hash.read_stored_hash", return_value=stored_hash), - ): - clang_tidy_hash.main() - - captured = capsys.readouterr() - assert f"Current hash: {current_hash}" in captured.out - assert f"Stored hash: {stored_hash}" in captured.out - assert "Match: False" in captured.out - - -def test_read_file_lines(tmp_path: Path) -> None: - """Test read_file_lines helper function.""" - test_file = tmp_path / "test.txt" - test_content = "line1\nline2\nline3\n" - test_file.write_text(test_content) - - result = clang_tidy_hash.read_file_lines(test_file) - - assert result == ["line1\n", "line2\n", "line3\n"] + assert len(result) == 64 # sha256 hexdigest length def test_read_file_bytes(tmp_path: Path) -> None: @@ -330,35 +81,3 @@ def test_read_file_bytes(tmp_path: Path) -> None: result = clang_tidy_hash.read_file_bytes(test_file) assert result == test_content - - -def test_write_file_content(tmp_path: Path) -> None: - """Test write_file_content helper function.""" - test_file = tmp_path / "test.txt" - test_content = "test content" - - clang_tidy_hash.write_file_content(test_file, test_content) - - assert test_file.read_text() == test_content - - -@pytest.mark.parametrize( - ("line", "expected"), - [ - ("clang-tidy==18.1.5", ("clang-tidy", "clang-tidy==18.1.5")), - ( - "clang-tidy==18.1.5 # comment", - ("clang-tidy", "clang-tidy==18.1.5 # comment"), - ), - ("some-package>=1.0,<2.0", ("some-package", "some-package>=1.0,<2.0")), - ("pkg_with-dashes==1.0", ("pkg_with-dashes", "pkg_with-dashes==1.0")), - ("# just a comment", None), - ("", None), - (" ", None), - ("invalid line without version", None), - ], -) -def test_parse_requirement_line(line: str, expected: tuple[str, str] | None) -> None: - """Test parsing individual requirement lines.""" - result = clang_tidy_hash.parse_requirement_line(line) - assert result == expected diff --git a/tests/script/test_determine_jobs.py b/tests/script/test_determine_jobs.py index a9defcacac..f8f359ee22 100644 --- a/tests/script/test_determine_jobs.py +++ b/tests/script/test_determine_jobs.py @@ -5,7 +5,7 @@ import importlib.util import json from pathlib import Path import sys -from unittest.mock import Mock, call, patch +from unittest.mock import Mock, patch import pytest @@ -653,52 +653,38 @@ def test_determine_integration_tests_non_yaml_fixture_runs_all() -> None: @pytest.mark.parametrize( - ("check_returncode", "changed_files", "expected_result"), + ("changed_files", "expected_result"), [ - (0, [], True), # Hash changed - need full scan - (1, ["esphome/core.cpp"], True), # C++ file changed - (1, ["README.md"], False), # No C++ files changed - (1, [".clang-tidy.hash"], True), # Hash file itself changed - (1, ["platformio.ini", ".clang-tidy.hash"], True), # Config + hash changed + ([], False), # Nothing changed + (["esphome/core.cpp"], True), # C++ file changed + (["README.md"], False), # No C++ files changed + ([".clang-tidy"], True), # clang-tidy config changed - full scan + (["platformio.ini"], True), # build config changed - full scan + (["requirements_dev.txt"], True), # clang-tidy version source changed + (["sdkconfig.defaults"], True), # sdkconfig changed - full scan + (["sdkconfig.defaults.esp32c6"], True), # per-target sdkconfig changed + (["esphome/idf_component.yml"], True), # idf managed deps changed + (["platformio.ini", "README.md"], True), # config + non-C++ ], ) def test_should_run_clang_tidy( - check_returncode: int, changed_files: list[str], expected_result: bool, ) -> None: """Test should_run_clang_tidy function.""" - with ( - patch.object(determine_jobs, "changed_files", return_value=changed_files), - patch("subprocess.run") as mock_run, - ): - # Test with hash check returning specific code - mock_run.return_value = Mock(returncode=check_returncode) + with patch.object(determine_jobs, "changed_files", return_value=changed_files): result = determine_jobs.should_run_clang_tidy() assert result == expected_result -def test_should_run_clang_tidy_hash_check_exception() -> None: - """Test should_run_clang_tidy when hash check fails with exception.""" - # When hash check fails, clang-tidy should run as a safety measure - with ( - patch.object(determine_jobs, "changed_files", return_value=["README.md"]), - patch("subprocess.run", side_effect=Exception("Hash check failed")), - ): - result = determine_jobs.should_run_clang_tidy() - assert result is True # Fail safe - run clang-tidy - - def test_should_run_clang_tidy_with_branch() -> None: """Test should_run_clang_tidy with branch argument.""" with patch.object(determine_jobs, "changed_files") as mock_changed: mock_changed.return_value = [] - with patch("subprocess.run") as mock_run: - mock_run.return_value = Mock(returncode=1) # Hash unchanged - determine_jobs.should_run_clang_tidy("release") - # Changed files is called twice now - once for hash check, once for .clang-tidy.hash check - assert mock_changed.call_count == 2 - mock_changed.assert_has_calls([call("release"), call("release")]) + determine_jobs.should_run_clang_tidy("release") + # changed_files is queried against the given branch by both the + # config-file full-scan check and the C++ extension check. + mock_changed.assert_called_with("release") @pytest.mark.parametrize(