From 7eddf429ea3810f4e1b019b13c20f768de936411 Mon Sep 17 00:00:00 2001 From: Javier Peletier Date: Tue, 24 Mar 2026 10:57:22 +0100 Subject: [PATCH] [substitutions] speed up config loading: substitutions pass and `!include` redesign (package refactor part 4) (#12126) Co-authored-by: J. Nick Koston --- esphome/components/packages/__init__.py | 406 ++++++++++++------ esphome/config.py | 6 +- tests/component_tests/packages/test_init.py | 4 +- .../component_tests/packages/test_packages.py | 178 +++++++- .../06-remote_packages.approved.yaml | 21 +- .../06-remote_packages.input.yaml | 22 +- .../07-package_merging.approved.yaml | 2 - .../08-include_hierarchy.approved.yaml | 49 +++ .../08-include_hierarchy.input.yaml | 16 + .../10-dynamic_packages.approved.yaml | 69 +++ .../10-dynamic_packages.input.yaml | 62 +++ .../substitutions/level1_package.yaml | 21 + .../substitutions/level2_package.yaml | 21 + .../substitutions/level3_package.yaml | 16 + .../fixtures/substitutions/package2.yaml | 10 + tests/unit_tests/test_substitutions.py | 4 +- tests/unit_tests/test_yaml_util.py | 42 +- 17 files changed, 781 insertions(+), 168 deletions(-) create mode 100644 tests/unit_tests/fixtures/substitutions/08-include_hierarchy.approved.yaml create mode 100644 tests/unit_tests/fixtures/substitutions/08-include_hierarchy.input.yaml create mode 100644 tests/unit_tests/fixtures/substitutions/10-dynamic_packages.approved.yaml create mode 100644 tests/unit_tests/fixtures/substitutions/10-dynamic_packages.input.yaml create mode 100644 tests/unit_tests/fixtures/substitutions/level1_package.yaml create mode 100644 tests/unit_tests/fixtures/substitutions/level2_package.yaml create mode 100644 tests/unit_tests/fixtures/substitutions/level3_package.yaml create mode 100644 tests/unit_tests/fixtures/substitutions/package2.yaml diff --git a/esphome/components/packages/__init__.py b/esphome/components/packages/__init__.py index f9bdb677a7..1a6df84fe0 100644 --- a/esphome/components/packages/__init__.py +++ b/esphome/components/packages/__init__.py @@ -6,6 +6,7 @@ from pathlib import Path from typing import Any from esphome import git, yaml_util +from esphome.components.substitutions import ContextVars, push_context, substitute from esphome.components.substitutions.jinja import has_jinja from esphome.config_helpers import Remove, merge_config import esphome.config_validation as cv @@ -32,43 +33,44 @@ _LOGGER = logging.getLogger(__name__) DOMAIN = CONF_PACKAGES -def validate_has_jinja(value: Any): - if not isinstance(value, str) or not has_jinja(value): - raise cv.Invalid("string does not contain Jinja syntax") - return value +def is_remote_package(package_config: dict) -> bool: + """Returns True if the package_config is a remote package definition.""" + return CONF_URL in package_config -def valid_package_contents(allow_jinja: bool = True) -> Callable[[Any], dict]: - """Returns a validator that checks if a package_config that will be merged looks as - much as possible to a valid config to fail early on obvious mistakes.""" +def valid_package_contents(package_config: dict) -> dict: + """Validate that a package looks like a plausible ESPHome config fragment. - def validator(package_config: dict) -> dict: - if isinstance(package_config, dict): - if CONF_URL in package_config: - # If a URL key is found, then make sure the config conforms to a remote package schema: - return REMOTE_PACKAGE_SCHEMA(package_config) - - # Validate manually since Voluptuous would regenerate dicts and lose metadata - # such as ESPHomeDataBase - for k, v in package_config.items(): - if not isinstance(k, str): - raise cv.Invalid("Package content keys must be strings") - if isinstance(v, (dict, list, Remove)): - continue # e.g. script: [], psram: !remove, logger: {level: debug} - if v is None: - continue # e.g. web_server: - if allow_jinja and isinstance(v, str) and has_jinja(v): - # e.g: remote package shorthand: - # package_name: github://esphome/repo/file.yaml@${ branch }, or: - # switch: ${ expression that evals to a switch } - continue - - raise cv.Invalid("Invalid component content in package definition") - return package_config + Rejects non-dict values, remote package schemas (which should have been + handled earlier), non-string keys, and scalar values that aren't Jinja + expressions. This is a lightweight check to catch obvious mistakes before + full component validation runs later. + """ + if not isinstance(package_config, dict): raise cv.Invalid("Package contents must be a dict") - return validator + if is_remote_package(package_config): + # Package contents must not contain a root `url:` key + raise cv.Invalid("Remote package schema not expected here") + + # Validate manually since Voluptuous would regenerate dicts and lose metadata + # such as ESPHomeDataBase + for k, v in package_config.items(): + if not isinstance(k, str): + raise cv.Invalid("Package content keys must be strings") + if isinstance(v, (dict, list, Remove)): + continue # e.g. script: [], psram: !remove, logger: {level: debug} + if v is None: + continue # e.g. web_server: + if isinstance(v, str) and has_jinja(v): + # e.g: remote package shorthand: + # package_name: github://esphome/repo/file.yaml@${ branch }, or: + # switch: ${ expression that evals to a switch } + continue + + raise cv.Invalid("Invalid component content in package definition") + return package_config def expand_file_to_files(config: dict): @@ -105,7 +107,7 @@ def validate_source_shorthand(value): return REMOTE_PACKAGE_SCHEMA(conf) -def deprecate_single_package(config): +def deprecate_single_package(config: dict) -> dict: _LOGGER.warning( """ Including a single package under `packages:`, i.e., `packages: !include mypackage.yaml` is deprecated. @@ -158,10 +160,7 @@ REMOTE_PACKAGE_SCHEMA = cv.All( PACKAGE_SCHEMA = cv.Any( # A package definition is either: validate_source_shorthand, # A git URL shorthand string that expands to a remote package schema, or REMOTE_PACKAGE_SCHEMA, # a valid remote package schema, or - validate_has_jinja, # a Jinja string that may resolve to a package, or - valid_package_contents( - allow_jinja=True - ), # Something that at least looks like an actual package, e.g. {wifi:{ssid: xxx}} + valid_package_contents, # Something that at least looks like an actual package, e.g. {wifi:{ssid: xxx}} # which will have to be fully validated later as per each component's schema. ) @@ -179,7 +178,15 @@ CONFIG_SCHEMA = cv.Any( # under `packages:` we can have either: def _process_remote_package(config: dict, skip_update: bool = False) -> dict: - # When skip_update is True, use NEVER_REFRESH to prevent updates + """Clone/update a git repo and load the YAML files listed in the package definition. + + Returns ``{"packages": {: , ...}}`` so the caller + can recurse into the loaded packages. Each loaded YAML node is tagged + with any ``vars:`` from the file entry via :func:`yaml_util.add_context`. + + If loading fails after cloning, attempts a revert and retry in case + a prior cached checkout is stale. + """ actual_refresh = git.NEVER_REFRESH if skip_update else config[CONF_REFRESH] repo_dir, revert = git.clone_or_update( url=config[CONF_URL], @@ -189,7 +196,7 @@ def _process_remote_package(config: dict, skip_update: bool = False) -> dict: username=config.get(CONF_USERNAME), password=config.get(CONF_PASSWORD), ) - files = [] + files: list[dict[str, Any]] = [] if base_path := config.get(CONF_PATH): repo_dir = repo_dir / base_path @@ -200,126 +207,255 @@ def _process_remote_package(config: dict, skip_update: bool = False) -> dict: else: files.append(file) - def get_packages(files) -> dict: - packages = {} + def _load_package_yaml(yaml_file: Path, filename: str) -> dict: + """Load a YAML file from a remote package, validating min_version.""" + try: + new_yaml = yaml_util.load_yaml(yaml_file) + except EsphomeError as e: + raise cv.Invalid( + f"{filename} is not a valid YAML file." + f" Please check the file contents.\n{e}" + ) from e + esphome_config = new_yaml.get(CONF_ESPHOME) or {} + min_version = esphome_config.get(CONF_MIN_VERSION) + if min_version is not None and cv.Version.parse(min_version) > cv.Version.parse( + ESPHOME_VERSION + ): + raise cv.Invalid( + f"Current ESPHome Version is too old to use" + f" this package: {ESPHOME_VERSION} < {min_version}" + ) + return new_yaml + + def get_packages(files: list[dict[str, Any]]) -> dict: + packages: dict[str, Any] = {} for idx, file in enumerate(files): filename = file[CONF_PATH] yaml_file: Path = repo_dir / filename - vars = file.get(CONF_VARS, {}) - if not yaml_file.is_file(): raise cv.Invalid( f"{filename} does not exist in repository", path=[CONF_FILES, idx, CONF_PATH], ) - - try: - new_yaml = yaml_util.load_yaml(yaml_file) - if ( - CONF_ESPHOME in new_yaml - and CONF_MIN_VERSION in new_yaml[CONF_ESPHOME] - ): - min_version = new_yaml[CONF_ESPHOME][CONF_MIN_VERSION] - if cv.Version.parse(min_version) > cv.Version.parse( - ESPHOME_VERSION - ): - raise cv.Invalid( - f"Current ESPHome Version is too old to use this package: {ESPHOME_VERSION} < {min_version}" - ) - new_yaml = yaml_util.add_context(new_yaml, vars or None) - packages[f"{filename}{idx}"] = new_yaml - except EsphomeError as e: - raise cv.Invalid( - f"{filename} is not a valid YAML file. Please check the file contents.\n{e}" - ) from e + new_yaml = _load_package_yaml(yaml_file, filename) + new_yaml = yaml_util.add_context(new_yaml, file.get(CONF_VARS)) + packages[f"{filename}{idx}"] = new_yaml return packages - packages = None - error = "" - - try: - packages = get_packages(files) - except cv.Invalid as e: - error = e + if revert is not None: + # If loading fails, the cached checkout may be stale — revert and retry once. try: - if revert is not None: - revert() - packages = get_packages(files) - except cv.Invalid as er: - error = er + return {CONF_PACKAGES: get_packages(files)} + except cv.Invalid: + revert() + try: + return {CONF_PACKAGES: get_packages(files)} + except cv.Invalid as err: + raise cv.Invalid(f"Failed to load packages. {err}", path=err.path) from err - if packages is None: - raise cv.Invalid(f"Failed to load packages. {error}", path=error.path) + return {CONF_PACKAGES: get_packages(files)} - return {"packages": packages} + +def _walk_package_dict( + packages: dict, + callback: Callable[[dict, ContextVars | None], dict], + context: ContextVars | None, +) -> cv.Invalid | None: + """Iterate a packages dict in reverse priority order, invoking callback on each entry. + + Returns ``None`` on success, or the first :class:`cv.Invalid` error if a callback fails. + """ + for package_name, package_config in reversed(packages.items()): + with cv.prepend_path(package_name): + try: + packages[package_name] = callback(package_config, context) + except cv.Invalid as err: + return err + return None + + +def _walk_package_list( + packages: list, + callback: Callable[[dict, ContextVars | None], dict], + context: ContextVars | None, +) -> None: + """Iterate a packages list in reverse priority order, invoking callback on each entry.""" + for idx in reversed(range(len(packages))): + with cv.prepend_path(idx): + packages[idx] = callback(packages[idx], context) def _walk_packages( - config: dict, callback: Callable[[dict], dict], validate_deprecated: bool = True + config: dict, + callback: Callable[[dict, ContextVars | None], dict], + context: ContextVars | None = None, + validate_deprecated: bool = True, ) -> dict: + """Walks the packages structure in priority order, invoking ``callback`` on each package definition found. + + This function only iterates over the immediate ``packages:`` entries in *config*. + If packages may contain nested ``packages:`` keys, the *callback* is responsible + for recursing by calling ``_walk_packages`` on the returned package config. + """ if CONF_PACKAGES not in config: return config packages = config[CONF_PACKAGES] - # The following block and `validate_deprecated` parameter can be safely removed - # once single-package deprecation is effective - if validate_deprecated: - packages = CONFIG_SCHEMA(packages) + if not isinstance(packages, (dict, list)): + raise cv.Invalid( + f"Packages must be a key to value mapping or list, got {type(packages)} instead" + ) with cv.prepend_path(CONF_PACKAGES): - if isinstance(packages, dict): - for package_name, package_config in reversed(packages.items()): - with cv.prepend_path(package_name): - package_config = callback(package_config) - packages[package_name] = _walk_packages(package_config, callback) - elif isinstance(packages, list): - for idx in reversed(range(len(packages))): - with cv.prepend_path(idx): - package_config = callback(packages[idx]) - packages[idx] = _walk_packages(package_config, callback) - else: - raise cv.Invalid( - f"Packages must be a key to value mapping or list, got {type(packages)} instead" - ) + if not isinstance(packages, dict): + _walk_package_list(packages, callback, context) + elif (result := _walk_package_dict(packages, callback, context)) is not None: + if not validate_deprecated: + raise result + # Fallback: treat the dict as a single deprecated package. + # Note: this catches *any* cv.Invalid from the callback, which may + # mask real validation errors in named package dicts. + # This block can be removed once the single-package + # deprecation period (2026.7.0) is over. + config[CONF_PACKAGES] = [packages] + return _walk_packages(deprecate_single_package(config), callback, context) + config[CONF_PACKAGES] = packages return config -def do_packages_pass(config: dict, skip_update: bool = False) -> dict: - """Processes, downloads and validates all packages in the config. - Also extracts and merges all substitutions found in packages into the main config substitutions. +def _substitute_package_definition( + package_config: dict | str, context_vars: ContextVars | None +) -> dict | str: + """Substitute variables in a package definition string or remote package dict. + + Only substitutes strings and remote package dicts (URLs, refs, paths). + Local package contents are left untouched — they will be substituted + later during the main substitution pass. + """ + if isinstance(package_config, str) or ( + isinstance(package_config, dict) and is_remote_package(package_config) + ): + package_config = substitute( + item=package_config, + path=[], + parent_context=context_vars or ContextVars(), + strict_undefined=False, + ) + return package_config + + +def _update_substitutions_context( + parent_context: UserDict, + package_substitutions: dict[str, Any], +) -> None: + """Resolve and add new substitutions to the parent context. + + Skips keys already present (higher-priority sources win). + String values are substituted against the current context so that + cross-references between substitutions are expanded when possible. + """ + for key, value in package_substitutions.items(): + if key in parent_context: + continue + if not isinstance(value, str): + parent_context[key] = value + continue + parent_context[key] = substitute( + item=value, + path=[CONF_SUBSTITUTIONS, key], + parent_context=ContextVars(parent_context), + strict_undefined=False, + ) + + +class _PackageProcessor: + """Stateful processor that resolves packages and collects substitutions. + + Packages are processed highest-priority first (later-declared before + earlier-declared) so that their substitutions are available when + resolving lower-priority package definitions. For each entry: + + 1. Substitute variables in remote package definitions (URLs, refs, paths). + 2. Validate against ``PACKAGE_SCHEMA`` and download remote packages. + 3. Extract ``substitutions:`` and merge into the shared context + (higher-priority packages win on conflicts). + 4. Recurse into any nested ``packages:`` keys. + + Command-line substitutions take the highest priority and are never overridden. + """ + + def __init__( + self, + substitutions: UserDict, + command_line_substitutions: dict[str, Any] | None, + skip_update: bool, + ) -> None: + self.substitutions = substitutions + self.parent_context = UserDict(command_line_substitutions or {}) + self.skip_update = skip_update + + def resolve_package( + self, package_config: dict | str, context_vars: ContextVars | None + ) -> dict: + """Substitute variables in the definition and fetch remote packages. + + The input may be a ``str`` (git shorthand or Jinja expression) or a + ``dict`` (remote or local package). After ``PACKAGE_SCHEMA`` validation + the result is always a ``dict``. + """ + package_config = _substitute_package_definition(package_config, context_vars) + package_config = PACKAGE_SCHEMA(package_config) + if is_remote_package(package_config): + package_config = _process_remote_package(package_config, self.skip_update) + return package_config + + def collect_substitutions(self, package_config: dict) -> None: + """Extract substitutions from a package and merge into the shared context.""" + if subs := package_config.pop(CONF_SUBSTITUTIONS, {}): + self.substitutions.data = merge_config(subs, self.substitutions.data) + _update_substitutions_context(self.parent_context, subs) + + def process_package( + self, package_config: dict | str, context_vars: ContextVars | None + ) -> dict: + """Resolve a single package and recurse into any nested packages.""" + package_config = self.resolve_package(package_config, context_vars) + self.collect_substitutions(package_config) + + if CONF_PACKAGES not in package_config: + return package_config + + # Push context from !include vars on the package root and on the packages key + context_vars = push_context(package_config, context_vars) + context_vars = push_context(package_config[CONF_PACKAGES], context_vars) + return _walk_packages(package_config, self.process_package, context_vars) + + +def do_packages_pass( + config: dict, + *, + command_line_substitutions: dict[str, Any] | None = None, + skip_update: bool = False, +) -> dict: + """Load, validate, and flatten all packages in the config. + + Returns the config with all packages loaded in-place (but not yet merged) + and a consolidated ``substitutions:`` block restored at the front. """ if CONF_PACKAGES not in config: return config substitutions = UserDict(config.pop(CONF_SUBSTITUTIONS, {})) + processor = _PackageProcessor( + substitutions, command_line_substitutions, skip_update + ) + _update_substitutions_context(processor.parent_context, substitutions) - def process_package_callback(package_config: dict) -> dict: - """This will be called for each package found in the config.""" - if isinstance(package_config, yaml_util.ConfigContext): - context_vars = package_config.vars - if CONF_PACKAGES in package_config or CONF_URL in package_config: - # Remote package definition: eagerly resolve before PACKAGE_SCHEMA validation. - from esphome.components.substitutions import ContextVars, substitute - - package_config = substitute( - package_config, - [], - ContextVars(context_vars), - strict_undefined=False, - ) - package_config = PACKAGE_SCHEMA(package_config) - if isinstance(package_config, str): - return package_config # Jinja string, skip processing - if CONF_URL in package_config: - package_config = _process_remote_package(package_config, skip_update) - # Extract substitutions from the package and merge them into the main substitutions: - substitutions.data = merge_config( - package_config.pop(CONF_SUBSTITUTIONS, {}), substitutions.data - ) - return package_config - - _walk_packages(config, process_package_callback) + context_vars = push_context( + config[CONF_PACKAGES], ContextVars(processor.parent_context) + ) + _walk_packages(config, processor.process_package, context_vars) if substitutions: config[CONF_SUBSTITUTIONS] = substitutions.data @@ -328,19 +464,27 @@ def do_packages_pass(config: dict, skip_update: bool = False) -> dict: def merge_packages(config: dict) -> dict: - """Merges all packages into the main config and removes the `packages:` key.""" + """Flatten the ``packages:`` tree into the main config. + + Collects every package (including nested ones) into a flat list in + priority order, then merges them into *config* using :func:`merge_config`. + Higher-priority packages (declared later) override lower-priority ones. + + The ``packages:`` key is removed from the returned config. + Must be called after :func:`do_packages_pass` has resolved all packages. + """ if CONF_PACKAGES not in config: return config # Build flat list of all package configs to merge in priority order: merge_list: list[dict] = [] - validate_package = valid_package_contents(allow_jinja=False) - - def process_package_callback(package_config: dict) -> dict: + def process_package_callback( + package_config: dict, context: ContextVars | None + ) -> dict: """This will be called for each package found in the config.""" - merge_list.append(validate_package(package_config)) - return package_config + merge_list.append(package_config) + return _walk_packages(package_config, process_package_callback) _walk_packages(config, process_package_callback, validate_deprecated=False) # Merge all packages into the main config: diff --git a/esphome/config.py b/esphome/config.py index b80aaf3700..7a6feea3d3 100644 --- a/esphome/config.py +++ b/esphome/config.py @@ -989,7 +989,11 @@ def validate_config( result.add_output_path([CONF_PACKAGES], CONF_PACKAGES) try: - config = do_packages_pass(config, skip_update=skip_external_update) + config = do_packages_pass( + config, + command_line_substitutions=command_line_substitutions, + skip_update=skip_external_update, + ) except vol.Invalid as err: result.update(config) result.add_error(err) diff --git a/tests/component_tests/packages/test_init.py b/tests/component_tests/packages/test_init.py index 779244e2ed..fd30c2433f 100644 --- a/tests/component_tests/packages/test_init.py +++ b/tests/component_tests/packages/test_init.py @@ -69,7 +69,7 @@ def test_packages_skip_update_false( } # Call with skip_update=False (default) - do_packages_pass(config, skip_update=False) + do_packages_pass(config, command_line_substitutions={}, skip_update=False) # Verify clone_or_update was called with actual refresh value mock_clone_or_update.assert_called_once() @@ -104,7 +104,7 @@ def test_packages_default_no_skip( } # Call without skip_update parameter - do_packages_pass(config) + do_packages_pass(config, command_line_substitutions={}) # Verify clone_or_update was called with actual refresh value mock_clone_or_update.assert_called_once() diff --git a/tests/component_tests/packages/test_packages.py b/tests/component_tests/packages/test_packages.py index 60dc0dccda..0893c7dcbb 100644 --- a/tests/component_tests/packages/test_packages.py +++ b/tests/component_tests/packages/test_packages.py @@ -37,6 +37,7 @@ from esphome.const import ( ) from esphome.core import CORE from esphome.util import OrderedDict +from esphome.yaml_util import add_context # Test strings TEST_DEVICE_NAME = "test_device_name" @@ -70,7 +71,7 @@ def fixture_basic_esphome(): def packages_pass(config): - """Wrapper around packages_pass that also resolves Extend and Remove.""" + """Passes the config through the packages processing steps.""" config = do_packages_pass(config) config = do_substitution_pass(config) config = merge_packages(config) @@ -705,6 +706,85 @@ def test_remote_packages_with_files_list( assert actual == expected +@patch("esphome.yaml_util.load_yaml") +@patch("pathlib.Path.is_file") +@patch("esphome.git.clone_or_update") +def test_remote_packages_with_files_list_and_substitutions( + mock_clone_or_update, mock_is_file, mock_load_yaml +) -> None: + """ + Ensures that packages are loaded as mixed list of dictionary and strings + """ + # Mock the response from git.clone_or_update + mock_revert = MagicMock() + mock_clone_or_update.return_value = (Path("/tmp/noexists"), mock_revert) + + # Mock the response from pathlib.Path.is_file + mock_is_file.return_value = True + + # Mock the response from esphome.yaml_util.load_yaml + mock_load_yaml.side_effect = [ + OrderedDict( + { + CONF_SENSOR: [ + { + CONF_PLATFORM: TEST_SENSOR_PLATFORM_1, + CONF_NAME: TEST_SENSOR_NAME_1, + } + ] + } + ), + OrderedDict( + { + CONF_SENSOR: [ + { + CONF_PLATFORM: TEST_SENSOR_PLATFORM_1, + CONF_NAME: TEST_SENSOR_NAME_2, + } + ] + } + ), + ] + + # Define the input config + config = { + CONF_PACKAGES: { + "package1": add_context( + { + CONF_URL: r"${url}", + CONF_REF: r"${branch}", + CONF_FILES: [ + {CONF_PATH: r"$file"}, + "sensor2.yaml", + ], + CONF_REFRESH: "1d", + }, + { + "branch": "main", + "file": TEST_YAML_FILENAME, + "url": "https://github.com/esphome/non-existant-repo", + }, + ) + } + } + + expected = { + CONF_SENSOR: [ + { + CONF_PLATFORM: TEST_SENSOR_PLATFORM_1, + CONF_NAME: TEST_SENSOR_NAME_1, + }, + { + CONF_PLATFORM: TEST_SENSOR_PLATFORM_1, + CONF_NAME: TEST_SENSOR_NAME_2, + }, + ] + } + + actual = packages_pass(config) + assert actual == expected + + @patch("esphome.yaml_util.load_yaml") @patch("pathlib.Path.is_file") @patch("esphome.git.clone_or_update") @@ -906,7 +986,7 @@ def test_packages_merge_substitutions() -> None: }, } - actual = do_packages_pass(config) + actual = do_packages_pass(config, command_line_substitutions={}) assert actual == expected @@ -970,33 +1050,107 @@ def test_package_merge() -> None: assert actual == expected +def test_packages_invalid_type_raises() -> None: + """Packages that are not a dict or list raise cv.Invalid.""" + config = { + CONF_PACKAGES: "not_a_dict_or_list", + } + with pytest.raises( + cv.Invalid, match="Packages must be a key to value mapping or list" + ): + do_packages_pass(config) + + @pytest.mark.parametrize( "invalid_package", [ 6, "some string", - ["some string"], - None, True, - {"some_component": 8}, - {3: 2}, - {"some_component": r"${unevaluated expression}"}, ], ) -def test_package_merge_invalid(invalid_package) -> None: - """ - Tests that trying to merge an invalid package raises an error. - """ +def test_invalid_package_contents_rejected(invalid_package: object) -> None: + """Invalid package contents are rejected by PACKAGE_SCHEMA during do_packages_pass.""" config = { CONF_PACKAGES: { "some_package": invalid_package, }, } - with pytest.raises(cv.Invalid): + do_packages_pass(config) + + +@pytest.mark.xfail( + reason="Deprecated single-package fallback swallows these errors. " + "Remove xfail when single-package deprecation is removed (2026.7.0).", + strict=True, +) +@pytest.mark.parametrize( + "invalid_package", + [ + None, + ["some string"], + {"some_component": 8}, + {3: 2}, + ], +) +def test_invalid_package_contents_masked_by_deprecation( + invalid_package: object, +) -> None: + """These invalid packages are swallowed by the deprecated single-package fallback.""" + config = { + CONF_PACKAGES: { + "some_package": invalid_package, + }, + } + with pytest.raises(cv.Invalid): + do_packages_pass(config) + + +def test_merge_packages_invalid_nested_type_raises() -> None: + """Invalid nested packages type during merge raises cv.Invalid.""" + config = { + CONF_PACKAGES: { + "pkg": { + CONF_PACKAGES: "invalid", + }, + }, + } + with pytest.raises( + cv.Invalid, match="Packages must be a key to value mapping or list" + ): merge_packages(config) +@patch("esphome.yaml_util.load_yaml") +@patch("pathlib.Path.is_file") +@patch("esphome.git.clone_or_update") +def test_remote_packages_no_revert( + mock_clone_or_update, mock_is_file, mock_load_yaml +) -> None: + """Remote packages with revert=None load without retry logic.""" + mock_clone_or_update.return_value = (Path("/tmp/noexists"), None) + mock_is_file.return_value = True + mock_load_yaml.return_value = OrderedDict( + {CONF_SENSOR: [{CONF_PLATFORM: TEST_SENSOR_PLATFORM_1, CONF_NAME: "test"}]} + ) + + config = { + CONF_PACKAGES: { + "pkg": { + CONF_URL: "https://github.com/esphome/repo", + CONF_REF: "main", + CONF_FILES: [{CONF_PATH: "file.yaml"}], + CONF_REFRESH: "1d", + } + } + } + actual = packages_pass(config) + assert actual[CONF_SENSOR] == [ + {CONF_PLATFORM: TEST_SENSOR_PLATFORM_1, CONF_NAME: "test"} + ] + + def test_raw_config_contains_merged_esphome_from_package(tmp_path) -> None: """Test that CORE.raw_config contains esphome section from merged package. diff --git a/tests/unit_tests/fixtures/substitutions/06-remote_packages.approved.yaml b/tests/unit_tests/fixtures/substitutions/06-remote_packages.approved.yaml index 0fffbfb7cb..300cd85950 100644 --- a/tests/unit_tests/fixtures/substitutions/06-remote_packages.approved.yaml +++ b/tests/unit_tests/fixtures/substitutions/06-remote_packages.approved.yaml @@ -1,7 +1,3 @@ -substitutions: - x: 10 - y: 20 - z: 30 values_from_repo1_main: - package_name: package1 x: 3 @@ -28,3 +24,20 @@ values_from_repo1_main: y: 20 z: 5 volume: 1000 + - package_name: package6 + x: 12 + y: 13 + z: 5 + volume: 780 + - package_name: default + x: 10 + y: 20 + z: 5 + volume: 1000 +substitutions: + x: 10 + y: 20 + z: 30 + my_repo: repo1 + my_file: file1 + my_ref: main diff --git a/tests/unit_tests/fixtures/substitutions/06-remote_packages.input.yaml b/tests/unit_tests/fixtures/substitutions/06-remote_packages.input.yaml index 772860bf19..c61eeab28d 100644 --- a/tests/unit_tests/fixtures/substitutions/06-remote_packages.input.yaml +++ b/tests/unit_tests/fixtures/substitutions/06-remote_packages.input.yaml @@ -2,16 +2,26 @@ substitutions: x: 10 y: 20 z: 30 + my_repo: default_repo + my_file: default_file + my_ref: main + +# The following key is only used by the test framework +# to simulate command line substitutions +command_line_substitutions: + my_repo: repo1 + my_file: file1 + packages: package1: url: https://github.com/esphome/repo1 + ref: main files: - path: file1.yaml vars: package_name: package1 x: 3 y: 4 - ref: main package2: !include # a package that just includes the given remote package file: remote_package_proxy.yaml vars: @@ -41,3 +51,13 @@ packages: repo: repo1 file: file1.yaml ref: main + package6: + url: https://github.com/esphome/${my_repo} + ref: ${my_ref} + files: + - path: ${my_file + ".yaml"} + vars: + package_name: package6 + x: 12 + y: 13 + package7: github://esphome/${my_repo}/${my_file + ".yaml"}@${my_ref} diff --git a/tests/unit_tests/fixtures/substitutions/07-package_merging.approved.yaml b/tests/unit_tests/fixtures/substitutions/07-package_merging.approved.yaml index 867889b7bc..9e62fcae86 100644 --- a/tests/unit_tests/fixtures/substitutions/07-package_merging.approved.yaml +++ b/tests/unit_tests/fixtures/substitutions/07-package_merging.approved.yaml @@ -37,8 +37,6 @@ substitutions: - id: component8 value: 8 fancy_package: - substitutions: - fancy_subst: 42 fancy_component: *id001 pin: 12 some_switches: *id002 diff --git a/tests/unit_tests/fixtures/substitutions/08-include_hierarchy.approved.yaml b/tests/unit_tests/fixtures/substitutions/08-include_hierarchy.approved.yaml new file mode 100644 index 0000000000..fce47b01bf --- /dev/null +++ b/tests/unit_tests/fixtures/substitutions/08-include_hierarchy.approved.yaml @@ -0,0 +1,49 @@ +substitutions: + a: 10 + b: 20 + x: 79 +test_list: + - level1: + a: 10 + b: 20 + c: 10 + d: 20 + e: ${e} + f: ${f} + g: ${g} + h: ${h} + i: ${i} + j: ${j} + x: 80 + y: 40 + level2: + - level2: + a: 10 + b: 20 + c: 10 + d: 20 + e: 20 + f: 40 + g: ${g} + h: ${h} + i: ${i} + j: ${j} + x: 81 + y: 40 + level3: + - level3: + a: 10 + b: 20 + c: 10 + d: 20 + e: 20 + f: 40 + g: 100 + h: 200 + i: 30 + j: ${undefined_variable} + x: 82 + y: 40 + - a: 10 + b: 20 + x: 79 diff --git a/tests/unit_tests/fixtures/substitutions/08-include_hierarchy.input.yaml b/tests/unit_tests/fixtures/substitutions/08-include_hierarchy.input.yaml new file mode 100644 index 0000000000..6997ef56c1 --- /dev/null +++ b/tests/unit_tests/fixtures/substitutions/08-include_hierarchy.input.yaml @@ -0,0 +1,16 @@ +substitutions: + a: 10 + b: 20 + x: 79 + +test_list: + - !include + file: level1_package.yaml + vars: + x: ${x+1} + y: ${d*2} + c: ${a} + d: ${b} + - a: ${a} + b: ${b} + x: ${x} diff --git a/tests/unit_tests/fixtures/substitutions/10-dynamic_packages.approved.yaml b/tests/unit_tests/fixtures/substitutions/10-dynamic_packages.approved.yaml new file mode 100644 index 0000000000..ec2ae711bb --- /dev/null +++ b/tests/unit_tests/fixtures/substitutions/10-dynamic_packages.approved.yaml @@ -0,0 +1,69 @@ +substitutions: + a: from base config + b: from package3 + c: from nested package4 + nested_package: + nested_package_test_list: + - a: from base config + - b: from package3 + - c: from nested package4 + package1: + package1_test_list: + - a: from base config + - b: from package3 + - c: from nested package4 + package2: + package2_test_list: + - a: from package2 vars + - b: from package3 + - c: from nested package4 + package3: + package3_test_list: + - a: from base config + - b: from package3 + - c: from nested package4 + package4: + packages: + - nested_package_test_list: + - a: from base config + - b: from package3 + - c: from nested package4 + package_map: + package1: + package1_test_list: + - a: from base config + - b: from package3 + - c: from nested package4 + package2: + package2_test_list: + - a: from package2 vars + - b: from package3 + - c: from nested package4 + package3: &id001 + package3_test_list: + - a: from base config + - b: from package3 + - c: from nested package4 + selected_package_number: 3 + selected_package_name: package3 + selected_package: *id001 +base_test_list: + - a: from base config + - b: from package3 + - c: from nested package4 +package1_test_list: + - a: from base config + - b: from package3 + - c: from nested package4 +package2_test_list: + - a: from package2 vars + - b: from package3 + - c: from nested package4 +package3_test_list: + - a: from base config + - b: from package3 + - c: from nested package4 +nested_package_test_list: + - a: from base config + - b: from package3 + - c: from nested package4 diff --git a/tests/unit_tests/fixtures/substitutions/10-dynamic_packages.input.yaml b/tests/unit_tests/fixtures/substitutions/10-dynamic_packages.input.yaml new file mode 100644 index 0000000000..800e3cc7a8 --- /dev/null +++ b/tests/unit_tests/fixtures/substitutions/10-dynamic_packages.input.yaml @@ -0,0 +1,62 @@ +command_line_substitutions: + selected_package_number: 3 + +substitutions: + a: from base config + + package1: &p1 + substitutions: + a: from package1 + b: from package1 + c: from package1 + package1_test_list: + - a: ${ a } + - b: ${ b } + - c: ${ c } + + package2: &p2 !include + file: package2.yaml + vars: + a: from package2 vars + + package3: &p3 + substitutions: + a: from package3 + b: from package3 + c: from package3 + package3_test_list: + - a: ${ a } + - b: ${ b } + - c: ${ c } + + package4: + substitutions: + nested_package: + substitutions: + c: from nested package4 + nested_package_test_list: + - a: ${ a } + - b: ${ b } + - c: ${ c } + packages: + - ${ nested_package } + + package_map: + package1: *p1 + package2: *p2 + package3: *p3 + + selected_package_number: 2 # will be overridden by command line substitutions + selected_package_name: package${ selected_package_number } + selected_package: ${ package_map[selected_package_name] } + +packages: + - ${ package1 } + - ${ package2 } + - ${ selected_package } + - ${ package4 } + +base_test_list: + - a: ${ a } + - b: ${ b } + - c: ${ c } diff --git a/tests/unit_tests/fixtures/substitutions/level1_package.yaml b/tests/unit_tests/fixtures/substitutions/level1_package.yaml new file mode 100644 index 0000000000..d8a994b7b4 --- /dev/null +++ b/tests/unit_tests/fixtures/substitutions/level1_package.yaml @@ -0,0 +1,21 @@ +# this file is included by 07-include_hierarchy.input.yaml +level1: + a: ${a} # top-level substitution + b: ${b} # top-level substitution + c: ${c} # from vars when including + d: ${d} # from vars when including + e: ${e} # undefined at this level + f: ${f} # undefined at this level + g: ${g} # undefined at this level + h: ${h} # undefined at this level + i: ${i} # undefined at this level + j: ${j} # undefined at this level + x: ${x} # from vars when including, calculated + y: ${y} # from vars when including, calculated + level2: + - !include + file: level2_package.yaml + vars: + e: ${c*2} + f: ${d*2} + x: ${x+1} diff --git a/tests/unit_tests/fixtures/substitutions/level2_package.yaml b/tests/unit_tests/fixtures/substitutions/level2_package.yaml new file mode 100644 index 0000000000..460135553a --- /dev/null +++ b/tests/unit_tests/fixtures/substitutions/level2_package.yaml @@ -0,0 +1,21 @@ +# this file is included by level1_package.yaml +level2: + a: ${a} # top-level substitution + b: ${b} # top-level substitution + c: ${c} # visible from level1 vars + d: ${d} # visible from level1 vars + e: ${e} # from vars when including + f: ${f} # from vars when including + g: ${g} # undefined at this level + h: ${h} # undefined at this level + i: ${i} # undefined at this level + j: ${j} # undefined at this level + x: ${x} # from vars when including, calculated + y: ${y} # from vars when including, calculated + level3: + - !include + file: level3_package.yaml + vars: + g: ${e*5} + h: ${f*5} + x: ${x+1} diff --git a/tests/unit_tests/fixtures/substitutions/level3_package.yaml b/tests/unit_tests/fixtures/substitutions/level3_package.yaml new file mode 100644 index 0000000000..b16ed5fcf6 --- /dev/null +++ b/tests/unit_tests/fixtures/substitutions/level3_package.yaml @@ -0,0 +1,16 @@ +# this file is included by level2_package.yaml +defaults: + i: 30 +level3: + a: ${a} # top-level substitution + b: ${b} # top-level substitution + c: ${c} # visible from level1 vars + d: ${d} # visible from level1 vars + e: ${e} # visible from level2 vars + f: ${f} # visible from level2 vars + g: ${g} # from vars when including + h: ${h} # from vars when including + i: ${i} # Should take the default value of 30 + j: ${undefined_variable} # Does not exist, should be output as-is + x: ${x} # from vars when including, calculated + y: ${y} # from vars when including, calculated diff --git a/tests/unit_tests/fixtures/substitutions/package2.yaml b/tests/unit_tests/fixtures/substitutions/package2.yaml new file mode 100644 index 0000000000..998cd74c52 --- /dev/null +++ b/tests/unit_tests/fixtures/substitutions/package2.yaml @@ -0,0 +1,10 @@ +# included from 10-dynamic_packages.input.yaml +substitutions: + a: from package2 # must not override base config's a + # b not defined here, won't override package1's b + c: from package2 # will override package1's c + +package2_test_list: + - a: ${ a } + - b: ${ b } + - c: ${ c } diff --git a/tests/unit_tests/test_substitutions.py b/tests/unit_tests/test_substitutions.py index 30478f9521..c7b0bbcf7c 100644 --- a/tests/unit_tests/test_substitutions.py +++ b/tests/unit_tests/test_substitutions.py @@ -143,7 +143,9 @@ def test_substitutions_fixtures( command_line_substitutions = config.pop("command_line_substitutions", None) - config = do_packages_pass(config) + config = do_packages_pass( + config, command_line_substitutions=command_line_substitutions + ) config = substitutions.do_substitution_pass(config, command_line_substitutions) diff --git a/tests/unit_tests/test_yaml_util.py b/tests/unit_tests/test_yaml_util.py index 35a4bc3707..667b593819 100644 --- a/tests/unit_tests/test_yaml_util.py +++ b/tests/unit_tests/test_yaml_util.py @@ -98,13 +98,15 @@ def test_construct_secret_missing(fixture_path: Path, tmp_path: Path) -> None: """Test that missing secrets raise proper errors.""" # Create a YAML file with a secret that doesn't exist test_yaml = tmp_path / "test.yaml" - test_yaml.write_text(""" + test_yaml.write_text( + """ esphome: name: test wifi: password: !secret nonexistent_secret -""") +""" + ) # Create an empty secrets file secrets_yaml = tmp_path / "secrets.yaml" @@ -118,10 +120,12 @@ def test_construct_secret_no_secrets_file(tmp_path: Path) -> None: """Test that missing secrets.yaml file raises proper error.""" # Create a YAML file with a secret but no secrets.yaml test_yaml = tmp_path / "test.yaml" - test_yaml.write_text(""" + test_yaml.write_text( + """ wifi: password: !secret some_secret -""") +""" + ) # Mock CORE.config_path to avoid NoneType error with ( @@ -140,10 +144,12 @@ def test_construct_secret_fallback_to_main_config_dir( subdir.mkdir() test_yaml = subdir / "test.yaml" - test_yaml.write_text(""" + test_yaml.write_text( + """ wifi: password: !secret test_secret -""") +""" + ) # Create secrets.yaml in the main directory main_secrets = tmp_path / "secrets.yaml" @@ -164,9 +170,11 @@ def test_construct_include_dir_named(fixture_path: Path, tmp_path: Path) -> None # Create test YAML that uses include_dir_named test_yaml = dst_dir / "test_include_named.yaml" - test_yaml.write_text(""" + test_yaml.write_text( + """ sensor: !include_dir_named named_dir -""") +""" + ) actual = yaml_util.load_yaml(test_yaml) actual_sensor = actual["sensor"] @@ -199,9 +207,11 @@ def test_construct_include_dir_named_empty_dir(tmp_path: Path) -> None: empty_dir.mkdir() test_yaml = tmp_path / "test.yaml" - test_yaml.write_text(""" + test_yaml.write_text( + """ sensor: !include_dir_named empty_dir -""") +""" + ) actual = yaml_util.load_yaml(test_yaml) @@ -231,9 +241,11 @@ def test_construct_include_dir_named_with_dots(tmp_path: Path) -> None: hidden_subfile.write_text("key: hidden_subfile_value") test_yaml = tmp_path / "test.yaml" - test_yaml.write_text(""" + test_yaml.write_text( + """ test: !include_dir_named test_dir -""") +""" + ) actual = yaml_util.load_yaml(test_yaml) @@ -255,9 +267,11 @@ def test_find_files_recursive(fixture_path: Path, tmp_path: Path) -> None: # This indirectly tests _find_files by using include_dir_named test_yaml = dst_dir / "test_include_recursive.yaml" - test_yaml.write_text(""" + test_yaml.write_text( + """ all_sensors: !include_dir_named named_dir -""") +""" + ) actual = yaml_util.load_yaml(test_yaml)