python-test-reviewer

# Python Test Reviewer Skill This skill provides a checklist and set of principles for reviewing Python tests within the Matter repository. ## Core Review Principles - **Prefer Real Interactions Over Mocks**: For integration tests, prioritize using the real Matter stack (via fixtures) unless the interaction is too complex or slow. - **Explicit Assertions**: Never accept tests that "pass by not crashing." Every test must have at least one clear, meaningful assertion. - **Async Hygiene**: Matter is heavily asynchronous. Ensure async patterns are used correctly. ## Specific Checkpoints ### 1. Asyncio Usage - **Check**: Are all awaitable calls (like `devCtrl.ReadAttribute`) actually awaited? - **Propagate Exceptions in Parallel Code**: When using concurrent.futures or similar parallel execution mechanisms, ensure that exceptions thrown in background threads are not silently ignored. Call .result() on futures to propagate errors. ### 2. Mocking Cluster Interactions - **Guideline**: When mocking cluster responses, ensure the mock structure matches the `chip.clusters` attributes or commands exactly. - **Pitfall**: Avoid mocking the entire `ChipDeviceCtrl` if you only need to mock a single attribute read. ### 3. Code Style - **Re-use**: Avoid re-writing code that is present in matter/testing or the chip core libraries. If you are creating a class or function that is substantially similar to existing classes or functions, consider whether extension would be preferable to duplication - **Attribute reads**: Prefer using read_single_attribute_check_success in matter/testing to the base Read and ReadAttribute functions in ChipDeviceCtrl.py unless there is a good reason. This function contains additional checks that are useful for testing. - **Exceptions**: Avoid `except Exception` - use a specific exception - **try / except can hide bugs**: Avoid fixing CI issues with a try: except:. This pattern can hide real bugs that should be fixed and is a code smell. Try except blocks should only be used where the exception is expected and that should normally include an assertion that the exception happens or a comment explaining in detail why the exception is thrown and how this is spec compliant. - **Sleeps can hide bugs**: a sleep in a test is a code smell. Ensure sleeps in tests are actually required. If the test is using sleep to wait for the device to do something, consider whether a subscription can be used instead. - **Tolerances should be well considered**: If there is a fudge factor built in, ensure the logic is both explained and reasonable. - **Use dataclass**: Do not use dicts with string keys. Use dataclasses with named members instead. This helps programmers avoid uncaught typos. - **Avoid Decorative Separators**: Remove long lines of hashes (e.g., ###########) or other decorative separators. Use standard spacing or docstrings instead. - **Cleanup of Stale Code**: Remove outdated TODO comments, unbound variables, and leftover debugging code. - **Type Hints**: Use type hints for function signatures to improve readability and maintainability, especially for functions that are part of the test framework or are likely to be reused. - **TODOs**: TODOs should be linked to a tracking issue and used sparingly. ### 4. Matter-Specific Patterns - **Commissioning**: If the test involves commissioning, ensure it uses the standard commissioning fixtures to avoid duplicated setup logic. - **IDs**: IDs and values should be taken from the codegen files (ex. Objects.py). Values that are not in the codegen should be flagged for follow up ### 5. Testing failures - **Test for unexpected successes**: If a test is meant to verify that an error condition is properly handled, ensure that it fails if the error condition does not occur. ## Feedback Style - Be concise. - Reference specific lines. - If a pattern is repeated across many tests, suggest a helper function or fixture.