[core] Improve framework mirror selection, download errors, and version parsing (#17615)

tests/component_tests/esp32/test_esp32.py

@@ -740,3 +740,28 @@ def test_signed_ota_keys_invalid_combinations(config: dict, match: str) -> None:
 
     with pytest.raises(cv.Invalid, match=match):
         _validate_signed_ota_keys(config)
+
+
+@pytest.mark.parametrize(
+    ("value", "expected"),
+    [
+        # Full x.y.z versions are rewritten into pioarduino release URLs
+        (
+            "55.3.30",
+            "https://github.com/pioarduino/platform-espressif32/releases/download/55.03.30/platform-espressif32.zip",
+        ),
+        (
+            "55.3.31-2",
+            "https://github.com/pioarduino/platform-espressif32/releases/download/55.03.31-2/platform-espressif32.zip",
+        ),
+        # Non-version values pass through untouched
+        (
+            "https://github.com/pioarduino/platform-espressif32.git#develop",
+            "https://github.com/pioarduino/platform-espressif32.git#develop",
+        ),
+    ],
+)
+def test_parse_pio_platform_version(value: str, expected: str) -> None:
+    from esphome.components.esp32 import _parse_pio_platform_version
+
+    assert _parse_pio_platform_version(value) == expected

tests/unit_tests/test_config_validation.py

@@ -1436,9 +1436,41 @@ def test_version_parse_with_extra() -> None:
     assert version.extra == "dev20240101"
 
 
-def test_version_parse_invalid() -> None:
+def test_version_parse_without_patch() -> None:
+    """A two-part version parses with patch defaulting to 0, so framework
+    shorthands like '6.0' and '6.0-rc1' are accepted."""
+    version = cv.Version.parse("6.0")
+    assert (version.major, version.minor, version.patch, version.extra) == (
+        6,
+        0,
+        0,
+        "",
+    )
+    version = cv.Version.parse("6.0-rc1")
+    assert (version.major, version.minor, version.patch, version.extra) == (
+        6,
+        0,
+        0,
+        "rc1",
+    )
+
+
+def test_version_parse_numeric_extra() -> None:
+    """Four-part versions keep the trailing component as extra (pioarduino
+    packaging revisions, e.g. 5.5.3.1)."""
+    version = cv.Version.parse("5.5.3.1")
+    assert (version.major, version.minor, version.patch, version.extra) == (
+        5,
+        5,
+        3,
+        "1",
+    )
+
+
+@pytest.mark.parametrize("value", ["not.a.version", "6", "a.b", ""])
+def test_version_parse_invalid(value: str) -> None:
     with pytest.raises(ValueError, match="Not a valid version number"):
-        cv.Version.parse("not.a.version")
+        cv.Version.parse(value)
 
 
 def test_version_is_beta() -> None:

tests/unit_tests/test_espidf_framework.py

@@ -489,6 +489,29 @@ def test_check_esp_idf_install_unparseable_version(
     espidf_mocks.extract.assert_called_once()
 
 
+@pytest.mark.parametrize(
+    ("version", "short_version"),
+    [
+        ("6.0.0", "6.0"),
+        ("6.0.0-rc1", "6.0-rc1"),
+        ("5.5.4", None),  # vX.Y tags only exist for X.Y.0 releases
+    ],
+)
+def test_check_esp_idf_install_short_version_substitution(
+    espidf_mocks: SimpleNamespace, version: str, short_version: str | None
+) -> None:
+    """SHORT_VERSION is only offered for x.y.0 releases, so the vX.Y mirror
+    template is never tried for versions whose tag cannot exist."""
+    _get_framework_path(version).mkdir(parents=True, exist_ok=True)
+    check_esp_idf_install(version, force=True)
+
+    # First call downloads the framework archive; a later call fetches the
+    # constraints file with its own substitutions.
+    substitutions = espidf_mocks.download.call_args_list[0][0][1]
+    assert substitutions.get("SHORT_VERSION") == short_version
+    assert substitutions["VERSION"] == version
+
+
 # ---------------------------------------------------------------------------
 # _patch_tools_json_for_linux_arm64 (arm64-only ninja backport)
 # ---------------------------------------------------------------------------

tests/unit_tests/test_framework_helpers.py

@@ -16,6 +16,7 @@ import zipfile
 import pytest
 import requests as req
 
+from esphome.core import EsphomeError
 from esphome.framework_helpers import (
     _7z_extract_all,
     _detect_archive_root,
@@ -546,6 +547,99 @@ class TestDownloadFromMirrors:
             )
         assert mock_get.call_args[0][0] == "https://example.com/1.2.3.bin"
 
+    def test_template_with_missing_substitution_is_skipped(
+        self, tmp_path: Path
+    ) -> None:
+        """A template referencing an unavailable substitution is skipped, not
+        formatted into a bogus URL (e.g. SHORT_VERSION only exists for x.y.0
+        framework versions)."""
+        with patch(
+            "requests.get",
+            return_value=_mock_response(b"x"),
+        ) as mock_get:
+            url = download_from_mirrors(
+                [
+                    "https://example.com/{SHORT_VERSION}.bin",
+                    "https://example.com/{VERSION}.bin",
+                ],
+                {"VERSION": "1.2.3"},
+                tmp_path / "out.bin",
+            )
+        assert url == "https://example.com/1.2.3.bin"
+        assert mock_get.call_count == 1
+
+    def test_all_templates_skipped_raises_esphome_error(self, tmp_path: Path) -> None:
+        with (
+            patch("requests.get") as mock_get,
+            pytest.raises(EsphomeError, match="No mirror URL template matched") as ei,
+        ):
+            download_from_mirrors(
+                ["https://example.com/{MISSING}.bin"],
+                {"VERSION": "1.2.3"},
+                tmp_path / "out.bin",
+            )
+        mock_get.assert_not_called()
+        # The skipped template and its missing substitution are named
+        assert "https://example.com/{MISSING}.bin" in str(ei.value)
+        assert "MISSING" in str(ei.value)
+
+    def test_failure_message_includes_skipped_templates(self, tmp_path: Path) -> None:
+        """When downloads fail, templates that were skipped for missing
+        substitutions are also listed so a typo'd custom mirror is
+        attributable."""
+        with (
+            patch(
+                "requests.get",
+                return_value=_mock_response(b"", ok=False),
+            ),
+            pytest.raises(EsphomeError, match="all mirrors") as ei,
+        ):
+            download_from_mirrors(
+                [
+                    "https://example.com/{TYPO}.bin",
+                    "https://example.com/{VERSION}.bin",
+                ],
+                {"VERSION": "1.2.3"},
+                tmp_path / "out.bin",
+            )
+        message = str(ei.value)
+        assert "https://example.com/1.2.3.bin" in message
+        assert (
+            "https://example.com/{TYPO}.bin\n    not applicable (TYPO not available)"
+            in message
+        )
+
+    def test_malformed_template_warns_and_is_reported(
+        self, tmp_path: Path, caplog: pytest.LogCaptureFixture
+    ) -> None:
+        """A structurally malformed template is an authoring error: warned
+        about even when another mirror succeeds, and named in the aggregate
+        error when everything fails."""
+        with (
+            patch("requests.get", return_value=_mock_response(b"x")),
+            caplog.at_level(logging.WARNING, logger="esphome.framework_helpers"),
+        ):
+            url = download_from_mirrors(
+                ["https://example.com/{oops.bin", "https://example.com/{VERSION}.bin"],
+                {"VERSION": "1.2.3"},
+                tmp_path / "out.bin",
+            )
+        assert url == "https://example.com/1.2.3.bin"
+        assert "malformed mirror URL template" in caplog.text
+
+        with (
+            patch("requests.get", return_value=_mock_response(b"", ok=False)),
+            pytest.raises(EsphomeError, match="all mirrors") as ei,
+        ):
+            download_from_mirrors(
+                ["https://example.com/{oops.bin", "https://example.com/{VERSION}.bin"],
+                {"VERSION": "1.2.3"},
+                tmp_path / "out.bin",
+            )
+        assert "https://example.com/{oops.bin\n    skipped (ValueError(" in str(
+            ei.value
+        )
+
     def test_falls_back_to_second_mirror(self, tmp_path: Path) -> None:
         with patch(
             "requests.get",
@@ -559,15 +653,26 @@ class TestDownloadFromMirrors:
         assert url == "https://mirror2.com/f"
         assert (tmp_path / "out.bin").read_bytes() == b"second"
 
-    def test_all_mirrors_fail_reraises_last_exception(self, tmp_path: Path) -> None:
+    def test_all_mirrors_fail_raises_error_listing_every_attempt(
+        self, tmp_path: Path
+    ) -> None:
         with (
             patch(
                 "requests.get",
                 return_value=_mock_response(b"", ok=False),
             ),
-            pytest.raises(req.HTTPError),
+            pytest.raises(EsphomeError, match="all mirrors") as excinfo,
         ):
-            download_from_mirrors(["https://example.com/f"], {}, tmp_path / "out.bin")
+            download_from_mirrors(
+                ["https://mirror1.com/f", "https://mirror2.com/f"],
+                {},
+                tmp_path / "out.bin",
+            )
+        # Every attempted URL appears in the message, and the first mirror's
+        # exception (the primary URL, usually the one that matters) is chained.
+        assert "https://mirror1.com/f" in str(excinfo.value)
+        assert "https://mirror2.com/f" in str(excinfo.value)
+        assert isinstance(excinfo.value.__cause__, req.HTTPError)
 
     def test_empty_mirrors_raises_value_error(self, tmp_path: Path) -> None:
         with pytest.raises(ValueError, match="empty mirrors list"):