From 49fc4be861cf4fdbf946489c5e96fd1373b891ad Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 24 Aug 2026 12:18:17 -0500 Subject: [PATCH] [ci] Run C++ unit tests when a component's Python or test override changes (#18706) --- script/build_helpers.py | 9 +-- script/determine-jobs.py | 14 +++-- script/helpers.py | 70 +++++++++++++++-------- script/list-components.py | 13 ++--- tests/script/test_determine_jobs.py | 50 ++++++++++++++++ tests/script/test_helpers.py | 89 +++++++++++++++++++++++++++++ 6 files changed, 202 insertions(+), 43 deletions(-) diff --git a/script/build_helpers.py b/script/build_helpers.py index 50830c221e..b4b25924c3 100644 --- a/script/build_helpers.py +++ b/script/build_helpers.py @@ -10,7 +10,7 @@ from pathlib import Path import subprocess import sys -from helpers import get_all_dependencies, root_path as _root_path +from helpers import get_all_dependencies, has_cpp_unit_tests, root_path as _root_path import yaml # Ensure the repo root is on sys.path so that ``tests.testing_helpers`` and @@ -131,14 +131,11 @@ def filter_components_with_files(components: list[str], tests_dir: Path) -> list """ filtered_components: list[str] = [] for component in components: - test_dir = tests_dir / component - if test_dir.is_dir() and ( - any(test_dir.glob("*.cpp")) or any(test_dir.glob("*.h")) - ): + if has_cpp_unit_tests(component, tests_dir): filtered_components.append(component) else: print( - f"WARNING: No files found for component '{component}' in {test_dir}, skipping.", + f"WARNING: No files found for component '{component}' in {tests_dir / component}, skipping.", file=sys.stderr, ) return filtered_components diff --git a/script/determine-jobs.py b/script/determine-jobs.py index 3e11deeb9a..2bdf7807a9 100755 --- a/script/determine-jobs.py +++ b/script/determine-jobs.py @@ -66,7 +66,6 @@ from helpers import ( base_python_changed, changed_files, core_changed, - filter_component_and_test_cpp_files, filter_component_and_test_files, get_changed_components, get_component_from_path, @@ -628,12 +627,17 @@ def determine_cpp_unit_tests( C++ unit tests will run when any of the following conditions are met: - 1. Any C++ core source files changed (esphome/core/*), in which case + 1. Any core C++ or Python files changed (esphome/core/*), in which case all cpp unit tests run. 2. A test file for a component changed, which triggers tests for that component. 3. The code for a component changed, which triggers tests for that - component and all components that depend on it. + component and all components that depend on it. Python files count + too: a component's Python decides which sources and defines go into + the host test build, so a Python-only change can break the link. + + Components without C++ test sources are dropped from the list, so the + job is only scheduled when there is something to build. Args: branch: Branch to compare against. If None, uses default. @@ -647,9 +651,7 @@ def determine_cpp_unit_tests( if core_changed(files): return (True, []) - # Filter to only C++ files - cpp_files = list(filter(filter_component_and_test_cpp_files, files)) - return (False, get_cpp_changed_components(cpp_files)) + return (False, get_cpp_changed_components(files)) # Paths within tests/benchmarks/ that contain component benchmark files diff --git a/script/helpers.py b/script/helpers.py index 8132ee49e5..9e3969e5ce 100644 --- a/script/helpers.py +++ b/script/helpers.py @@ -1150,17 +1150,41 @@ def filter_component_and_test_files(file_path: str) -> bool: ) -def filter_component_and_test_cpp_files(file_path: str) -> bool: - """Check if a file is a C++ source file in component or test directories. +def filter_cpp_unit_test_files(file_path: str) -> bool: + """Check if a file can affect a component's C++ unit test build. + + Besides C++ sources, a component's Python code (defines, source file + filters, libraries) and the ``__init__.py`` manifest overrides under + ``tests/components//`` decide what the host test binary + compiles and links. Other Python files under ``tests/components/`` + (pytest conftest.py, fixtures) do not. Args: file_path: Path to check Returns: - True if the file is a C++ source/header file in component or test directories + True if the file is a C++ or Python file in a component directory, or + a C++ file or ``__init__.py`` in a component test directory """ - return file_path.endswith(CPP_FILE_EXTENSIONS) and file_path.startswith( - COMPONENT_AND_TESTS_PATHS + if file_path.startswith(ESPHOME_COMPONENTS_PATH): + return file_path.endswith(CPP_AND_PYTHON_FILE_EXTENSIONS) + if file_path.startswith(ESPHOME_TESTS_COMPONENTS_PATH): + return file_path.endswith(CPP_FILE_EXTENSIONS) or file_path.endswith( + "/__init__.py" + ) + return False + + +def has_cpp_unit_tests(component: str, tests_dir: Path) -> bool: + """Check if a component has C++ test or benchmark sources in ``tests_dir``. + + Shared by CI job selection and the build itself + (``build_helpers.filter_components_with_files``) so both agree on + which components have something to build. + """ + component_dir = tests_dir / component + return component_dir.is_dir() and ( + any(component_dir.glob("*.cpp")) or any(component_dir.glob("*.h")) ) @@ -1486,41 +1510,41 @@ def base_python_changed(files: list[str]) -> bool: def get_cpp_changed_components(files: list[str]) -> list[str]: - """Get components that have changed C++ files or tests. + """Get components whose C++ unit tests are affected by changed files. This function analyzes a list of changed files and determines which components are affected. It handles two scenarios: - 1. Test files changed (tests/components//*.cpp): + 1. Test files changed (tests/components//*.cpp or __init__.py): - Adds the component to the affected list - Only that component needs to be tested - 2. Component C++ files changed (esphome/components//*): + 2. Component files changed (esphome/components//*.cpp or *.py): - Adds the component to the affected list - Also adds all components that depend on this component (recursively) - This ensures that changes propagate to dependent components + Python files count because a component's Python code decides which + sources and defines end up in the host test build. Components without + C++ test sources are dropped so CI does not schedule the job for nothing. + Args: - files: List of file paths to analyze (should be C++ files) + files: List of changed file paths; irrelevant ones are ignored Returns: Sorted list of component names that need C++ unit tests run """ components_graph = create_components_graph() + tests_dir = Path(root_path) / ESPHOME_TESTS_COMPONENTS_PATH affected: set[str] = set() for file in files: - if not file.endswith(CPP_FILE_EXTENSIONS): + if not filter_cpp_unit_test_files(file): continue - if file.startswith(ESPHOME_TESTS_COMPONENTS_PATH): - parts = file.split("/") - if len(parts) >= 4: - component_dir = Path(ESPHOME_TESTS_COMPONENTS_PATH) / parts[2] - if component_dir.is_dir(): - affected.add(parts[2]) - elif file.startswith(ESPHOME_COMPONENTS_PATH): - parts = file.split("/") - if len(parts) >= 4: - component = parts[2] - affected.update(find_children_of_component(components_graph, component)) - affected.add(component) - return sorted(affected) + parts = file.split("/") + if len(parts) < 4: + continue + component = parts[2] + affected.add(component) + if file.startswith(ESPHOME_COMPONENTS_PATH): + affected.update(find_children_of_component(components_graph, component)) + return sorted(c for c in affected if has_cpp_unit_tests(c, tests_dir)) diff --git a/script/list-components.py b/script/list-components.py index 31a1609f88..45efccb133 100755 --- a/script/list-components.py +++ b/script/list-components.py @@ -3,7 +3,6 @@ import argparse from helpers import ( changed_files, - filter_component_and_test_cpp_files, filter_component_and_test_files, get_all_component_files, get_components_with_dependencies, @@ -38,7 +37,7 @@ def main(): parser.add_argument( "--cpp-changed", action="store_true", - help="List components with changed C++ files", + help="List components whose C++ unit tests are affected by changed files", ) args = parser.parse_args() @@ -78,9 +77,9 @@ def main(): # Returns: Components with code changes + their dependencies (not infrastructure) # Reason: CI needs to test changed components and their dependents # - # - --cpp-changed: Used by CI to determine if any C++ files changed (script/determine-jobs.py) - # Returns: Only components with changed C++ files - # Reason: Only components with C++ changes need C++ testing + # - --cpp-changed: Mirrors the C++ unit test selection in script/determine-jobs.py + # Returns: Components with changed C++ or Python files (plus dependents) + # Reason: Python decides which sources and defines go into the host test build base_test_changed = any( "tests/test_build_components" in file for file in changed @@ -115,9 +114,7 @@ def main(): for c in get_components_with_dependencies(files, False): print(c) elif args.cpp_changed: - # Only look at changed cpp files - files = list(filter(filter_component_and_test_cpp_files, changed)) - for c in get_cpp_changed_components(files): + for c in get_cpp_changed_components(changed): print(c) else: # Return all changed components (with dependencies) - default behavior diff --git a/tests/script/test_determine_jobs.py b/tests/script/test_determine_jobs.py index b42c33de96..565f8c563f 100644 --- a/tests/script/test_determine_jobs.py +++ b/tests/script/test_determine_jobs.py @@ -1214,6 +1214,56 @@ def test_count_changed_cpp_files_with_branch() -> None: mock_changed.assert_called_once_with("release") +@pytest.mark.parametrize( + ("changed_files", "expected"), + [ + # Core C++ change runs everything + (["esphome/core/helpers.cpp"], (True, [])), + # Core Python change runs everything too + (["esphome/core/config.py"], (True, [])), + # Component C++ change: component plus dependents with C++ tests + (["esphome/components/time/posix_tz.cpp"], (False, ["sntp", "time"])), + # Component Python change shapes the host build (defines, source + # filters), so it must trigger the same tests as a C++ change + (["esphome/components/time/__init__.py"], (False, ["sntp", "time"])), + # Nothing to build when no selected component has C++ tests + (["esphome/components/homeassistant/__init__.py"], (False, [])), + # Test manifest override changes only that component + (["tests/components/time/__init__.py"], (False, ["time"])), + # Test source change only that component + (["tests/components/time/posix_tz.cpp"], (False, ["time"])), + # pytest files and YAML build tests do not affect the test binary + (["tests/components/socket/conftest.py"], (False, [])), + (["tests/components/time/test.esp32-idf.yaml"], (False, [])), + (["README.md", "script/helpers.py"], (False, [])), + ([], (False, [])), + ], +) +def test_determine_cpp_unit_tests( + changed_files: list[str], + expected: tuple[bool, list[str]], + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Test which C++ unit tests a set of changed files selects.""" + tests_dir = tmp_path / "tests" / "components" + for component in ("time", "sntp"): + (tests_dir / component).mkdir(parents=True) + (tests_dir / component / f"{component}.cpp").write_text("") + (tests_dir / "homeassistant").mkdir() + (tests_dir / "socket").mkdir() + monkeypatch.setattr(helpers, "root_path", str(tmp_path)) + with ( + patch.object(determine_jobs, "changed_files", return_value=changed_files), + patch.object( + helpers, + "create_components_graph", + return_value={"time": ["homeassistant", "sntp"]}, + ), + ): + assert determine_jobs.determine_cpp_unit_tests() == expected + + def test_main_filters_components_without_tests( mock_determine_integration_tests: Mock, mock_should_run_clang_tidy: Mock, diff --git a/tests/script/test_helpers.py b/tests/script/test_helpers.py index 2c3ae95655..38b8c57368 100644 --- a/tests/script/test_helpers.py +++ b/tests/script/test_helpers.py @@ -2031,3 +2031,92 @@ def test_get_changed_files_from_command_gh_failure_keeps_stderr() -> None: pytest.raises(Exception, match="maximum number of changed files"), ): _get_changed_files_from_command(["gh", "pr", "diff", "123", "--name-only"]) + + +@pytest.mark.parametrize( + ("file_path", "expected"), + [ + ("esphome/components/time/posix_tz.cpp", True), + ("esphome/components/time/posix_tz.h", True), + ("esphome/components/time/__init__.py", True), + ("esphome/components/sntp/time.py", True), + ("tests/components/time/posix_tz.cpp", True), + ("tests/components/time/__init__.py", True), + # Platform override: tests/components///__init__.py + ("tests/components/template/sensor/__init__.py", True), + # pytest-only files do not shape the C++ test binary + ("tests/components/socket/conftest.py", False), + ("tests/components/socket/test_socket.py", False), + ("tests/components/time/test.esp32-idf.yaml", False), + ("esphome/core/time.cpp", False), + ("esphome/config.py", False), + ("script/helpers.py", False), + ("README.md", False), + ], +) +def test_filter_cpp_unit_test_files(file_path: str, expected: bool) -> None: + """Test which changed files can affect a component's C++ unit test build.""" + assert helpers.filter_cpp_unit_test_files(file_path) is expected + + +@pytest.fixture +def cpp_unit_test_tree(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: + """Fake repo root where time, sntp and api have C++ unit tests. + + homeassistant depends on time but has no C++ tests, so it must be + dropped from the selection; socket has only pytest files. + """ + tests_dir = tmp_path / "tests" / "components" + for component in ("time", "sntp", "api"): + (tests_dir / component).mkdir(parents=True) + (tests_dir / component / f"{component}.cpp").write_text("") + (tests_dir / "homeassistant").mkdir() + (tests_dir / "homeassistant" / "__init__.py").write_text("") + (tests_dir / "socket").mkdir() + (tests_dir / "socket" / "conftest.py").write_text("") + monkeypatch.setattr(helpers, "root_path", str(tmp_path)) + monkeypatch.setattr( + helpers, + "create_components_graph", + lambda: {"time": ["homeassistant", "sntp"]}, + ) + return tmp_path + + +@pytest.mark.parametrize( + ("files", "expected"), + [ + # Component changes expand to dependents with C++ tests + (["esphome/components/time/posix_tz.cpp"], ["sntp", "time"]), + (["esphome/components/time/__init__.py"], ["sntp", "time"]), + # Dependent without C++ tests is dropped + (["esphome/components/homeassistant/__init__.py"], []), + # Test changes select only that component + (["tests/components/time/posix_tz.cpp"], ["time"]), + (["tests/components/time/__init__.py"], ["time"]), + (["tests/components/homeassistant/__init__.py"], []), + (["tests/components/socket/conftest.py"], []), + (["tests/components/time/test.esp32-idf.yaml"], []), + ( + ["esphome/components/time/__init__.py", "tests/components/api/api.cpp"], + ["api", "sntp", "time"], + ), + ([], []), + ], +) +@pytest.mark.usefixtures("cpp_unit_test_tree") +def test_get_cpp_changed_components(files: list[str], expected: list[str]) -> None: + """Test that C++ and Python component changes select the right unit tests.""" + assert helpers.get_cpp_changed_components(files) == expected + + +def test_get_cpp_changed_components_independent_of_cwd( + cpp_unit_test_tree: Path, + tmp_path_factory: pytest.TempPathFactory, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Test directories resolve against root_path, not the current directory.""" + monkeypatch.chdir(tmp_path_factory.mktemp("elsewhere")) + assert helpers.get_cpp_changed_components( + ["tests/components/time/__init__.py"] + ) == ["time"]