test: Verify each driver is frozen in the upstream firmware manifest.#393
test: Verify each driver is frozen in the upstream firmware manifest.#393
Conversation
Adds a parametrized pytest test that generates one case per `lib/*/` and asserts the driver name is declared in the STEAM32_WB55RG manifest hosted in steamicc/micropython-steami. Catches regressions like the accidental removal of mcp23009e and the forgotten steami_screen. The upstream manifest is fetched at test time (no dependency on a local clone), with an env override for forks. A `lib/<driver>/.not-frozen` marker opts a driver out with a one-line reason — used here for gc9a01 (pending freeze, #368) and im34dt05 (not yet integrated). Network failures skip cleanly so offline dev is unaffected. Closes #392.
There was a problem hiding this comment.
Pull request overview
Adds a CI-facing safeguard to ensure every driver under lib/ is represented in the upstream MicroPython firmware frozen-manifest, with an explicit per-driver opt-out mechanism and documentation.
Changes:
- Add
tests/test_frozen_manifest.pyto fetch and validate the upstreamSTEAM32_WB55RGmanifest againstlib/*/drivers. - Add
.not-frozenmarker files forgc9a01andim34dt05to intentionally skip checks for drivers not shipped in firmware yet. - Document the frozen-manifest check and
.not-frozenopt-out inCONTRIBUTING.md.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/test_frozen_manifest.py | New test that fetches the upstream board manifest and asserts each lib/<driver>/ is required (or skipped via .not-frozen). |
| lib/gc9a01/.not-frozen | Marks gc9a01 as intentionally not frozen (skipped by the new test). |
| lib/im34dt05/.not-frozen | Marks im34dt05 as intentionally not frozen (skipped by the new test). |
| CONTRIBUTING.md | Documents the new frozen-manifest verification and the .not-frozen opt-out. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| except (URLError, TimeoutError, OSError) as exc: | ||
| pytest.skip(f"cannot fetch upstream manifest: {exc}") |
There was a problem hiding this comment.
The exception handling treats all URLError as an offline/network condition and skips the entire check. urllib.error.HTTPError is a URLError, so a 404/403 (e.g., upstream path moved, branch renamed, rate-limited) will silently skip and permanently disable this protection in CI. Consider handling HTTPError separately (fail with a clear message for non-2xx responses, or only skip on genuine connectivity/timeouts).
There was a problem hiding this comment.
Fixed in 34281f7: HTTPError is caught separately and fails the test loudly with a pointer to MANIFEST_URL, so a renamed branch or moved path cannot silently disable the check. Only URLError / TimeoutError / OSError still skip (genuine offline cases).
| REQUIRE_RE = re.compile( | ||
| r'require\(\s*"([^"]+)"\s*,\s*library\s*=\s*"micropython-steami-lib"\s*\)' | ||
| ) |
There was a problem hiding this comment.
REQUIRE_RE is very formatting-specific: it only matches double-quoted require("name", library="micropython-steami-lib") with no extra kwargs and with library in that exact position. A semantically equivalent manifest using single quotes, additional keyword args, or different argument ordering will produce an empty/partial frozen_drivers set and cause false failures. To make the test resilient, parse the manifest as Python (e.g., via ast) and extract require() calls by inspecting call nodes/keywords, or broaden the pattern to accept both quote styles and additional args.
There was a problem hiding this comment.
Fixed in 34281f7: replaced the regex with an ast-based walker that inspects require() call nodes and matches on the library= keyword. Verified locally against a manifest using single quotes, extra keyword arguments, and both — both parse correctly.
| * The directory name must match the driver name (e.g. `mcp23009e`, `wsen-hids`) | ||
| * The main class must be exposed in `__init__.py` | ||
| * Drivers must be self-contained (no cross-driver dependencies) | ||
| * Every driver is automatically checked against the upstream firmware manifest by `tests/test_frozen_manifest.py`. If a driver is intentionally **not** frozen (experimental, not yet integrated, etc.), add an empty `lib/<driver>/.not-frozen` file containing a one-line reason — the test will skip it with that reason displayed. |
There was a problem hiding this comment.
This bullet is internally inconsistent: it says to add an empty .not-frozen file while also saying it should contain a one-line reason. Consider rewording to “add a lib/<driver>/.not-frozen marker file (may be empty, optionally containing a one-line reason)” to match the behavior in tests/test_frozen_manifest.py.
| * Every driver is automatically checked against the upstream firmware manifest by `tests/test_frozen_manifest.py`. If a driver is intentionally **not** frozen (experimental, not yet integrated, etc.), add an empty `lib/<driver>/.not-frozen` file containing a one-line reason — the test will skip it with that reason displayed. | |
| * Every driver is automatically checked against the upstream firmware manifest by `tests/test_frozen_manifest.py`. If a driver is intentionally **not** frozen (experimental, not yet integrated, etc.), add a `lib/<driver>/.not-frozen` marker file (it may be empty; optionally include a one-line reason) — the test will skip it and display the reason if present. |
There was a problem hiding this comment.
Fixed in 34281f7: reworded using your suggestion — "it may be empty; optionally include a one-line reason".
Three issues raised on #393: 1. URLError catches HTTPError too, so a 404 (branch/path renamed, path moved) would silently skip the test and permanently disable the protection. Handle HTTPError separately and fail loudly with a message pointing to MANIFEST_URL. 2. The regex-based extraction was formatting-sensitive: single quotes, extra keyword arguments, or a different argument order would produce false failures. Replace with an ast-based walker that inspects require() calls and matches on the library= keyword — resilient to any legal Python formatting. 3. CONTRIBUTING.md contradicted itself ("empty file containing a one-line reason"). Reword per Copilot's suggestion: empty by default, optionally with a one-line reason.
|
🎉 This PR is included in version 0.16.8 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Summary
tests/test_frozen_manifest.py: one parametrized case perlib/*/, asserts the driver name is required in the upstreamSTEAM32_WB55RGmanifest.gc9a01andim34dt05as.not-frozen(currently not shipped in the firmware). The marker file contains a one-line reason shown in the skip message..not-frozenopt-out inCONTRIBUTING.md.Why
Two regressions slipped through in the past because no test watched the frozen manifest:
mcp23009ewas silently removed (fix: Add daplink_bridge and bme280 to frozen manifest micropython-steami#1) and restored in fix: Restore mcp23009e in frozen manifest. micropython-steami#2.steami_screenwas never added, caught manually via feat: Freeze steami_screen into the STEAM32_WB55RG firmware. micropython-steami#3.How it works
The test fetches the raw manifest from
steamicc/micropython-steami@stm32-steami-rev1d-finalonce per session, extracts everyrequire("<name>", library="micropython-steami-lib"), and asserts eachlib/<dir>/appears in that set. Network failures skip cleanly, so offline dev and CI flakes don't produce false negatives.The URL is overridable via
STEAMI_FIRMWARE_MANIFEST_URLfor forks or local manifest testing.Test plan
make test-mock→ 15 passed + 2 skipped (gc9a01,im34dt05) on the new test.STEAMI_FIRMWARE_MANIFEST_URL=file:///tmp/fake_manifest.py) with onlyhts221in the file → 14 failed with actionable messages, 1 passed (hts221), 2 skipped.make lintpasses.Closes #392.