mirror of
https://github.com/esphome/esphome.git
synced 2026-09-17 01:58:39 +00:00
[ci] Address copilot review feedback on import-time check
- determine-jobs: trigger on changes to requirements_test.txt too; that file is hashed into the venv cache key and installed during restore-python, so a change there can alter the import environment. - ci.yml: merge the --check and --har steps so we only run importtime-waterfall once per job (was measuring twice: ~7-8s of wasted CI time per run). Uses the new script/check_import_time.py --check --har <path> combination; the HAR reflects the same measurement that produced the pass/fail decision. - script/check_import_time.py: refactor the CLI so --har is a standalone option rather than a mutually-exclusive mode. --check and --update each accept an optional --har PATH that writes the HAR from the same subprocess invocation. Plain --har is still supported for local use. - tests: add tests/script/test_check_import_time.py covering HAR parsing, root lookup, offender ranking/dedup, budget round-trip, and the three --check exit paths (pass, regression, missing budget) plus the new --check --har combined write. Add requirements_test.txt case to the should_run_import_time parametrized test.
This commit is contained in:
J. Nick Koston
@@ -123,21 +123,17 @@ jobs:
|
||||
with:
|
||||
python-version: ${{ env.DEFAULT_PYTHON }}
|
||||
cache-key: ${{ needs.common.outputs.cache-key }}
|
||||
- name: Check import time against budget
|
||||
- name: Check import time against budget and write waterfall HAR
|
||||
run: |
|
||||
. venv/bin/activate
|
||||
script/check_import_time.py --check
|
||||
- name: Generate waterfall HAR
|
||||
if: always()
|
||||
run: |
|
||||
. venv/bin/activate
|
||||
script/check_import_time.py --har importtime.har
|
||||
script/check_import_time.py --check --har importtime.har
|
||||
- name: Upload waterfall HAR
|
||||
if: always()
|
||||
uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1
|
||||
with:
|
||||
name: import-time-waterfall
|
||||
path: importtime.har
|
||||
if-no-files-found: ignore
|
||||
retention-days: 14
|
||||
|
||||
pytest:
|
||||
|
||||
+26
-14
@@ -42,9 +42,16 @@ def run_waterfall(module: str) -> str:
|
||||
return result.stdout
|
||||
|
||||
|
||||
def measure(module: str) -> dict[str, Any]:
|
||||
"""Return the parsed HAR for importing `module`."""
|
||||
return json.loads(run_waterfall(module))
|
||||
def measure(module: str, har_path: Path | None = None) -> dict[str, Any]:
|
||||
"""Return the parsed HAR for importing `module`.
|
||||
|
||||
When `har_path` is given, also write the raw HAR JSON to that path so
|
||||
callers can combine `--check` with `--har` without measuring twice.
|
||||
"""
|
||||
har_text = run_waterfall(module)
|
||||
if har_path is not None:
|
||||
har_path.write_text(har_text)
|
||||
return json.loads(har_text)
|
||||
|
||||
|
||||
def _entries(har: dict[str, Any]) -> list[dict[str, Any]]:
|
||||
@@ -135,7 +142,7 @@ def cmd_check(args: argparse.Namespace) -> int:
|
||||
)
|
||||
return 2
|
||||
|
||||
har = measure(TARGET_MODULE)
|
||||
har = measure(TARGET_MODULE, har_path=Path(args.har) if args.har else None)
|
||||
measured = root_cumulative_us(har, TARGET_MODULE)
|
||||
|
||||
baseline = budget["cumulative_us"]
|
||||
@@ -175,7 +182,7 @@ def cmd_check(args: argparse.Namespace) -> int:
|
||||
|
||||
|
||||
def cmd_update(args: argparse.Namespace) -> int:
|
||||
har = measure(TARGET_MODULE)
|
||||
har = measure(TARGET_MODULE, har_path=Path(args.har) if args.har else None)
|
||||
measured = root_cumulative_us(har, TARGET_MODULE)
|
||||
write_budget(measured, args.margin_pct)
|
||||
print(
|
||||
@@ -186,7 +193,7 @@ def cmd_update(args: argparse.Namespace) -> int:
|
||||
return 0
|
||||
|
||||
|
||||
def cmd_har(args: argparse.Namespace) -> int:
|
||||
def cmd_har_only(args: argparse.Namespace) -> int:
|
||||
Path(args.har).write_text(run_waterfall(TARGET_MODULE))
|
||||
print(f"Wrote waterfall HAR to {args.har}")
|
||||
return 0
|
||||
@@ -200,7 +207,16 @@ def main() -> int:
|
||||
default=DEFAULT_MARGIN_PCT,
|
||||
help=(f"Margin over baseline for --update (default: {DEFAULT_MARGIN_PCT}%%)."),
|
||||
)
|
||||
mode = parser.add_mutually_exclusive_group(required=True)
|
||||
parser.add_argument(
|
||||
"--har",
|
||||
metavar="PATH",
|
||||
help=(
|
||||
"Write a waterfall HAR file at PATH. Can be combined with "
|
||||
"--check or --update to reuse that run's measurement (avoids "
|
||||
"measuring twice)."
|
||||
),
|
||||
)
|
||||
mode = parser.add_mutually_exclusive_group()
|
||||
mode.add_argument(
|
||||
"--check", action="store_true", help="Fail if measured time exceeds budget."
|
||||
)
|
||||
@@ -209,11 +225,6 @@ def main() -> int:
|
||||
action="store_true",
|
||||
help="Rewrite the budget from a fresh measurement.",
|
||||
)
|
||||
mode.add_argument(
|
||||
"--har",
|
||||
metavar="PATH",
|
||||
help="Write a waterfall HAR file via `importtime_waterfall --har`.",
|
||||
)
|
||||
args = parser.parse_args()
|
||||
|
||||
if args.check:
|
||||
@@ -221,8 +232,9 @@ def main() -> int:
|
||||
if args.update:
|
||||
return cmd_update(args)
|
||||
if args.har:
|
||||
return cmd_har(args)
|
||||
return 2
|
||||
return cmd_har_only(args)
|
||||
parser.error("Specify at least one of --check, --update, or --har PATH.")
|
||||
return 2 # unreachable; parser.error exits. Here to satisfy ruff RET503.
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
|
||||
@@ -356,6 +356,7 @@ IMPORT_TIME_TRIGGER_FILES = frozenset(
|
||||
{
|
||||
"requirements.txt",
|
||||
"requirements_dev.txt",
|
||||
"requirements_test.txt",
|
||||
"pyproject.toml",
|
||||
"script/check_import_time.py",
|
||||
"script/import_time_budget.json",
|
||||
|
||||
@@ -0,0 +1,191 @@
|
||||
"""Unit tests for script/check_import_time.py."""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import importlib.util
|
||||
import json
|
||||
import os
|
||||
from pathlib import Path
|
||||
import sys
|
||||
from unittest.mock import patch
|
||||
|
||||
import pytest
|
||||
|
||||
# Load the script-under-test as `check_import_time` (it's a hyphenated path
|
||||
# inside `script/` that mirrors the existing `determine_jobs` pattern).
|
||||
script_dir = os.path.abspath(
|
||||
os.path.join(os.path.dirname(__file__), "..", "..", "script")
|
||||
)
|
||||
sys.path.insert(0, script_dir)
|
||||
spec = importlib.util.spec_from_file_location(
|
||||
"check_import_time", os.path.join(script_dir, "check_import_time.py")
|
||||
)
|
||||
check_import_time = importlib.util.module_from_spec(spec)
|
||||
spec.loader.exec_module(check_import_time)
|
||||
|
||||
|
||||
def _entry(name: str, self_us: int, cumulative_us: int) -> dict:
|
||||
"""Build a minimal HAR entry matching `importtime_waterfall --har`."""
|
||||
return {
|
||||
"request": {"url": name},
|
||||
"time": cumulative_us,
|
||||
"timings": {"receive": self_us, "wait": cumulative_us - self_us},
|
||||
}
|
||||
|
||||
|
||||
def _har(*entries: dict) -> dict:
|
||||
return {"log": {"entries": list(entries)}}
|
||||
|
||||
|
||||
def test_root_cumulative_us_returns_time_for_root_module() -> None:
|
||||
har = _har(
|
||||
_entry("dep_a", 500, 500),
|
||||
_entry("dep_b", 300, 300),
|
||||
_entry("esphome.__main__", 100, 1000),
|
||||
)
|
||||
assert check_import_time.root_cumulative_us(har, "esphome.__main__") == 1000
|
||||
|
||||
|
||||
def test_root_cumulative_us_missing_module_raises() -> None:
|
||||
har = _har(_entry("something.else", 100, 100))
|
||||
with pytest.raises(RuntimeError, match="No HAR entry for 'esphome.__main__'"):
|
||||
check_import_time.root_cumulative_us(har, "esphome.__main__")
|
||||
|
||||
|
||||
def test_top_offenders_ranks_by_self_time_descending() -> None:
|
||||
har = _har(
|
||||
_entry("small", 100, 100),
|
||||
_entry("big", 5000, 5000),
|
||||
_entry("medium", 2000, 2500),
|
||||
)
|
||||
result = check_import_time.top_offenders(har, n=10)
|
||||
assert [name for name, _, _ in result] == ["big", "medium", "small"]
|
||||
assert result[0] == ("big", 5000, 5000)
|
||||
|
||||
|
||||
def test_top_offenders_respects_n_limit() -> None:
|
||||
har = _har(*[_entry(f"m{i}", i * 100, i * 100) for i in range(1, 20)])
|
||||
assert len(check_import_time.top_offenders(har, n=5)) == 5
|
||||
|
||||
|
||||
def test_top_offenders_dedupes_repeat_names_keeping_first() -> None:
|
||||
har = _har(
|
||||
_entry("pkg", 5000, 5000),
|
||||
_entry("pkg", 100, 100), # reimport later in trace
|
||||
_entry("other", 1000, 1000),
|
||||
)
|
||||
result = check_import_time.top_offenders(har, n=10)
|
||||
assert [name for name, _, _ in result] == ["pkg", "other"]
|
||||
# First occurrence wins
|
||||
assert ("pkg", 5000, 5000) in result
|
||||
|
||||
|
||||
def test_format_us_switches_to_ms_at_threshold() -> None:
|
||||
assert check_import_time._format_us(500) == "500us"
|
||||
assert check_import_time._format_us(999) == "999us"
|
||||
assert check_import_time._format_us(1000) == "1.0ms"
|
||||
assert check_import_time._format_us(12345) == "12.3ms"
|
||||
|
||||
|
||||
def test_read_write_budget_roundtrip(tmp_path: Path) -> None:
|
||||
budget_path = tmp_path / "budget.json"
|
||||
with patch.object(check_import_time, "BUDGET_PATH", budget_path):
|
||||
assert check_import_time.read_budget() == {}
|
||||
check_import_time.write_budget(cumulative_us=12345, margin_pct=20)
|
||||
loaded = check_import_time.read_budget()
|
||||
assert loaded["cumulative_us"] == 12345
|
||||
assert loaded["margin_pct"] == 20
|
||||
assert loaded["target_module"] == check_import_time.TARGET_MODULE
|
||||
|
||||
|
||||
def test_cmd_check_passes_when_measured_within_ceiling(
|
||||
tmp_path: Path, capsys: pytest.CaptureFixture[str]
|
||||
) -> None:
|
||||
budget_path = tmp_path / "budget.json"
|
||||
budget_path.write_text(
|
||||
json.dumps(
|
||||
{
|
||||
"target_module": check_import_time.TARGET_MODULE,
|
||||
"margin_pct": 15,
|
||||
"cumulative_us": 100000, # 100ms
|
||||
}
|
||||
)
|
||||
)
|
||||
# Measured 90ms: inside 100ms + 15% = 115ms ceiling
|
||||
har = _har(_entry(check_import_time.TARGET_MODULE, 1000, 90000))
|
||||
args = type("A", (), {"har": None})()
|
||||
with (
|
||||
patch.object(check_import_time, "BUDGET_PATH", budget_path),
|
||||
patch.object(check_import_time, "measure", return_value=har),
|
||||
):
|
||||
rc = check_import_time.cmd_check(args)
|
||||
assert rc == 0
|
||||
out = capsys.readouterr().out
|
||||
assert "measured esphome.__main__:" in out
|
||||
assert "budget 100.0ms" in out
|
||||
|
||||
|
||||
def test_cmd_check_fails_when_measured_exceeds_ceiling(
|
||||
tmp_path: Path, capsys: pytest.CaptureFixture[str]
|
||||
) -> None:
|
||||
budget_path = tmp_path / "budget.json"
|
||||
budget_path.write_text(
|
||||
json.dumps(
|
||||
{
|
||||
"target_module": check_import_time.TARGET_MODULE,
|
||||
"margin_pct": 15,
|
||||
"cumulative_us": 100000,
|
||||
}
|
||||
)
|
||||
)
|
||||
# Measured 120ms: over 100ms + 15% = 115ms ceiling
|
||||
har = _har(
|
||||
_entry("offender_a", 10000, 10000),
|
||||
_entry(check_import_time.TARGET_MODULE, 1000, 120000),
|
||||
)
|
||||
args = type("A", (), {"har": None})()
|
||||
with (
|
||||
patch.object(check_import_time, "BUDGET_PATH", budget_path),
|
||||
patch.object(check_import_time, "measure", return_value=har),
|
||||
):
|
||||
rc = check_import_time.cmd_check(args)
|
||||
assert rc == 1
|
||||
err = capsys.readouterr().err
|
||||
assert "REGRESSION" in err
|
||||
assert "120.0ms" in err
|
||||
assert "offender_a" in err # top offender table
|
||||
|
||||
|
||||
def test_cmd_check_returns_2_when_budget_missing(
|
||||
tmp_path: Path, capsys: pytest.CaptureFixture[str]
|
||||
) -> None:
|
||||
budget_path = tmp_path / "nonexistent.json"
|
||||
args = type("A", (), {"har": None})()
|
||||
with patch.object(check_import_time, "BUDGET_PATH", budget_path):
|
||||
rc = check_import_time.cmd_check(args)
|
||||
assert rc == 2
|
||||
assert "missing" in capsys.readouterr().err
|
||||
|
||||
|
||||
def test_cmd_check_writes_har_when_path_given(tmp_path: Path) -> None:
|
||||
budget_path = tmp_path / "budget.json"
|
||||
budget_path.write_text(
|
||||
json.dumps(
|
||||
{
|
||||
"target_module": check_import_time.TARGET_MODULE,
|
||||
"margin_pct": 15,
|
||||
"cumulative_us": 100000,
|
||||
}
|
||||
)
|
||||
)
|
||||
har_path = tmp_path / "out.har"
|
||||
har_text = json.dumps(_har(_entry(check_import_time.TARGET_MODULE, 1000, 80000)))
|
||||
args = type("A", (), {"har": str(har_path)})()
|
||||
with (
|
||||
patch.object(check_import_time, "BUDGET_PATH", budget_path),
|
||||
patch.object(check_import_time, "run_waterfall", return_value=har_text),
|
||||
):
|
||||
rc = check_import_time.cmd_check(args)
|
||||
assert rc == 0
|
||||
assert har_path.exists()
|
||||
assert json.loads(har_path.read_text()) == json.loads(har_text)
|
||||
@@ -625,6 +625,7 @@ def test_should_run_python_linters_with_branch() -> None:
|
||||
# Dependency declarations and the check's own files trigger
|
||||
(["requirements.txt"], True),
|
||||
(["requirements_dev.txt"], True),
|
||||
(["requirements_test.txt"], True),
|
||||
(["pyproject.toml"], True),
|
||||
(["script/check_import_time.py"], True),
|
||||
(["script/import_time_budget.json"], True),
|
||||
|
||||
Reference in New Issue
Block a user