test: add architectural and schema conformance test suite - #955
Open
allenporter wants to merge 6 commits into
Open
test: add architectural and schema conformance test suite#955allenporter wants to merge 6 commits into
allenporter wants to merge 6 commits into
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Moderate issues leave reflection coverage and trait boundary enforcement incomplete.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds reflection-based conformance tests for models, enums, traits, and exceptions.
Changes:
- Added shared discovery utilities and package marker.
- Added architectural and schema conformance checks.
- Added legacy enum fallback baselines.
File summaries
| File | Summary |
|---|---|
tests/conformance/test_trait_boundary_conformance.py |
Checks trait constructor boundaries; type-based transport dependencies may bypass the checks. |
tests/conformance/test_model_conformance.py |
Validates RoborockBase inheritance for models. |
tests/conformance/test_exception_conformance.py |
Validates exception hierarchy rooting. |
tests/conformance/test_enum_conformance.py |
Checks enum fallbacks; plain enums may be missed and the fixed sentinel may be valid. |
tests/conformance/discovery.py |
Provides reflection helpers; import and attribute errors can silently skip modules. |
tests/conformance/__init__.py |
Defines the conformance test package. |
Review details
Suppressed comments (2)
tests/conformance/test_enum_conformance.py:87
- These collections only discover classes that already inherit from
RoborockEnumorRoborockModeEnum. A new wire/status enum declared as a plainEnum,IntEnum, orStrEnumis invisible to all three checks, so the suite cannot enforce the stated requirement that wire enums use the resilient bases. Add discovery or an explicit allowlist for wire code-mapping enums, while excluding intentional UI/domain enums.
_ALL_ROBOROCK_ENUMS = discover_subclasses(roborock, RoborockEnum, exclude=(RoborockEnum,))
_ALL_MODE_ENUMS = discover_subclasses(roborock, RoborockModeEnum, exclude=(RoborockModeEnum,))
tests/conformance/test_enum_conformance.py:98
99999is not guaranteed to be unknown: a future enum can legitimately add that firmware code, making this assertion exercise a valid member and stop testing the fallback. Derive a sentinel outside the enum's current values instead.
assert enum_cls(99999) == enum_cls.unknown
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
Summary
This PR introduces an automated conformance test suite in
tests/conformance/using package reflection to enforce architectural invariants and prevent recurring regressions previously caught manually during PR reviews.Related to Issue #953
Each conformance test directly targets patterns discussed in past code reviews (particularly with @Lash-L and @allenporter):
1. Data Model Serialization &
RoborockBaseInheritance (test_model_conformance.py)RoborockBase(which provides.as_dict(), camelCase conversion, and.from_dict()integration). When models are defined using plain@dataclass, standard serialization helpers fail or necessitate ad-hoc dictionary conversion workarounds.c981220("fix: subclass Q10Room and Q10Point from RoborockBase") by @allenporter:Q10RoomandQ10Pointwere originally plain@dataclasswithoutRoborockBase, breaking.as_dict()and requiring a_to_camel_dicthack inMapContentTraituntil refactored.roborock.dataand ensures every declared domain@dataclassinherits fromRoborockBase.2. Wire Enum Fallback Resilience (
test_enum_conformance.py)RoborockEnumsubclasses provide anunknownfallback member, tested against dynamically derived sentinels (max(val) + 1) outside the enum's range.RoborockModeEnumsubclasses implementfrom_code_optional()orfrom_code(), tested against dynamic sentinels.*code_mappings*modules inherit fromRoborockEnumorRoborockModeEnum(with 4 intentional command/domain exceptions allowlisted).unknownusing@pytest.mark.xfail(strict=True)so new enums must handle fallbacks without breaking existing legacy enums.3. Trait Channel & Transport Boundaries (
test_trait_boundary_conformance.py)RoborockChannel/ RPC dispatchers), rather than taking direct references to transport sockets, encryption keys, tokens, or IP addresses.b01_q10_channel, with traits consuming clean domain bytes.__init__constructor parameter names to ensure no transport/credential parameters (socket,token,local_key,ip,key, etc.) are injected.socket.socket,asyncio.BaseTransport,LocalChannel,LocalChannelParams,MqttParams,MqttSession).4. Exception Hierarchy Rooting (
test_exception_conformance.py)RoborockExceptionso caller reconnection and retry loops catch errors reliably without leaking unexpected standard exceptions.a8b96a8("feat: implement RoborockParsingException for trait responses"): Resolved unhandled standardValueErrors escaping callerexcept RoborockException:retry blocks.roborock.exceptionsand across the library inherit fromRoborockException.5. Robust Package Discovery (
discovery.py)walk_modules()imports modules directly without swallowingImportErrororAttributeError, guaranteeing that syntax or module initialization errors fail fast in CI rather than causing modules to be silently skipped by reflection.Verification
uv run pytest tests/conformance/ -v: 280 passed, 53 xfaileduv run pytest: 1,263 passed, 53 xfailed, 92 snapshots passeduv run pre-commit run --all-files: all passed (ruff, mypy, codespell, check-yaml, etc.)