Repository navigation
test(levoit): compile tests + CI on every PR, and the build failures they found - #68
Merged
Merged
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Critical issues remain with sentinel preservation, no-fan dereferences, and nullable status handling.
Review effort: Lite
Findings: 4
Open (4)
What changed in this PR
Adds Levoit compile-test coverage, CI validation, and fixes optional-platform build dependencies.
Changes:
- Adds three self-contained compile configurations and GitHub Actions CI.
- Introduces
USE_LEVOIT_*guards and accessor-based entity handling. - Updates documentation and changelog details.
| File | Summary |
|---|---|
README.md |
Documents build fixes and compile tests. |
components/levoit/vital_status.cpp |
Routes status updates through accessors. |
components/levoit/vital_commands.cpp |
Removes direct platform dependencies. |
components/levoit/text_sensor/__init__.py |
Defines platform build configuration. |
components/levoit/tests/test-foreign-platforms.yaml |
Adds foreign-platform compile coverage. |
components/levoit/tests/test-fan-only.yaml |
Adds fan-only compile coverage. |
components/levoit/tests/test-all-entities.yaml |
Adds all-entities compile coverage. |
components/levoit/tests/README.md |
Documents compile tests. |
components/levoit/tests/.gitignore |
Ignores test artifacts. |
components/levoit/switch/__init__.py |
Defines the Levoit switch guard. |
components/levoit/superior_status.cpp |
Uses accessor-based status handling. |
components/levoit/superior_commands.cpp |
Removes direct platform dependencies. |
components/levoit/sprout_commands.cpp |
Removes direct platform dependencies. |
components/levoit/sensor/__init__.py |
Defines the Levoit sensor guard. |
components/levoit/select/__init__.py |
Defines the Levoit select guard. |
components/levoit/number/__init__.py |
Defines the Levoit number guard. |
components/levoit/light/__init__.py |
Defines the Levoit light guard. |
components/levoit/levoit.h |
Declares entity accessors. |
components/levoit/levoit.cpp |
Implements guarded accessors and entity handling. |
components/levoit/fan/__init__.py |
Defines the Levoit fan guard. |
components/levoit/core_status.cpp |
Uses accessor-based status handling. |
components/levoit/core_commands.cpp |
Removes direct platform dependencies. |
components/levoit/button/__init__.py |
Defines the Levoit button guard. |
components/levoit/binary_sensor/__init__.py |
Defines the Levoit binary sensor guard. |
.github/workflows/compile-tests.yml |
Runs compile tests in CI. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
tuct
added a commit
that referenced
this pull request
Sep 26, 2026
Review findings on #68, all four real: - apply_fan_status() took (bool, uint8_t, uint32_t), but the Vital and Superior decoders deliberately pass -1 for "not reported in this frame, leave the field alone". Narrowing turned an unknown power into ON and an unknown speed into 255, which then clamps to maximum - so a partial status frame would switch the fan on at full speed. Takes (int, int, int) now, matching LevoitFan::apply_device_status(). - The rewrite of vital_status.cpp dropped the `self != nullptr` check at that call site, even though decode_vital_status() accepts a nullable self and guards its other entity updates. Restored. - Guarding the fan include was not enough on its own: track_cadr_usage() and calculate_current_cadr_per_hour() still dereferenced fan_ against a forward-declared type. Both are now inside USE_LEVOIT_FAN, with the latter returning 0 when there is no fan entity to derive a duty cycle from. The first two were regressions I introduced in the previous commit; only the third was pre-existing. Verified: all three test configs compile, plus Core 400S and Vital 200S. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… found
Adds three self-contained configs under components/levoit/tests that build the
component without a secrets.yaml, plus a GitHub Actions workflow running them
on every PR touching components/.
The tests failed immediately on two real bugs, both now fixed:
- A levoit config with only a fan did not build. core_commands.cpp,
sprout_commands.cpp, superior_commands.cpp and superior_status.cpp included
number/levoit_number.h and sensor/levoit_sensor.h unguarded, and
core_status.cpp / vital_status.cpp included fan/levoit_fan.h the same way.
ESPHome only copies levoit/<platform>/ into the build when a levoit entity
of that platform is configured, so those headers were simply absent.
- levoit.cpp guarded its includes on the generic USE_SWITCH / USE_NUMBER /
... macros, which ANY component defines. A config with a template switch
but no levoit switch took the include and failed the same way - the same
flaw already fixed in the philips component.
Each platform now defines its own USE_LEVOIT_* macro. Rather than scatter
guards through switch-cases, the entity classes are confined to levoit.cpp
behind new accessors - try_get_number_state(), try_get_select_index(),
try_get_switch_state() and apply_fan_status() - so the *_commands.cpp and
*_status.cpp sources no longer reference those types at all. Each accessor
returns false when the entity is absent or has no state, preserving the
existing "leave the default alone" behaviour at every call site.
The 1.5.0 changelog entry claimed the USE_LEVOIT_* guards were already in
place. They were not - that was only ever done for philips - so the entry is
corrected here.
Verified: all three test configs compile, and Core 200S/400S, Vital 200S and
Sprout device configs still build.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review findings on #68, all four real: - apply_fan_status() took (bool, uint8_t, uint32_t), but the Vital and Superior decoders deliberately pass -1 for "not reported in this frame, leave the field alone". Narrowing turned an unknown power into ON and an unknown speed into 255, which then clamps to maximum - so a partial status frame would switch the fan on at full speed. Takes (int, int, int) now, matching LevoitFan::apply_device_status(). - The rewrite of vital_status.cpp dropped the `self != nullptr` check at that call site, even though decode_vital_status() accepts a nullable self and guards its other entity updates. Restored. - Guarding the fan include was not enough on its own: track_cadr_usage() and calculate_current_cadr_per_hour() still dereferenced fan_ against a forward-declared type. Both are now inside USE_LEVOIT_FAN, with the latter returning 0 when there is no fan entity to derive a duty cycle from. The first two were regressions I introduced in the previous commit; only the third was pre-existing. Verified: all three test configs compile, plus Core 400S and Vital 200S. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Mirrors the levoit suite for philips: every entity type, a fan-only config, and one where template entities supply the platforms. That last one is the regression test for a bug that was real - before the USE_PHILIPS_* guards, an AC0650 config with an unrelated template switch failed with "fatal error: switch/philips_switch.h: No such file". Nothing was guarding that fix until now. All three pass on the first run, so the philips guards were already correct. The workflow becomes a component x config matrix, so both components build on every PR - six jobs instead of three. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rrection Review finding: test-all-entities.yaml omitted the light platform, so USE_LEVOIT_LIGHT and the Sprout LED-ring codegen went untested. The generator built the entity list from each platform's TYPE_MAP and light has none - it is a single light_schema entity - so it was skipped silently. Listed explicitly now, with a comment saying why. All nine platforms that define a USE_LEVOIT_* macro are exercised. The changelog correction in this branch targeted the root README, but #69 moved the levoit change log to components/levoit/README.md in the meantime, so the rebase dropped it and the inaccurate claim survived at its new location. Re-applied there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
tuct
force-pushed
the
test/levoit-compile-tests-ci
branch
from
September 26, 2026 16:29
71e8bdd to
5104f88
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #67, which merged before this landed.
Compile tests
Three self-contained configs in
components/levoit/tests/. They need nosecrets.yaml(inline dummy credentials) and point at../.., so they always build the working copy.test-all-entities.yamltest-fan-only.yamlUSE_LEVOIT_*guardtest-foreign-platforms.yamltemplateentities define the genericUSE_SWITCHetc. while no levoit platform directory is copiedCI
.github/workflows/compile-tests.ymlruns them on every PR touchingcomponents/, as afail-fast: falsematrix with PlatformIO/pip caching. This is the repo's first workflow.Two real bugs they found immediately
A levoit config with only a fan did not build at all.
core_commands.cpp,sprout_commands.cpp,superior_commands.cppandsuperior_status.cppincludednumber/levoit_number.h/sensor/levoit_sensor.hunguarded;core_status.cppandvital_status.cppdid the same withfan/levoit_fan.h. ESPHome only copieslevoit/<platform>/when a levoit entity of that platform is configured, so those headers were absent.levoit.cppguarded on the generic macros —USE_SWITCH,USE_NUMBER, … are defined by any component using that platform, so a config with atemplateswitch but no levoit switch failed the same way. Same flaw previously fixed in thephilipscomponent.Fix
Each platform now defines its own
USE_LEVOIT_*macro. Rather than scatter#ifdefthrough switch-cases, the entity classes are confined tolevoit.cppbehind new accessors —try_get_number_state(),try_get_select_index(),try_get_switch_state(),apply_fan_status()— so*_commands.cppand*_status.cppno longer reference those types at all. Each returnsfalsewhen the entity is absent or has no state, preserving the existing "leave the default alone" behaviour at every call site.Changelog correction
The 1.5.0 entry claimed the
USE_LEVOIT_*guards were already in place. They were not — that had only ever been done forphilips— so the entry is corrected to describe what actually shipped.Verified
All three test configs compile, and the Core 200S / Core 400S / Vital 200S / Sprout device configs still build.
🤖 Generated with Claude Code