diff --git a/esphome/components/feedback/feedback_cover.cpp b/esphome/components/feedback/feedback_cover.cpp index 672e99949b..1139e6fa18 100644 --- a/esphome/components/feedback/feedback_cover.cpp +++ b/esphome/components/feedback/feedback_cover.cpp @@ -375,12 +375,10 @@ void FeedbackCover::start_direction_(CoverOperation dir) { // check if we have a wait time if (this->direction_change_waittime_.has_value() && dir != COVER_OPERATION_IDLE && this->current_operation != COVER_OPERATION_IDLE && dir != this->current_operation) { + const uint32_t waittime = *this->direction_change_waittime_; ESP_LOGD(TAG, "'%s' - Reversing direction.", this->name_.c_str()); this->start_direction_(COVER_OPERATION_IDLE); - - this->set_timeout(DIRECTION_CHANGE_TIMEOUT_ID, *this->direction_change_waittime_, - [this, dir]() { this->start_direction_(dir); }); - + this->set_timeout(DIRECTION_CHANGE_TIMEOUT_ID, waittime, [this, dir]() { this->start_direction_(dir); }); } else { this->set_current_operation_(dir, true); this->prev_command_trigger_ = trig; diff --git a/esphome/external_files.py b/esphome/external_files.py index 8efebd047d..dfabc54f47 100644 --- a/esphome/external_files.py +++ b/esphome/external_files.py @@ -211,10 +211,11 @@ def download_content_many( """Run `download_content` for each (url, path) pair concurrently. Wall time drops from `sum(latency)` to roughly `max(latency)` for cached - files where the HEAD round-trip dominates. Worker exceptions propagate - when iteration reaches the corresponding input item (`ex.map` yields - results in input order), and remaining workers complete before this - returns. + files where the HEAD round-trip dominates. All workers run to + completion before this returns; every `cv.Invalid` raised by a worker + is collected and surfaced together as `cv.MultipleInvalid` so the user + sees every broken file in a single validation pass instead of fixing + them one round-trip at a time. Items are de-duplicated by `path` -- two callers asking for the same cache file (e.g. the same URL referenced twice in a config) would @@ -238,9 +239,19 @@ def download_content_many( download_content(url, path, timeout) workers = max(1, min(max_workers, len(seen))) + errors: list[cv.Invalid] = [] with ThreadPoolExecutor(max_workers=workers) as ex: - # list() forces iteration so exceptions surface here, not silently. - list(ex.map(_download_one, seen.items())) + futures = [ex.submit(_download_one, item) for item in seen.items()] + for future in futures: + try: + future.result() + except cv.Invalid as e: + errors.append(e) + if not errors: + return + if len(errors) == 1: + raise errors[0] + raise cv.MultipleInvalid(errors) # Each component that uses external_files defines its own local diff --git a/script/build_helpers.py b/script/build_helpers.py index 4cf2f93fbb..0e0e8170a0 100644 --- a/script/build_helpers.py +++ b/script/build_helpers.py @@ -57,6 +57,59 @@ def hash_components(components: list[str]) -> str: return hashlib.sha256(key.encode()).hexdigest()[:16] +def populate_dependency_config( + config: dict, + component_names: list[str], + *, + get_component_fn: Callable[[str], object | None] = get_component, + register_platform_fn: Callable[[str], None] | None = None, +) -> None: + """Populate ``config`` with empty entries for transitive dependencies. + + For every name in ``component_names``: + + * ``domain.platform`` form (e.g. ``sensor.gpio``) appends + ``{platform: }`` to ``config[domain]``, creating the list if needed. + * Bare components are looked up via ``get_component_fn``. Platform + components (``IS_PLATFORM_COMPONENT``) and ``MULTI_CONF`` components are + initialised as ``[]`` so the sibling ``domain.platform`` branch can + ``append`` into them. Everything else is populated by running the + component's schema with ``{}`` so defaults exist; if the schema requires + explicit input, an empty ``{}`` is used as a fallback. + + Platform components must always be a list here even when no + ``domain.platform`` entry follows, because the ``domain.platform`` branch + does ``config.setdefault(domain, []).append(...)`` and would crash on a + leftover dict. + """ + if register_platform_fn is None: + register_platform_fn = CORE.testing_ensure_platform_registered + for component_name in component_names: + if "." in component_name: + domain, component = component_name.split(".", maxsplit=1) + domain_list = config.setdefault(domain, []) + register_platform_fn(domain) + domain_list.append({CONF_PLATFORM: component}) + continue + # Skip "core" — it's a pseudo-component handled by the build + # system, not a real loadable component (get_component returns None) + component = get_component_fn(component_name) + if component is None: + continue + if component.multi_conf or component.is_platform_component: + config.setdefault(component_name, []) + elif component_name not in config: + schema = component.config_schema + try: + config[component_name] = schema({}) if schema is not None else {} + except Exception: # noqa: BLE001 + # Schema requires explicit input we can't synthesize; fall + # back to an empty mapping so subscripting at least returns + # KeyError on missing keys rather than crashing on the + # wrong type. + config[component_name] = {} + + def filter_components_with_files(components: list[str], tests_dir: Path) -> list[str]: """Filter out components that do not have .cpp or .h files in the tests dir. @@ -316,31 +369,7 @@ def compile_and_get_binary( # Add remaining components and dependencies to the configuration after # validation, so their source files are included in the build. - for component_name in components_with_dependencies: - if "." in component_name: - domain, component = component_name.split(".", maxsplit=1) - domain_list = config.setdefault(domain, []) - CORE.testing_ensure_platform_registered(domain) - domain_list.append({CONF_PLATFORM: component}) - # Skip "core" — it's a pseudo-component handled by the build - # system, not a real loadable component (get_component returns None) - elif (component := get_component(component_name)) is not None: - # MULTI_CONF components store their config as a list of dicts, - # everything else stores a single dict. Run the component's - # schema with {} so defaults get populated -- code paths like - # socket.FILTER_SOURCE_FILES expect a fully-populated mapping. - if component.multi_conf: - config.setdefault(component_name, []) - elif component_name not in config: - schema = component.config_schema - try: - config[component_name] = schema({}) if schema is not None else {} - except Exception: # noqa: BLE001 - # Schema requires explicit input we can't synthesize; fall - # back to an empty mapping so subscripting at least returns - # KeyError on missing keys rather than crashing on the - # wrong type. - config[component_name] = {} + populate_dependency_config(config, components_with_dependencies) # Register platforms from the extra config (benchmark.yaml) so # USE_SENSOR, USE_LIGHT, etc. defines are emitted without needing diff --git a/tests/script/test_test_helpers.py b/tests/script/test_test_helpers.py index 467940fc33..3149712563 100644 --- a/tests/script/test_test_helpers.py +++ b/tests/script/test_test_helpers.py @@ -258,3 +258,161 @@ def test_load_wraps_platform_component(tmp_path: Path) -> None: assert key == "bthome.sensor" assert isinstance(installed, ComponentManifestOverride) assert installed.to_code is None + + +# --------------------------------------------------------------------------- +# populate_dependency_config +# --------------------------------------------------------------------------- + + +def _make_component_stub( + *, + multi_conf: bool = False, + is_platform_component: bool = False, + config_schema=None, +) -> MagicMock: + stub = MagicMock() + stub.multi_conf = multi_conf + stub.is_platform_component = is_platform_component + stub.config_schema = config_schema + return stub + + +def test_populate_platform_component_listed_alone_uses_list() -> None: + """Regression: a platform component (sensor) with no `sensor.x` siblings + must land as `[]` in config. Previously it was populated as a dict via + `schema({})`, which then crashed the sibling `domain.platform` branch + when later dependencies tried `config.setdefault('sensor', []).append(...)`. + """ + sensor = _make_component_stub(is_platform_component=True) + config: dict = {} + + build_helpers.populate_dependency_config( + config, + ["sensor"], + get_component_fn=lambda name: sensor if name == "sensor" else None, + register_platform_fn=lambda _: None, + ) + + assert config["sensor"] == [] + + +def test_populate_platform_component_then_platform_entry() -> None: + """When `sensor` is processed before `sensor.gpio` (sorted order), + the bare-component branch must leave `config['sensor']` as a list so + the platform-entry branch can append into it. + """ + sensor = _make_component_stub(is_platform_component=True) + gpio = _make_component_stub() # the bare `gpio` component + components: dict[str, object] = {"sensor": sensor, "gpio": gpio} + config: dict = {} + + build_helpers.populate_dependency_config( + config, + ["gpio", "sensor", "sensor.gpio"], + get_component_fn=components.get, + register_platform_fn=lambda _: None, + ) + + assert config["sensor"] == [{"platform": "gpio"}] + + +def test_populate_multi_conf_component_uses_list() -> None: + multi = _make_component_stub(multi_conf=True) + config: dict = {} + + build_helpers.populate_dependency_config( + config, + ["multi"], + get_component_fn=lambda name: multi if name == "multi" else None, + register_platform_fn=lambda _: None, + ) + + assert config["multi"] == [] + + +def test_populate_plain_component_uses_schema_defaults() -> None: + schema = MagicMock(return_value={"default_key": 42}) + plain = _make_component_stub(config_schema=schema) + config: dict = {} + + build_helpers.populate_dependency_config( + config, + ["plain"], + get_component_fn=lambda name: plain if name == "plain" else None, + register_platform_fn=lambda _: None, + ) + + schema.assert_called_once_with({}) + assert config["plain"] == {"default_key": 42} + + +def test_populate_plain_component_falls_back_when_schema_raises() -> None: + def picky_schema(_): + raise ValueError("required field missing") + + plain = _make_component_stub(config_schema=picky_schema) + config: dict = {} + + build_helpers.populate_dependency_config( + config, + ["plain"], + get_component_fn=lambda name: plain if name == "plain" else None, + register_platform_fn=lambda _: None, + ) + + assert config["plain"] == {} + + +def test_populate_skips_unresolvable_pseudo_components() -> None: + """`core` and other names that get_component returns None for are skipped + silently without inserting anything into the config. + """ + config: dict = {} + + build_helpers.populate_dependency_config( + config, + ["core"], + get_component_fn=lambda _: None, + register_platform_fn=lambda _: None, + ) + + assert config == {} + + +def test_populate_preserves_existing_plain_component_config() -> None: + """If a plain component already has a config entry (e.g. from the user's + YAML), the schema-defaults branch must not overwrite it. + """ + schema = MagicMock() + plain = _make_component_stub(config_schema=schema) + config: dict = {"plain": {"user_key": "set_by_user"}} + + build_helpers.populate_dependency_config( + config, + ["plain"], + get_component_fn=lambda name: plain if name == "plain" else None, + register_platform_fn=lambda _: None, + ) + + schema.assert_not_called() + assert config["plain"] == {"user_key": "set_by_user"} + + +def test_populate_registers_platform_for_platform_entry() -> None: + """Each `domain.platform` entry triggers register_platform_fn(domain) so + USE_ defines get emitted later in the build pipeline. + """ + registered: list[str] = [] + config: dict = {} + + build_helpers.populate_dependency_config( + config, + ["sensor.gpio", "binary_sensor.gpio"], + get_component_fn=lambda _: None, + register_platform_fn=registered.append, + ) + + assert registered == ["sensor", "binary_sensor"] + assert config["sensor"] == [{"platform": "gpio"}] + assert config["binary_sensor"] == [{"platform": "gpio"}] diff --git a/tests/unit_tests/test_external_files.py b/tests/unit_tests/test_external_files.py index b816e986e0..0b545f9ed8 100644 --- a/tests/unit_tests/test_external_files.py +++ b/tests/unit_tests/test_external_files.py @@ -9,7 +9,7 @@ import pytest import requests from esphome import external_files -from esphome.config_validation import Invalid +from esphome.config_validation import Invalid, MultipleInvalid from esphome.core import CORE, EsphomeError, TimePeriod @@ -574,65 +574,6 @@ def test_download_content_skip_external_update_downloads_when_missing( assert test_file.read_bytes() == new_content -def test_download_content_saves_etag( - mock_has_remote_file_changed: MagicMock, - mock_requests_get: MagicMock, - setup_core: Path, -) -> None: - """Test download_content writes the ETag sidecar after a successful download.""" - test_file = setup_core / "fresh.txt" - new_content = b"fresh content" - - mock_has_remote_file_changed.return_value = True - mock_response = MagicMock() - mock_response.content = new_content - mock_response.headers = {external_files.ETAG: '"deadbeef"'} - mock_response.raise_for_status = MagicMock() - mock_requests_get.return_value = mock_response - - url = "https://example.com/file.txt" - external_files.download_content(url, test_file) - - assert external_files._etag_sidecar_path(test_file).read_text() == '"deadbeef"' - - -def test_download_content_atomic_write_no_partial_on_failure( - mock_has_remote_file_changed: MagicMock, - mock_requests_get: MagicMock, - mock_write_file: MagicMock, - setup_core: Path, -) -> None: - """If `write_file` (the atomic-write helper) fails, the existing cache - file must remain untouched and no temp files may be left behind. Patching - `write_file` directly exercises the atomic-rename path -- a failure inside - `write_file` is the only reason the rename wouldn't have happened. - """ - from esphome.core import EsphomeError - - test_file = setup_core / "cached.txt" - original_content = b"original content" - test_file.write_bytes(original_content) - - mock_has_remote_file_changed.return_value = True - mock_response = MagicMock() - mock_response.content = b"new content" - mock_response.headers = {} - mock_response.raise_for_status = MagicMock() - mock_requests_get.return_value = mock_response - - mock_write_file.side_effect = EsphomeError("disk full") - - with pytest.raises(EsphomeError, match="disk full"): - external_files.download_content("https://example.com/file.txt", test_file) - - # Original file is untouched -- write_file aborted before its rename step. - assert test_file.read_bytes() == original_content - # write_file is responsible for cleaning its own temp files; nothing leaks - # into the cache directory either way. - leftover_tmps = list(setup_core.glob("tmp*")) - assert leftover_tmps == [] - - def test_download_content_many_empty_is_noop( mock_download_content: MagicMock, setup_core: Path ) -> None: @@ -676,10 +617,12 @@ def test_download_content_many_runs_in_parallel( assert mock_download_content.call_count == 3 -def test_download_content_many_propagates_errors( +def test_download_content_many_propagates_single_error( mock_download_content: MagicMock, setup_core: Path ) -> None: - """An exception from any worker must propagate out of download_content_many.""" + """A single failing worker should raise its `Invalid` directly, not wrap + it in a `MultipleInvalid` that the caller would have to unpack. + """ def fake_download(url: str, path: Path, timeout: int) -> bytes: if url.endswith("bad"): @@ -691,8 +634,37 @@ def test_download_content_many_propagates_errors( ("https://example.com/ok", setup_core / "ok"), ("https://example.com/bad", setup_core / "bad"), ] - with pytest.raises(Invalid, match="could not download"): + with pytest.raises(Invalid, match="could not download") as exc_info: external_files.download_content_many(items) + assert not isinstance(exc_info.value, MultipleInvalid) + + +def test_download_content_many_aggregates_multiple_errors( + mock_download_content: MagicMock, setup_core: Path +) -> None: + """Every failing worker should be reported in a single MultipleInvalid so + the user sees all broken URLs in one validation pass instead of fixing + them one network round-trip at a time. + """ + + def fake_download(url: str, path: Path, timeout: int) -> bytes: + if url.endswith("ok"): + return b"" + raise Invalid(f"could not download {url}") + + mock_download_content.side_effect = fake_download + items = [ + ("https://example.com/ok", setup_core / "ok"), + ("https://example.com/bad1", setup_core / "bad1"), + ("https://example.com/bad2", setup_core / "bad2"), + ] + with pytest.raises(MultipleInvalid) as exc_info: + external_files.download_content_many(items) + messages = {str(e) for e in exc_info.value.errors} + assert messages == { + "could not download https://example.com/bad1", + "could not download https://example.com/bad2", + } def test_download_content_many_dedupes_by_path( @@ -767,3 +739,62 @@ def test_download_web_files_in_config_no_web_entries( external_files.download_web_files_in_config(config, lambda _: setup_core / "x") mock_download_content_many.assert_called_once() assert list(mock_download_content_many.call_args[0][0]) == [] + + +def test_download_content_saves_etag( + mock_has_remote_file_changed: MagicMock, + mock_requests_get: MagicMock, + setup_core: Path, +) -> None: + """Test download_content writes the ETag sidecar after a successful download.""" + test_file = setup_core / "fresh.txt" + new_content = b"fresh content" + + mock_has_remote_file_changed.return_value = True + mock_response = MagicMock() + mock_response.content = new_content + mock_response.headers = {external_files.ETAG: '"deadbeef"'} + mock_response.raise_for_status = MagicMock() + mock_requests_get.return_value = mock_response + + url = "https://example.com/file.txt" + external_files.download_content(url, test_file) + + assert external_files._etag_sidecar_path(test_file).read_text() == '"deadbeef"' + + +def test_download_content_atomic_write_no_partial_on_failure( + mock_has_remote_file_changed: MagicMock, + mock_requests_get: MagicMock, + mock_write_file: MagicMock, + setup_core: Path, +) -> None: + """If `write_file` (the atomic-write helper) fails, the existing cache + file must remain untouched and no temp files may be left behind. Patching + `write_file` directly exercises the atomic-rename path -- a failure inside + `write_file` is the only reason the rename wouldn't have happened. + """ + from esphome.core import EsphomeError + + test_file = setup_core / "cached.txt" + original_content = b"original content" + test_file.write_bytes(original_content) + + mock_has_remote_file_changed.return_value = True + mock_response = MagicMock() + mock_response.content = b"new content" + mock_response.headers = {} + mock_response.raise_for_status = MagicMock() + mock_requests_get.return_value = mock_response + + mock_write_file.side_effect = EsphomeError("disk full") + + with pytest.raises(EsphomeError, match="disk full"): + external_files.download_content("https://example.com/file.txt", test_file) + + # Original file is untouched -- write_file aborted before its rename step. + assert test_file.read_bytes() == original_content + # write_file is responsible for cleaning its own temp files; nothing leaks + # into the cache directory either way. + leftover_tmps = list(setup_core.glob("tmp*")) + assert leftover_tmps == []