Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes the UPnP discovery mocking in the test_discovery_wrapper test to properly prevent network access during unit testing. The changes add a socket forbid guardrail and corresponding error handling.
Changes:
- Added mock for
UpnpDiscoveryGatewayto prevent real UPnP discovery during testing - Added socket forbid guardrail to fail fast if the test accidentally attempts network access
- Added error handling in
upnp_gateway.pyto gracefully handle socket errors (including mock-raised errors)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/test_compat_wrappers.py | Updated test to mock both mDNS and UPnP gateways, added socket forbid guardrail, and improved import/naming clarity |
| src/devialetctl/infrastructure/upnp_gateway.py | Added try-except to handle OSError on socket.sendto, allowing graceful degradation when sockets are unavailable |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| monkeypatch.setattr("devialetctl.infrastructure.upnp_gateway.socket.socket", _forbid_socket) | ||
| monkeypatch.setattr(discovery_module, "MdnsDiscoveryGateway", FakeMdnsGateway) | ||
| monkeypatch.setattr(discovery_module, "UpnpDiscoveryGateway", lambda: FakeUpnpGateway()) |
There was a problem hiding this comment.
The UpnpDiscoveryGateway mock uses an inconsistent pattern compared to MdnsDiscoveryGateway. Line 63 patches MdnsDiscoveryGateway with the class directly (FakeMdnsGateway), but this line wraps FakeUpnpGateway in a lambda. For consistency and correctness, it should be: monkeypatch.setattr(discovery_module, "UpnpDiscoveryGateway", FakeUpnpGateway) without the lambda wrapper.
| monkeypatch.setattr(discovery_module, "UpnpDiscoveryGateway", lambda: FakeUpnpGateway()) | |
| monkeypatch.setattr(discovery_module, "UpnpDiscoveryGateway", FakeUpnpGateway) |
1 participant
No description provided.