From 99c80b7dfa5858110cbb1ee91da4aecf7b29ce3e Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Thu, 23 Apr 2026 14:47:03 -0500 Subject: [PATCH] [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 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. --- .github/workflows/ci.yml | 10 +- script/check_import_time.py | 40 ++++-- script/determine-jobs.py | 1 + tests/script/test_check_import_time.py | 191 +++++++++++++++++++++++++ tests/script/test_determine_jobs.py | 1 + 5 files changed, 222 insertions(+), 21 deletions(-) create mode 100644 tests/script/test_check_import_time.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 49767de0b75..d60bd6edc31 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -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: diff --git a/script/check_import_time.py b/script/check_import_time.py index 1c25a3cdd68..0d5362c9681 100755 --- a/script/check_import_time.py +++ b/script/check_import_time.py @@ -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__": diff --git a/script/determine-jobs.py b/script/determine-jobs.py index 6c2ab958263..1842aba3a02 100755 --- a/script/determine-jobs.py +++ b/script/determine-jobs.py @@ -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", diff --git a/tests/script/test_check_import_time.py b/tests/script/test_check_import_time.py new file mode 100644 index 00000000000..223c58002c9 --- /dev/null +++ b/tests/script/test_check_import_time.py @@ -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) diff --git a/tests/script/test_determine_jobs.py b/tests/script/test_determine_jobs.py index bef66254566..d44fecb95d7 100644 --- a/tests/script/test_determine_jobs.py +++ b/tests/script/test_determine_jobs.py @@ -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),