mirror of
https://github.com/esphome/esphome.git
synced 2026-06-05 01:52:47 +08:00
[ci] Address Copilot review on device-builder gate
- Skip the device-builder downstream job on beta/release target branches. Those branches lag behind device-builder@main, so a newer device-builder API requirement would falsely fail the run without reflecting any problem in the PR itself. Mirrors the same skip detect_memory_impact_config already does. - Broaden the trigger to any non-C++ file under esphome/. The package ships data files via include-package-data = true (e.g. esphome/idf_component.yml, dashboard templates, JSON), so a Python-only filter under-fires for changes that still affect what device-builder installs. Tests cover both: per-file behavior (with the skip mocked off) and the beta/release skip itself short-circuiting before changed_files is even consulted.
This commit is contained in:
@@ -457,9 +457,15 @@ def should_run_device_builder(branch: str | None = None) -> bool:
|
|||||||
"""Determine if downstream esphome/device-builder tests should run.
|
"""Determine if downstream esphome/device-builder tests should run.
|
||||||
|
|
||||||
device-builder imports esphome as a library, so whenever the importable
|
device-builder imports esphome as a library, so whenever the importable
|
||||||
Python surface or the runtime dependencies change we re-run its test
|
Python surface, the runtime dependencies, or any non-C++ file packaged
|
||||||
suite against the PR's code to catch breakage we'd otherwise only see
|
with esphome (pyproject.toml has ``include-package-data = true``, so
|
||||||
after a release.
|
things like esphome/idf_component.yml ship and can affect installs)
|
||||||
|
changes we re-run its test suite against the PR's code to catch
|
||||||
|
breakage we'd otherwise only see after a release.
|
||||||
|
|
||||||
|
Skipped on beta/release branches: those branches typically lag behind
|
||||||
|
device-builder@main, so a new device-builder API dependency would
|
||||||
|
falsely fail the run without reflecting any problem in the PR itself.
|
||||||
|
|
||||||
Args:
|
Args:
|
||||||
branch: Branch to compare against. If None, uses default.
|
branch: Branch to compare against. If None, uses default.
|
||||||
@@ -467,11 +473,22 @@ def should_run_device_builder(branch: str | None = None) -> bool:
|
|||||||
Returns:
|
Returns:
|
||||||
True if the device-builder downstream tests should run, False otherwise.
|
True if the device-builder downstream tests should run, False otherwise.
|
||||||
"""
|
"""
|
||||||
|
target_branch = get_target_branch()
|
||||||
|
if target_branch and (
|
||||||
|
target_branch.startswith("release") or target_branch.startswith("beta")
|
||||||
|
):
|
||||||
|
return False
|
||||||
|
|
||||||
for file in changed_files(branch):
|
for file in changed_files(branch):
|
||||||
if file.startswith("esphome/") and file.endswith(PYTHON_FILE_EXTENSIONS):
|
|
||||||
return True
|
|
||||||
if file in DEVICE_BUILDER_TRIGGER_FILES:
|
if file in DEVICE_BUILDER_TRIGGER_FILES:
|
||||||
return True
|
return True
|
||||||
|
# Anything under esphome/ that isn't C++ source can change the
|
||||||
|
# importable / packaged surface device-builder consumes
|
||||||
|
# (Python sources, packaged YAML/JSON like idf_component.yml,
|
||||||
|
# etc.). C++ files only affect compiled firmware, not the
|
||||||
|
# Python install device-builder pulls in.
|
||||||
|
if file.startswith("esphome/") and not file.endswith(CPP_FILE_EXTENSIONS):
|
||||||
|
return True
|
||||||
return False
|
return False
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -762,17 +762,27 @@ def test_should_run_import_time_with_branch() -> None:
|
|||||||
# Runtime dependency changes trigger
|
# Runtime dependency changes trigger
|
||||||
(["requirements.txt"], True),
|
(["requirements.txt"], True),
|
||||||
(["pyproject.toml"], True),
|
(["pyproject.toml"], True),
|
||||||
|
# Non-C++ files packaged with esphome trigger -- device-builder
|
||||||
|
# picks them up because esphome's pyproject sets
|
||||||
|
# include-package-data = true.
|
||||||
|
(["esphome/idf_component.yml"], True),
|
||||||
|
(["esphome/dashboard/templates/index.html"], True),
|
||||||
|
(["esphome/components/api/api_pb2_service.json"], True),
|
||||||
# Mixed: any triggering file is enough
|
# Mixed: any triggering file is enough
|
||||||
(["docs/README.md", "esphome/config.py"], True),
|
(["docs/README.md", "esphome/config.py"], True),
|
||||||
# Dev/test-only dependency changes don't trigger device-builder
|
# Dev/test-only dependency changes don't trigger device-builder
|
||||||
# (they don't affect the importable surface device-builder uses)
|
# (they don't affect the importable surface device-builder uses)
|
||||||
(["requirements_dev.txt"], False),
|
(["requirements_dev.txt"], False),
|
||||||
(["requirements_test.txt"], False),
|
(["requirements_test.txt"], False),
|
||||||
# Python files outside esphome/ don't trigger
|
# Files outside esphome/ don't trigger
|
||||||
(["script/some_other_script.py"], False),
|
(["script/some_other_script.py"], False),
|
||||||
(["tests/script/test_determine_jobs.py"], False),
|
(["tests/script/test_determine_jobs.py"], False),
|
||||||
# Non-Python changes don't trigger
|
# C++ files under esphome/ don't trigger -- they only affect
|
||||||
|
# compiled firmware, not the Python install device-builder pulls in.
|
||||||
(["esphome/core/component.cpp"], False),
|
(["esphome/core/component.cpp"], False),
|
||||||
|
(["esphome/core/component.h"], False),
|
||||||
|
(["esphome/components/wifi/wifi_component.cpp"], False),
|
||||||
|
# Files outside esphome/ entirely
|
||||||
(["tests/components/wifi/test.esp32-idf.yaml"], False),
|
(["tests/components/wifi/test.esp32-idf.yaml"], False),
|
||||||
(["README.md"], False),
|
(["README.md"], False),
|
||||||
([], False),
|
([], False),
|
||||||
@@ -781,20 +791,42 @@ def test_should_run_import_time_with_branch() -> None:
|
|||||||
def test_should_run_device_builder(
|
def test_should_run_device_builder(
|
||||||
changed_files: list[str], expected_result: bool
|
changed_files: list[str], expected_result: bool
|
||||||
) -> None:
|
) -> None:
|
||||||
"""Test should_run_device_builder function."""
|
"""Test should_run_device_builder function (non-beta/release target)."""
|
||||||
with patch.object(determine_jobs, "changed_files", return_value=changed_files):
|
with (
|
||||||
|
patch.object(determine_jobs, "changed_files", return_value=changed_files),
|
||||||
|
# Mock target branch to "dev" so the beta/release skip is bypassed
|
||||||
|
# for these per-file behavior checks.
|
||||||
|
patch.object(determine_jobs, "get_target_branch", return_value="dev"),
|
||||||
|
):
|
||||||
result = determine_jobs.should_run_device_builder()
|
result = determine_jobs.should_run_device_builder()
|
||||||
assert result == expected_result
|
assert result == expected_result
|
||||||
|
|
||||||
|
|
||||||
def test_should_run_device_builder_with_branch() -> None:
|
def test_should_run_device_builder_with_branch() -> None:
|
||||||
"""Test should_run_device_builder with branch argument."""
|
"""Test should_run_device_builder with branch argument."""
|
||||||
with patch.object(determine_jobs, "changed_files") as mock_changed:
|
with (
|
||||||
|
patch.object(determine_jobs, "changed_files") as mock_changed,
|
||||||
|
patch.object(determine_jobs, "get_target_branch", return_value="dev"),
|
||||||
|
):
|
||||||
mock_changed.return_value = []
|
mock_changed.return_value = []
|
||||||
determine_jobs.should_run_device_builder("release")
|
determine_jobs.should_run_device_builder("release")
|
||||||
mock_changed.assert_called_once_with("release")
|
mock_changed.assert_called_once_with("release")
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.parametrize("target_branch", ["beta", "release", "release-2026.5"])
|
||||||
|
def test_should_run_device_builder_skips_beta_release(target_branch: str) -> None:
|
||||||
|
"""Beta/release target branches skip device-builder (lag behind device-builder@main)."""
|
||||||
|
with (
|
||||||
|
patch.object(determine_jobs, "get_target_branch", return_value=target_branch),
|
||||||
|
patch.object(determine_jobs, "changed_files") as mock_changed,
|
||||||
|
):
|
||||||
|
# Even with a triggering file present, the target-branch guard wins.
|
||||||
|
mock_changed.return_value = ["esphome/__main__.py"]
|
||||||
|
assert determine_jobs.should_run_device_builder() is False
|
||||||
|
# changed_files shouldn't even be consulted -- the guard short-circuits.
|
||||||
|
mock_changed.assert_not_called()
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.parametrize(
|
@pytest.mark.parametrize(
|
||||||
("changed_files", "expected_result"),
|
("changed_files", "expected_result"),
|
||||||
[
|
[
|
||||||
|
|||||||
Reference in New Issue
Block a user