From 637180a628eab91ab7cb1b7f68549c09d31c31cf Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 16 Mar 2026 21:30:00 -1000 Subject: [PATCH] Fix review issues: boolean comparison, missing trigger, constants - Fix benchmarks CI condition: 'true' not 'True' (jq outputs lowercase) - Add script/setup_codspeed_lib.py to benchmark infrastructure triggers - Extract BENCHMARK_INFRASTRUCTURE_FILES and BENCHMARKS_COMPONENTS_PATH as top-level constants in determine-jobs.py - Fix extra_include_dirs docstring --- .github/workflows/ci.yml | 2 +- script/determine-jobs.py | 20 ++++++++++++++++---- script/test_helpers.py | 4 ++-- 3 files changed, 19 insertions(+), 7 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 554a49bd13..6f20a4a7d2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -316,7 +316,7 @@ jobs: needs: - common - determine-jobs - if: github.event_name == 'pull_request' && needs.determine-jobs.outputs.benchmarks == 'True' + if: github.event_name == 'pull_request' && needs.determine-jobs.outputs.benchmarks == 'true' steps: - name: Check out code from GitHub uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 diff --git a/script/determine-jobs.py b/script/determine-jobs.py index e9fcd8925f..ad08f8dce5 100755 --- a/script/determine-jobs.py +++ b/script/determine-jobs.py @@ -381,6 +381,19 @@ def determine_cpp_unit_tests( return (False, get_cpp_changed_components(cpp_files)) +# Paths within tests/benchmarks/ that contain component benchmark files +BENCHMARKS_COMPONENTS_PATH = "tests/benchmarks/components" + +# Files that, when changed, should trigger benchmark runs +BENCHMARK_INFRASTRUCTURE_FILES = frozenset( + { + "script/cpp_benchmark.py", + "script/test_helpers.py", + "script/setup_codspeed_lib.py", + } +) + + def should_run_benchmarks(branch: str | None = None) -> bool: """Determine if C++ benchmarks should run based on changed files. @@ -389,7 +402,7 @@ def should_run_benchmarks(branch: str | None = None) -> bool: 1. Core C++ files changed (esphome/core/*) 2. A directly changed component has benchmark files (no dependency expansion) 3. Benchmark infrastructure changed (tests/benchmarks/*, script/cpp_benchmark.py, - script/test_helpers.py) + script/test_helpers.py, script/setup_codspeed_lib.py) Unlike unit tests, benchmarks do NOT expand to dependent components. Changing ``sensor`` does not trigger ``api`` benchmarks just because @@ -407,14 +420,13 @@ def should_run_benchmarks(branch: str | None = None) -> bool: # Check if benchmark infrastructure changed if any( - f.startswith("tests/benchmarks/") - or f in ("script/cpp_benchmark.py", "script/test_helpers.py") + f.startswith("tests/benchmarks/") or f in BENCHMARK_INFRASTRUCTURE_FILES for f in files ): return True # Check if any directly changed component has benchmarks - benchmarks_dir = Path(root_path) / "tests" / "benchmarks" / "components" + benchmarks_dir = Path(root_path) / BENCHMARKS_COMPONENTS_PATH if not benchmarks_dir.is_dir(): return False benchmarked_components = { diff --git a/script/test_helpers.py b/script/test_helpers.py index 5ca2e4e408..25ff062e02 100644 --- a/script/test_helpers.py +++ b/script/test_helpers.py @@ -328,8 +328,8 @@ def build_and_run( label: Label for log messages build_only: If True, print binary path and return without running extra_run_args: Extra arguments to pass to the binary - extra_include_dirs: Additional directories (relative to tests_dir) - whose .cpp files should be compiled + extra_include_dirs: Additional directories whose .cpp files + should be compiled (resolved relative to tests_dir if possible) Returns: Exit code