Improving snapshot (current handling and expansion) #894
Conversation
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
|
Note Reviews pausedUse the following commands to manage reviews:
WalkthroughA new snapshot-based test for the Plugwise integration was introduced, including a supporting shell script for running the test in isolation. The core testing script was updated to implement a two-step snapshot validation and update process, improving error handling and messaging during snapshot testing. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant snap Script
participant ha-core/venv
participant pytest
participant test_setup.py
User->>snap Script: Run snap
snap Script->>ha-core/venv: Activate virtualenv
snap Script->>pytest: Run pytest on test_setup.py
pytest->>test_setup.py: Execute Plugwise registry snapshot test
test_setup.py->>pytest: Return test results
pytest->>snap Script: Output results
snap Script->>ha-core/venv: Deactivate virtualenv
snap Script->>User: Return to original directory
sequenceDiagram
participant core-testing.sh
participant pytest
core-testing.sh->>pytest: Run pytest without --snapshot-update
pytest-->>core-testing.sh: Return test result
alt Test fails
core-testing.sh->>pytest: Run pytest with --snapshot-update
pytest-->>core-testing.sh: Return test result
alt Test fails again
core-testing.sh->>core-testing.sh: Print failure (not snapshot-related)
core-testing.sh->>core-testing.sh: Exit with error
else Test passes
core-testing.sh->>core-testing.sh: Print success (snapshots updated)
end
else Test passes
core-testing.sh->>core-testing.sh: Print success (snapshots valid)
end
Suggested labels
Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai pause |
CoMPaTech
commented
Jul 2, 2025
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
scripts/core-testing.sh (1)
248-263: Fix exit statement and consider simplifying the complex command chaining.The two-step snapshot testing approach is excellent, but there are some issues with the current implementation:
- Line 255: The
exit 1statement within the command chain may not behave as expected- Complex nesting: The multiple levels of
||and&&make the logic hard to follow and maintain- Duplicate success messages: Both branches print the same success message
Consider this cleaner approach:
- # First test if snapshots still valid (silent fail, otherwise will update snapshots) - PYTEST_COMMAND="pytest ${debug_params} ${subject} tests/components/${REPO_NAME}/${basedir} --cov=homeassistant/components/${REPO_NAME}/ --cov-report term-missing" - eval "${PYTEST_COMMAND}" || { - echo "" - echo -e "${CFAIL}Pytest / Snapshot validation failed, re-running to update snapshot ...${CNORM}" - eval "${PYTEST_COMMAND} --snapshot-update" || { - echo "" - echo -e "${CFAIL}Pytest failed, not a snapshot issue ...${CNORM}" || exit 1 - } && { - echo "" - echo -e "${CINFO}Pytest / Snapshot validation passed" - } - } && { - echo "" - echo -e "${CINFO}Pytest / Snapshot validation passed" - } + # First test if snapshots still valid (silent fail, otherwise will update snapshots) + PYTEST_COMMAND="pytest ${debug_params} ${subject} tests/components/${REPO_NAME}/${basedir} --cov=homeassistant/components/${REPO_NAME}/ --cov-report term-missing" + if eval "${PYTEST_COMMAND}"; then + echo "" + echo -e "${CINFO}Pytest / Snapshot validation passed${CNORM}" + else + echo "" + echo -e "${CFAIL}Pytest / Snapshot validation failed, re-running to update snapshot ...${CNORM}" + if eval "${PYTEST_COMMAND} --snapshot-update"; then + echo "" + echo -e "${CINFO}Pytest / Snapshot update successful${CNORM}" + else + echo "" + echo -e "${CFAIL}Pytest failed, not a snapshot issue ...${CNORM}" + exit 1 + fi + fitests/components/plugwise/test_setup.py (2)
4-4: Fix inline comment spacing.Inline comments need at least two spaces before the
#symbol according to PEP 8.Apply this diff:
-import logging # For potential debugging +import logging # For potential debugging -_LOGGER = logging.getLogger(__name__) # For logging +_LOGGER = logging.getLogger(__name__) # For logging - clean_name_part = mock_config_entry.domain # Or mock_config_entry.title.lower().replace(" ", "_") + clean_name_part = mock_config_entry.domain # Or mock_config_entry.title.lower().replace(" ", "_") - "identifiers": sorted([list(i) for i in device_entry.identifiers]), # Sort nested lists - "connections": sorted([list(c) for c in device_entry.connections]), # Sort nested lists + "identifiers": sorted([list(i) for i in device_entry.identifiers]), # Sort nested lists + "connections": sorted([list(c) for c in device_entry.connections]), # Sort nested listsAlso applies to: 16-16, 48-48, 87-88
54-54: Remove excessive blank lines.Reduce the number of consecutive blank lines to follow PEP 8 guidelines (maximum of 2 blank lines).
Apply this diff:
- - entity_registry = er.async_get(hass) - - device_registry = dr.async_get(hass)Also applies to: 76-76
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
scripts/core-testing.sh(1 hunks)snap(1 hunks)tests/components/plugwise/test_setup.py(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: bouwew
PR: plugwise/plugwise-beta#793
File: custom_components/plugwise/manifest.json:10-11
Timestamp: 2024-12-12T19:48:34.652Z
Learning: In the Plugwise integration, temporary use of alpha versions from test repositories is acceptable for testing purposes.
Learnt from: CoMPaTech
PR: plugwise/plugwise-beta#0
File: :0-0
Timestamp: 2024-09-18T12:03:37.508Z
Learning: Be mindful of upstream PRs (e.g., HA-Core PRs) that address similar issues, and consider discussing them in reviews.
Learnt from: CoMPaTech
PR: plugwise/plugwise-beta#0
File: :0-0
Timestamp: 2024-10-15T22:02:21.046Z
Learning: Be mindful of upstream PRs (e.g., HA-Core PRs) that address similar issues, and consider discussing them in reviews.
Learnt from: bouwew
PR: plugwise/plugwise-beta#671
File: custom_components/plugwise/switch.py:82-83
Timestamp: 2024-10-08T15:35:25.180Z
Learning: User bouwew prefers to ignore exception handling suggestions for the `pw-beta` branch in the `custom_components/plugwise/switch.py` file.
Learnt from: bouwew
PR: plugwise/plugwise-beta#671
File: custom_components/plugwise/switch.py:82-83
Timestamp: 2024-06-22T08:05:56.791Z
Learning: User bouwew prefers to ignore exception handling suggestions for the `pw-beta` branch in the `custom_components/plugwise/switch.py` file.
Learnt from: CoMPaTech
PR: plugwise/plugwise-beta#0
File: :0-0
Timestamp: 2025-01-25T20:42:24.862Z
Learning: The linting rules in plugwise-beta have been updated to use TC001/TC002/TC003 instead of TCH001/TCH002/TCH003 for type-checking related rules.
Learnt from: CoMPaTech
PR: plugwise/plugwise-beta#0
File: :0-0
Timestamp: 2025-01-25T20:42:24.862Z
Learning: In the plugwise-beta project, `uv` package manager was removed from core-testing.sh in favor of pip due to issues it was causing.
Learnt from: bouwew
PR: plugwise/plugwise-beta#734
File: tests/components/plugwise/test_config_flow.py:58-58
Timestamp: 2024-09-27T09:48:30.890Z
Learning: In `tests/components/plugwise/test_config_flow.py`, the versions are mocked firmware versions for testing purposes and do not need to match the software version in `manifest.json`.
Learnt from: bouwew
PR: plugwise/plugwise-beta#734
File: tests/components/plugwise/test_config_flow.py:58-58
Timestamp: 2024-10-08T15:35:25.180Z
Learning: In `tests/components/plugwise/test_config_flow.py`, the versions are mocked firmware versions for testing purposes and do not need to match the software version in `manifest.json`.
Learnt from: CoMPaTech
PR: plugwise/plugwise-beta#789
File: .pre-commit-config.yaml:3-3
Timestamp: 2024-12-08T11:23:47.692Z
Learning: CoMPaTech considers Python 3.13 stable and prefers to use it in the project.
snap (3)
Learnt from: CoMPaTech
PR: plugwise/plugwise-beta#0
File: :0-0
Timestamp: 2025-01-25T20:42:24.862Z
Learning: In the plugwise-beta project, `uv` package manager was removed from core-testing.sh in favor of pip due to issues it was causing.
Learnt from: bouwew
PR: plugwise/plugwise-beta#734
File: tests/components/plugwise/test_config_flow.py:58-58
Timestamp: 2024-09-27T09:48:30.890Z
Learning: In `tests/components/plugwise/test_config_flow.py`, the versions are mocked firmware versions for testing purposes and do not need to match the software version in `manifest.json`.
Learnt from: bouwew
PR: plugwise/plugwise-beta#734
File: tests/components/plugwise/test_config_flow.py:58-58
Timestamp: 2024-10-08T15:35:25.180Z
Learning: In `tests/components/plugwise/test_config_flow.py`, the versions are mocked firmware versions for testing purposes and do not need to match the software version in `manifest.json`.
tests/components/plugwise/test_setup.py (2)
Learnt from: bouwew
PR: plugwise/plugwise-beta#734
File: tests/components/plugwise/test_config_flow.py:58-58
Timestamp: 2024-09-27T09:48:30.890Z
Learning: In `tests/components/plugwise/test_config_flow.py`, the versions are mocked firmware versions for testing purposes and do not need to match the software version in `manifest.json`.
Learnt from: bouwew
PR: plugwise/plugwise-beta#734
File: tests/components/plugwise/test_config_flow.py:58-58
Timestamp: 2024-10-08T15:35:25.180Z
Learning: In `tests/components/plugwise/test_config_flow.py`, the versions are mocked firmware versions for testing purposes and do not need to match the software version in `manifest.json`.
🪛 GitHub Check: CodeFactor
snap
[notice] 2-2: snap#L2
Use a ( subshell ) to avoid having to cd back. (SC2103)
🪛 Ruff (0.11.9)
tests/components/plugwise/test_setup.py
20-20: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
26-26: Trailing whitespace
Remove trailing whitespace
(W291)
🪛 Flake8 (7.2.0)
tests/components/plugwise/test_setup.py
[error] 16-16: at least two spaces before inline comment
(E261)
[error] 24-24: continuation line missing indentation or outdented
(E122)
[error] 48-48: at least two spaces before inline comment
(E261)
[error] 54-54: too many blank lines (2)
(E303)
[error] 76-76: too many blank lines (2)
(E303)
[error] 87-87: at least two spaces before inline comment
(E261)
[error] 88-88: at least two spaces before inline comment
(E261)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Prepare
🔇 Additional comments (1)
tests/components/plugwise/test_setup.py (1)
28-94: Excellent snapshot test implementation!The test design is very well thought out:
- Comprehensive coverage: Tests both entity and device registries
- Proper parameterization: Supports different mock configurations and environments
- Consistent snapshots: Proper sorting ensures reproducible results
- Clear snapshot naming: Uses descriptive names that include environment and cooling status
- Appropriate assertions: Correctly validates config entry loading before snapshot comparisons
The approach of extracting key attributes from both entity and device registries and comparing them against snapshots is excellent for regression testing.
✅ Actions performedReviews paused. |
|
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.
Transferring #893 from module to this repository. Code is not testable/working just to keep track of things (initial changes to
scripts/core-testing.share considered improvements!)Summary by CodeRabbit
New Features
Chores