|
| 1 | +--- |
| 2 | +name: python-test-reviewer |
| 3 | +description: |
| 4 | + Expert guidance for reviewing Python tests in the Matter (connectedhomeip) |
| 5 | + repository. Use this skill when reviewing changes to tests, specifically |
| 6 | + targeting common pitfalls in async execution, mocking cluster interactions, |
| 7 | + and assertion quality. |
| 8 | +--- |
| 9 | + |
| 10 | +# Python Test Reviewer Skill |
| 11 | + |
| 12 | +This skill provides a checklist and set of principles for reviewing Python tests |
| 13 | +within the Matter repository. |
| 14 | + |
| 15 | +## Core Review Principles |
| 16 | + |
| 17 | +- **Prefer Real Interactions Over Mocks**: For integration tests, prioritize |
| 18 | + using the real Matter stack (via fixtures) unless the interaction is too |
| 19 | + complex or slow. |
| 20 | +- **Explicit Assertions**: Never accept tests that "pass by not crashing." |
| 21 | + Every test must have at least one clear, meaningful assertion. |
| 22 | +- **Async Hygiene**: Matter is heavily asynchronous. Ensure async patterns are |
| 23 | + used correctly. |
| 24 | + |
| 25 | +## Specific Checkpoints |
| 26 | + |
| 27 | +### 1. Asyncio Usage |
| 28 | + |
| 29 | +- **Check**: Are all awaitable calls (like `devCtrl.ReadAttribute`) actually |
| 30 | + awaited? |
| 31 | +- **Propagate Exceptions in Parallel Code**: When using concurrent.futures or |
| 32 | + similar parallel execution mechanisms, ensure that exceptions thrown in |
| 33 | + background threads are not silently ignored. Call .result() on futures to |
| 34 | + propagate errors. |
| 35 | + |
| 36 | +### 2. Mocking Cluster Interactions |
| 37 | + |
| 38 | +- **Guideline**: When mocking cluster responses, ensure the mock structure |
| 39 | + matches the `chip.clusters` attributes or commands exactly. |
| 40 | +- **Pitfall**: Avoid mocking the entire `ChipDeviceCtrl` if you only need to |
| 41 | + mock a single attribute read. |
| 42 | + |
| 43 | +### 3. Code Style |
| 44 | + |
| 45 | +- **Re-use**: Avoid re-writing code that is present in matter/testing or the |
| 46 | + chip core libraries. If you are creating a class or function that is |
| 47 | + substantially similar to existing classes or functions, consider whether |
| 48 | + extension would be preferable to duplication |
| 49 | +- **Attribute reads**: Prefer using read_single_attribute_check_success in |
| 50 | + matter/testing to the base Read and ReadAttribute functions in |
| 51 | + ChipDeviceCtrl.py unless there is a good reason. This function contains |
| 52 | + additional checks that are useful for testing. |
| 53 | +- **Exceptions**: Avoid `except Exception` - use a specific exception |
| 54 | +- **try / except can hide bugs**: Avoid fixing CI issues with a try: except:. |
| 55 | + This pattern can hide real bugs that should be fixed and is a code smell. |
| 56 | + Try except blocks should only be used where the exception is expected and |
| 57 | + that should normally include an assertion that the exception happens or a |
| 58 | + comment explaining in detail why the exception is thrown and how this is |
| 59 | + spec compliant. |
| 60 | +- **Sleeps can hide bugs**: a sleep in a test is a code smell. Ensure sleeps |
| 61 | + in tests are actually required. If the test is using sleep to wait for the |
| 62 | + device to do something, consider whether a subscription can be used instead. |
| 63 | +- **Tolerances should be well considered**: If there is a fudge factor built |
| 64 | + in, ensure the logic is both explained and reasonable. |
| 65 | +- **Use dataclass**: Do not use dicts with string keys. Use dataclasses with |
| 66 | + named members instead. This helps programmers avoid uncaught typos. |
| 67 | +- **Avoid Decorative Separators**: Remove long lines of hashes (e.g., |
| 68 | + ###########) or other decorative separators. Use standard spacing or |
| 69 | + docstrings instead. |
| 70 | +- **Cleanup of Stale Code**: Remove outdated TODO comments, unbound variables, |
| 71 | + and leftover debugging code. |
| 72 | +- **Type Hints**: Use type hints for function signatures to improve |
| 73 | + readability and maintainability, especially for functions that are part of |
| 74 | + the test framework or are likely to be reused. |
| 75 | +- **TODOs**: TODOs should be linked to a tracking issue and used sparingly. |
| 76 | + |
| 77 | +### 4. Matter-Specific Patterns |
| 78 | + |
| 79 | +- **Commissioning**: If the test involves commissioning, ensure it uses the |
| 80 | + standard commissioning fixtures to avoid duplicated setup logic. |
| 81 | +- **IDs**: IDs and values should be taken from the codegen files (ex. |
| 82 | + Objects.py). Values that are not in the codegen should be flagged for follow |
| 83 | + up |
| 84 | + |
| 85 | +### 5. Testing failures |
| 86 | + |
| 87 | +- **Test for unexpected successes**: If a test is meant to verify that an |
| 88 | + error condition is properly handled, ensure that it fails if the error |
| 89 | + condition does not occur. |
| 90 | + |
| 91 | +## Feedback Style |
| 92 | + |
| 93 | +- Be concise. |
| 94 | +- Reference specific lines. |
| 95 | +- If a pattern is repeated across many tests, suggest a helper function or |
| 96 | + fixture. |
0 commit comments