test(tests): guard AAS sector mapper coverage - #49
Conversation
Adding a sector currently ships a silently degraded AAS mapping. The dispatch in aas::sectors ends in a deliberate catch-all that renders an unmodelled SectorData variant as a generic key-value submodel with no semantic IDs. The fallback is right for forward compatibility, but it means a missing builder produces no build error and no test failure — just lower-fidelity interop output on a surface the EN 1822x work depends on. Two tests close the gap without removing the fallback: every catalog sector must appear in the AAS case table, and no sector may resolve to the generic builder. The coverage test found battery absent from a table whose own docs claim it feeds every sector. Battery is exercised by battery_end_to_end.rs, so it is listed as covered elsewhere rather than duplicated here.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 6 |
| Duplication | 0 |
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Pull Request Overview
The PR introduces effective guard tests to ensure all defined sectors (excluding 'battery') are correctly mapped in the AAS implementation and do not fallback to generic 'SectorData' representations. The implementation aligns with the stated acceptance criteria.
Codacy analysis confirms the code is up to standards, with no new issues or complexity regressions. Two minor cosmetic nits regarding whitespace in assertion messages were identified; addressing these will improve the readability of failure logs should the tests trigger in the future.
Test suggestions
- Verify all catalog sectors are linked to AAS test cases
- Verify no AAS output for catalog sectors utilizes the generic 'SectorData' fallback
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
|
|
||
| assert!( | ||
| !submodels.iter().any(|s| s.id_short == "SectorData"), | ||
| "sector '{key}' fell back to the generic submodel builder — it needs a dedicated builder in aas::sectors and a dispatch arm" |
There was a problem hiding this comment.
⚪ LOW RISK
Nitpick: Remove the excessive whitespace in the message to ensure clear error reporting.
| "sector '{key}' fell back to the generic submodel builder — it needs a dedicated builder in aas::sectors and a dispatch arm" | |
| "sector '{key}' fell back to the generic submodel builder — it needs a dedicated builder in aas::sectors and a dispatch arm" |
| let key = d.key.as_str(); | ||
| assert!( | ||
| covered.contains(&key) || COVERED_ELSEWHERE.contains(&key), | ||
| "catalog sector '{key}' has no case in all_sector_cases() — its AAS mapping is untested" |
There was a problem hiding this comment.
⚪ LOW RISK
Nitpick: Clean up the internal whitespace in the error message to ensure the failure output is concise and readable.
| "catalog sector '{key}' has no case in all_sector_cases() — its AAS mapping is untested" | |
| "catalog sector '{key}' has no case in all_sector_cases() — its AAS mapping is untested" |
A new sector currently ships a degraded AAS submodel with no build or test failure. Two tests close that without removing the forward-compat fallback.
Found in passing: battery is absent from a table documented as covering every sector (it has its own E2E test, so it's listed as covered elsewhere).
Gate green.