Skip to content

Make psram mode/speed variant assertions skew-tolerant#1683

Merged
swoboda1337 merged 2 commits into
esphome:mainfrom
swoboda1337:psram-mode-variant-skew-tolerant
Jun 24, 2026
Merged

Make psram mode/speed variant assertions skew-tolerant#1683
swoboda1337 merged 2 commits into
esphome:mainfrom
swoboda1337:psram-mode-variant-skew-tolerant

Conversation

@swoboda1337

Copy link
Copy Markdown
Member

What does this implement/fix?

test_sync_components_psram.py pinned the exact variant list per psram mode and speed — e.g. assert by_value["octal"] == ["esp32s3"] and assert by_value["20MHZ"] == ["esp32p4"]. Those lists are derived from the installed esphome's psram CONFIG_SCHEMA, so the moment esphome adds a psram variant they flip from equality to inequality and the assertion fails.

esphome/esphome#17192 adds octal PSRAM for ESP32-S31 and quad for ESP32-H4, which turns by_value["octal"] into ["esp32s3", "esp32s31"] — breaking this repo's downstream test run against esphome dev, with no way for either side to go green first (the same lockstep that #1681 fixed for has_native_wifi).

Fix: assert membership ("esp32s3" in by_value["octal"]) instead of equality. The unioning logic stays covered (each mode/speed is still tagged with the variants that accept it) without pinning to esphome's exact, growing variant set. Verified the membership assertions hold against the currently-installed esphome.

Types of changes

  • Bugfix (non-breaking change which fixes an issue) — bugfix
  • New feature (non-breaking change which adds functionality) — new-feature
  • Enhancement to an existing feature — enhancement
  • Breaking change — breaking-change
  • Refactor (no behaviour change) — refactor
  • Documentation only — docs
  • Maintenance / chore — maintenance
  • CI / workflow change — ci
  • Dependencies bump — dependencies

Frontend coordination

  • No frontend change needed

Checklist

  • The code change is tested and works locally. (Verified the assertions against installed esphome by calling _psram_config_entries() directly; full pytest suite can't run in this env — conftest dep unavailable. CI runs it.)
  • Pre-commit hooks pass. (ruff/codespell ran on commit)
  • Tests have been added or updated under tests/ where applicable.
  • components.index.json / definitions/components/*.json have not been hand-edited.
  • Architecture-level changes are reflected in docs/ARCHITECTURE.md and/or docs/API.md (N/A).

test_sync_components_psram pinned the exact variant list per psram mode/speed
(`by_value["octal"] == ["esp32s3"]`). esphome adds psram variants over time
(e.g. esp32s31 octal, esp32h4 quad), which flips these to inequality and breaks
this repo's downstream test against esphome dev. Assert membership instead, so
the inference is still covered without lockstepping to esphome's variant set.
Copilot AI review requested due to automatic review settings June 24, 2026 18:01
@swoboda1337 swoboda1337 added the bugfix Bug fix label Jun 24, 2026
@swoboda1337

Copy link
Copy Markdown
Member Author

@esphbot review

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates PSRAM schema sync tests to be resilient to upstream ESPHome adding new variants, avoiding downstream CI failures caused by pinning exact per-variant option lists.

Changes:

  • Switch PSRAM mode/speed assertions from exact list equality to membership checks for key known variants.
  • Preserve coverage that each mode/speed option is tagged with compatible variants without locking to a fixed upstream set.

Comment thread tests/test_sync_components_psram.py
Comment thread tests/test_sync_components_psram.py Outdated
@codspeed-hq

codspeed-hq Bot commented Jun 24, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 27 untouched benchmarks


Comparing swoboda1337:psram-mode-variant-skew-tolerant (8075437) with main (fe91afb)

Open in CodSpeed

@esphbot

esphbot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

PR Review — Make psram mode/speed variant assertions skew-tolerant

Correct, minimal fix for a real downstream-lockstep problem. Merge-ready.

What's solid:

  • The membership-over-equality switch is exactly the right shape for variant lists derived from the installed esphome's psram CONFIG_SCHEMA — it survives [psram] Support ESP32-S31/H4 esphome#17192 adding octal for ESP32-S31 / quad for ESP32-H4 without flipping red, the same skew the has_native_wifi fix (Make has_native_wifi agreement test skew-tolerant #1681) addressed.
  • Coverage of the unioning logic is preserved: each mode/speed option is still asserted to carry its known variant tag, the structural invariants (set(by_value) == {quad, octal, hex}, ascending speed sort, no cross-chip default_value) are untouched, and the multi-variant quad assertion still pins both esp32 and esp32s3.
  • Scope is tight: test-only, four lines, bugfix label matches.

Worth noting:

  • Copilot's two inline comments flag the # Variants are lowercased... and # 20MHZ is P4-only... lines as newly-added rationale violating CLAUDE.md comment style. Those lines are pre-existing context, not introduced by this diff — CLAUDE.md says don't remove existing comments unless the code they describe is gone, and it isn't. No action needed; the comments stay.
  • Minor accepted trade-off: membership no longer catches an unexpected extra variant sneaking into octal/hex/20MHZ. That's the intended loosening and the correct call given the lockstep.


Checklist

  • Test changes verify observable behavior, not source text
  • Structural invariants still covered after loosening
  • Scope matches stated bugfix purpose

Automated review by Kōan (Claude) HEAD=8075437 26s

@esphbot esphbot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

@codecov

codecov Bot commented Jun 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.54%. Comparing base (fe91afb) to head (8075437).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1683   +/-   ##
=======================================
  Coverage   99.54%   99.54%           
=======================================
  Files         227      227           
  Lines       18129    18129           
=======================================
  Hits        18047    18047           
  Misses         82       82           
Flag Coverage Δ
py3.12 99.52% <ø> (+<0.01%) ⬆️
py3.14 99.54% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@swoboda1337 swoboda1337 merged commit 4bdc129 into esphome:main Jun 24, 2026
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants