Open
Conversation
- Use pytest.importorskip() for hypothesis in test_mqtt_hypothesis.py so that a missing hypothesis install skips the tests rather than breaking all test collection - Bump awsiotsdk minimum from >=1.27.0 to >=1.28.2 (latest patch release); awscrt 0.31.3 is pulled in transitively Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- handlers.py: anti-legionella set-period else branch called enable_anti_legionella() in both branches; else now correctly calls disable_anti_legionella() to preserve the disabled state - __main__.py: _detect_unit_system() returned None on TimeoutError violating its UnitSystemType return type; now returns 'us_customary' matching the warning message - events.py: once-listener detection in emit() used _once_callbacks set lookup which deduplicates by (event, callback); if the same callback was registered twice with once=True the second listener would become permanent after first emit; now uses listener.once directly - subscriptions.py: resubscribe_all() cleared internal state before re-subscribing; failed topics were permanently lost from memory; now restores failed entries so they are retried on next reconnection - factory.py: if NavienAPIClient/NavienMqttClient constructors raised after successful auth.__aenter__(), the auth session leaked; now wrapped in try/except with proper cleanup - tests/test_mqtt_hypothesis.py: add noqa: E402 to imports following pytest.importorskip() to satisfy ruff line-order check
Contributor
There was a problem hiding this comment.
Pull request overview
Routine maintenance updates across the dependency surface, test collection behavior, and several runtime correctness fixes in the CLI, MQTT subscription manager, event emitter, and client factory.
Changes:
- Bump
awsiotsdkminimum version and ensure Hypothesis-based tests skip cleanly when Hypothesis isn’t installed. - Fix multiple logic issues (anti-legionella CLI state handling, subscription resubscribe state restoration, unit-system timeout return value, once-listener handling).
- Prevent auth-session leakage when client construction fails after auth context entry.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
tests/test_mqtt_hypothesis.py |
Avoids pytest collection failure by skipping the file when Hypothesis is missing. |
src/nwp500/mqtt/subscriptions.py |
Restores failed topics/handlers into internal state so resubscribe can be retried on next reconnect. |
src/nwp500/factory.py |
Ensures auth context is exited if API/MQTT client construction raises after successful __aenter__(). |
src/nwp500/events.py |
Fixes once-listener semantics by using the listener’s once flag instead of a deduplicating set lookup. |
src/nwp500/cli/handlers.py |
Adjusts anti-legionella set-period logic to avoid always enabling the feature (but see review comment). |
src/nwp500/cli/__main__.py |
Returns a concrete UnitSystemType on timeout instead of None. |
setup.cfg |
Updates minimum awsiotsdk requirement to >=1.28.2. |
You can also share your feedback on Copilot code review. Take the survey.
- Revert pytest.importorskip() for hypothesis: hypothesis is already mandated via setup.cfg [testing] extras and installed by tox; the skip pattern was masking a misconfigured environment rather than solving the root cause - Anti-Legionella set-period: when the feature is disabled, disable_anti_legionella() takes no period_days argument so the period was silently not updated while printing a success message. Now informs the user that the period can only be set while the feature is enabled and directs them to 'anti-legionella enable'
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
Routine maintenance pass covering two dependency updates, a broken test collection issue, and five logic bugs found during a code review.
Dependency & Test Infrastructure
awsiotsdk minimum version bumped to 1.28.2
The previously declared minimum (>=1.27.0) was behind the current patch release. Bumped to >=1.28.2, which also pulls in awscrt 0.31.3 transitively.
Hypothesis test no longer breaks test collection
tests/test_mqtt_hypothesis.py imported hypothesis at module level. When hypothesis was not installed, pytest failed to collect all tests — not just that file. Replaced the bare import with pytest.importorskip("hypothesis") so the tests skip gracefully instead.
Bug Fixes
Anti-Legionella set-period always enabled the feature (cli/handlers.py)
Both if/else branches of the enabled-state check called enable_anti_legionella(). A device with anti-legionella disabled would be silently re-enabled when the user ran nwp-cli anti-legionella set-period. The else branch now calls disable_anti_legionella() to preserve the current state.
Subscriptions permanently lost after failed resubscription (mqtt/subscriptions.py)
resubscribe_all() cleared _subscriptions and _message_handlers before the re-subscribe loop. Any topic that failed to resubscribe was dropped from internal state entirely and could never be retried on the next reconnection. Failed topics are now restored to internal state so they are retried on the next reconnect.
_detect_unit_system() returned None on timeout (cli/main.py)
The function declared return type UnitSystemType but returned None on TimeoutError. Now returns "us_customary" to match the warning message and the type contract.
Once-listener could become permanent with duplicate callbacks (events.py)
emit() identified once-listeners via a set of (event, callback) tuples. If the same callback was registered twice with once=True, the set deduplicated the tuple — after the first emit, the second listener lost its once-status and became permanent. Fixed by checking listener.once directly on the EventListener object instead.
Auth session leaked if client construction raised (factory.py)
In create_navien_clients(), the NavienAPIClient and NavienMqttClient constructors were called after auth_client.aenter() but outside its cleanup guard. If either constructor raised, the auth session and its underlying aiohttp session would leak. Client construction is now wrapped in its own try/except that calls auth_client.aexit() on failure.
Test Results