Introduction of network.ports utility #88
Conversation
|
Warning Rate limit exceeded@richtja has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 52 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds a new network ports utility module with functions and a PortTracker class. Updates tests by adding unit and functional suites for the module. Adjusts docs to include the new network ports section and restructures some headings. Introduces metadata for the module. Expands disabled Pylint warnings for tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (10)
metadata/network.yml (1)
17-17: Potential typo in test filename.The test filename appears to have a typo:
network_funcional.pyshould likely benetwork_functional.py(missing 't').- - tests/modules/network/network_funcional.py + - tests/modules/network/network_functional.pytests/modules/network/network_funcional.py (1)
87-89: Remove redundant skip decoratorThe nested
@unittest.skipUnlessdecorator is redundant since the entire class is already skipped when not running in CI. Additionally,/sys/class/netshould always exist on Linux systems with networking support.- @unittest.skipUnless( - os.path.isdir("/sys/class/net"), "No network interfaces available" - ) def test_localhost_interfaces(self):autils/network/hosts.py (1)
165-167: Simplify return statementThe conditional return can be simplified to return the boolean result directly.
- if re.search(check, mac_id): - return True - return False + return bool(re.search(check, mac_id))tests/modules/network/network_unit.py (2)
212-212: Add comment to clarify intentional property accessThese statements are intentionally accessing properties to test exception handling. Add a comment to clarify this is not a mistake.
- local_host.interfaces # pylint: disable=pointless-statement + # Intentionally access property to trigger exception + local_host.interfaces # pylint: disable=pointless-statementAlso applies to: 357-357
96-110: Remove unused function get_all_local_addrsThis function is defined but never used in the test file. Consider removing it to reduce code clutter.
autils/network/interfaces.py (5)
216-218: Fix awkward f-string formattingThe f-string is split in a way that makes it harder to read.
- raise NWException( - f"Slave interface not found for " f"the bond {self.name}" - ) from exc + raise NWException( + f"Slave interface not found for the bond {self.name}" + ) from exc
334-336: Fix f-string formattingThe f-string should be on a single line for better readability.
- cmd = ( - f"ip link add link {self.name} name {vlan_name} " f"type vlan id {vlan_num}" - ) + cmd = f"ip link add link {self.name} name {vlan_name} type vlan id {vlan_num}"
845-847: Simplify return statementReturn the condition directly instead of using if/else.
- if "l-lan" in output: - return True - return False + return "l-lan" in output
862-864: Simplify return statementReturn the condition directly instead of using if/else.
- if "vnic" in output: - return True - return False + return "vnic" in output
879-882: Use any() for cleaner codeReplace the for loop with
any()for a more Pythonic approach.- for vpd in output.split(): - if "VF" in vpd and vpd.endswith("SN"): - return True - return False + return any("VF" in vpd and vpd.endswith("SN") for vpd in output.split())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.pylintrc_tests(1 hunks)autils/network/common.py(1 hunks)autils/network/exceptions.py(1 hunks)autils/network/hosts.py(1 hunks)autils/network/interfaces.py(1 hunks)autils/network/ports.py(1 hunks)docs/source/utils.rst(1 hunks)metadata/network.yml(1 hunks)tests/modules/network/network_funcional.py(1 hunks)tests/modules/network/network_unit.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/modules/network/network_unit.py (5)
autils/network/hosts.py (9)
interfaces(70-90)Host(33-190)LocalHost(193-213)get_interface_by_ipaddr(92-106)get_interface_by_hwaddr(108-123)get_all_hwaddr(125-145)validate_mac_addr(148-167)get_default_route_interface(169-190)RemoteHost(216-322)autils/network/ports.py (9)
PortTracker(156-221)is_port_free(60-72)register_port(179-193)release_port(214-221)_reset_retained_ports(175-177)find_free_port(76-107)find_free_port(195-212)is_port_available(29-57)find_free_ports(111-153)autils/network/common.py (1)
run_command(11-38)autils/network/exceptions.py (1)
NWException(9-10)autils/network/interfaces.py (38)
get_ipaddrs(454-478)get_hwaddr(480-494)NetworkInterface(60-1080)config_filename(90-117)slave_config_filename(144-168)_get_interface_details(170-196)set_hwaddr(257-273)bring_up(399-412)bring_down(382-397)get_mtu(496-509)ping_check(511-532)set_mtu(676-697)validate_ipv4_format(953-976)validate_ipv4_netmask_format(979-1015)netmask_to_cidr(938-950)add_ipaddr(275-295)remove_ipaddr(699-720)flush_ipaddr(722-738)is_admin_link_up(414-427)is_operational_link_up(429-442)is_available(798-813)is_bond(815-830)is_link_up(444-452)vlans(298-317)add_vlan_tag(319-340)remove_vlan_by_tag(342-365)remove_all_vlans(367-380)save(535-674)are_packets_lost(906-935)_get_bondinterface_details(198-218)_move_file_to_backup(220-238)remove_link(765-781)nm_flush_ipaddr(740-763)restore_from_backup(783-796)restore_slave_cfg_file(889-904)remove_cfg_file(884-887)ping_flood(1018-1054)get_device_IPI_name(1056-1080)
autils/network/interfaces.py (2)
autils/network/common.py (1)
run_command(11-38)autils/network/exceptions.py (1)
NWException(9-10)
🪛 Ruff (0.12.2)
tests/modules/network/network_unit.py
132-135: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
212-212: Found useless expression. Either assign it to a variable or remove it.
(B018)
357-357: Found useless expression. Either assign it to a variable or remove it.
(B018)
488-493: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
531-534: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
642-647: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
651-656: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
906-909: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
913-914: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
autils/network/ports.py
71-71: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
autils/network/interfaces.py
845-847: Return the condition "l-lan" in output directly
Replace with return "l-lan" in output
(SIM103)
862-864: Return the condition "vnic" in output directly
Replace with return "vnic" in output
(SIM103)
879-882: Use return any("VF" in vpd and vpd.endswith("SN") for vpd in output.split()) instead of for loop
Replace with return any("VF" in vpd and vpd.endswith("SN") for vpd in output.split())
(SIM110)
autils/network/hosts.py
65-65: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
165-167: Return the condition bool(re.search(check, mac_id)) directly
Replace with return bool(re.search(check, mac_id))
(SIM103)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: api-stability-tests
- GitHub Check: modules-tests
🔇 Additional comments (9)
.pylintrc_tests (1)
81-81: LGTM: Appropriate test configuration adjustment.Disabling the
protected-accesswarning for test code is a reasonable approach when testing internal behaviors of the new network utilities. This allows comprehensive testing without linting interruptions.autils/network/exceptions.py (1)
1-11: LGTM: Well-designed exception hierarchy foundation.The base exception class follows Python best practices with clear documentation and appropriate inheritance from
Exception. This provides a solid foundation for network utility error handling.docs/source/utils.rst (3)
4-6: LGTM: Appropriate documentation addition.The new "Ar" subsection properly documents the archive utility module.
12-34: LGTM: Well-organized network utilities documentation.The new "Network" section with its subsections (Common, Exceptions, Hosts, Interfaces, Ports) provides clear documentation structure for the network utilities modules. The Sphinx automodule directives are correctly formatted.
35-41: LGTM: Logical reorganization of path documentation.Moving the path documentation under the "File" section provides better organizational structure and is more intuitive than the previous placement.
autils/network/common.py (1)
11-39: LGTM: Well-documented function with comprehensive interface.The function provides a clean abstraction for command execution across local and remote hosts, with detailed documentation and consistent return types. The sudo support and error handling documentation are appropriate.
autils/network/hosts.py (2)
284-286: Review redundant connection logic in enterThe connection check and reconnection attempt seems redundant since
_connect()is already called in__init__. If the connection fails in__init__, an exception is raised and the object isn't created. Consider if this reconnection logic is really needed.
65-66: Use isinstance() for type checkingUse
isinstance()instead of direct type comparison for better practice and to handle inheritance properly.- if type(self) == Host: # pylint: disable=C0123 + if isinstance(self, type(self)):Actually, the better approach here is:
- if type(self) == Host: # pylint: disable=C0123 + if self.__class__ is Host:Likely an incorrect or invalid review comment.
tests/modules/network/network_unit.py (1)
159-159: Import path forsystem_outputis correct
Theprocessmodule is brought in viafrom avocado.utils import process(e.g. inautils/network/interfaces.pyandautils/network/common.py), so mockingavocado.utils.process.system_outputmatches the actual structure. No changes required.
richtja
force-pushed
the
network_utility
branch
from
13b47c6 to
c76d5a1
Compare
richtja
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #88 +/- ##
===========================================
+ Coverage 0 100.00% +100.00%
===========================================
Files 0 1 +1
Lines 0 61 +61
===========================================
+ Hits 0 61 +61 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
autils/network/ports.py (3)
71-71: Add stacklevel to deprecation warningThe deprecation warning is missing the
stacklevel=2parameter, causing it to point to this function instead of the caller's location.- warnings.warn("deprecated, use is_port_available() instead.", DeprecationWarning) + warnings.warn("deprecated, use is_port_available() instead.", DeprecationWarning, stacklevel=2)
189-189: Replace deprecated is_port_free with is_port_availableThe PortTracker class still uses the deprecated
is_port_freefunction internally, which should be replaced withis_port_available.- if (port not in self.retained_ports) and is_port_free(port, self.address): + if (port not in self.retained_ports) and is_port_available(port, self.address):
209-209: Replace deprecated is_port_free with is_port_availableThe PortTracker class still uses the deprecated
is_port_freefunction internally, which should be replaced withis_port_available.- while (port in self.retained_ports) or (not is_port_free(port, self.address)): + while (port in self.retained_ports) or (not is_port_available(port, self.address)):
🧹 Nitpick comments (1)
tests/modules/network/ports_unit.py (1)
105-111: Combine nested with statements for better readabilityThe nested
withstatements can be combined into a single statement as suggested by the static analysis tool.def test_is_port_free_deprecation_warning(self): - with self.assertWarns(DeprecationWarning): - with unittest.mock.patch( - "autils.network.ports.is_port_available" - ) as mock_is_port_available: + with ( + self.assertWarns(DeprecationWarning), + unittest.mock.patch("autils.network.ports.is_port_available") as mock_is_port_available, + ): ports.is_port_free(22, "localhost") mock_is_port_available.assert_called_once_with(22, "localhost")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/modules-tests.yml(1 hunks).pylintrc_tests(1 hunks)autils/network/ports.py(1 hunks)docs/source/utils.rst(1 hunks)metadata/network/ports.yml(1 hunks)tests/modules/network/ports_funcional.py(1 hunks)tests/modules/network/ports_unit.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- metadata/network/ports.yml
- .github/workflows/modules-tests.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- .pylintrc_tests
- docs/source/utils.rst
🧰 Additional context used
🪛 Ruff (0.12.2)
tests/modules/network/ports_unit.py
106-109: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
autils/network/ports.py
71-71: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
🔇 Additional comments (9)
tests/modules/network/ports_funcional.py (3)
1-26: LGTM - Well-structured functional test setupThe module docstring clearly explains the purpose and scope of functional tests. The conditional skipping based on CI environment is appropriate for resource-intensive network tests.
28-54: LGTM - Comprehensive PortTracker functional testsThe test class effectively validates the core PortTracker functionality:
- Port registration and release flow
- Borg pattern shared state behavior
- Proper test isolation with setUp method
The tests complement the unit tests by exercising real network operations.
56-81: LGTM - Effective utility function testingThe PortsTest class provides solid validation of the utility functions with real socket operations:
- Confirms free ports can actually be bound to
- Verifies bound ports are correctly detected as unavailable
- Proper use of socket context manager for resource cleanup
tests/modules/network/ports_unit.py (1)
87-126: LGTM - Comprehensive utility function testsThe PortsTest class provides excellent coverage of the utility functions:
- Tests normal operation and error conditions
- Validates deprecation warning behavior
- Uses appropriate mocking to isolate functionality
- Good edge case coverage
autils/network/ports.py (5)
17-26: LGTM - Imports and constants properly definedThe Borg import path has been correctly updated from the past review, and the network constants are well-defined.
29-57: LGTM - Robust port availability checkingThe function properly handles socket binding with appropriate exception handling for permission and OS errors. The implementation is correct and well-documented.
76-153: LGTM - Well-implemented port finding functionsBoth functions are properly implemented with:
- Comprehensive parameter documentation
- Correct delegation pattern
- Proper handling of sequential vs random search
- Good use of the non-deprecated
is_port_availablefunction
156-177: LGTM - Proper Borg pattern implementationThe PortTracker class correctly implements the Borg pattern with:
- Proper shared state initialization
- Sensible default values
- Clean string representation method
- Conditional attribute initialization
214-221: LGTM - Clean port release implementationThe
release_portmethod correctly removes ports from the retained list with proper existence checking.
richtja
force-pushed
the
network_utility
branch
2 times, most recently
from
a2a03fd to
9ca731a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
tests/modules/network/ports_unit.py (1)
34-40: Fix the mock setup in release_port testThe test is incorrectly mocking the method it's trying to test. This replaces
tracker.release_portwith a mock, so the assertion at line 39 just verifies the mock was called, not the actual method behavior.def test_release_port_does_not_poke_system(self): tracker = ports.PortTracker() - tracker.release_port = unittest.mock.MagicMock() + tracker.retained_ports = [22] ports.is_port_free = unittest.mock.MagicMock() tracker.release_port(22) - tracker.release_port.assert_called_once_with(22) + self.assertNotIn(22, tracker.retained_ports) ports.is_port_free.assert_not_called()autils/network/ports.py (3)
71-71: Add stacklevel to deprecation warningInclude
stacklevel=2in the warning to show it at the caller's location rather than this function.- warnings.warn("deprecated, use is_port_available() instead.", DeprecationWarning) + warnings.warn("deprecated, use is_port_available() instead.", DeprecationWarning, stacklevel=2)
189-189: Replace deprecated is_port_free with is_port_availableThe
PortTrackerclass internally uses the deprecatedis_port_freefunction. It should useis_port_availableinstead.- if (port not in self.retained_ports) and is_port_free(port, self.address): + if (port not in self.retained_ports) and is_port_available(port, self.address):
209-209: Replace deprecated is_port_free with is_port_availableThe
PortTrackerclass internally uses the deprecatedis_port_freefunction. It should useis_port_availableinstead.- while (port in self.retained_ports) or (not is_port_free(port, self.address)): + while (port in self.retained_ports) or (not is_port_available(port, self.address)):
🧹 Nitpick comments (2)
tests/modules/network/ports_funcional.py (1)
15-15: Remove unused importThe
osmodule is imported but never used in this file.-import ostests/modules/network/ports_unit.py (1)
106-111: Combine nested with statementsThe nested
withstatements can be combined for better readability.- with self.assertWarns(DeprecationWarning): - with unittest.mock.patch( - "autils.network.ports.is_port_available" - ) as mock_is_port_available: + with self.assertWarns(DeprecationWarning), unittest.mock.patch( + "autils.network.ports.is_port_available" + ) as mock_is_port_available: ports.is_port_free(22, "localhost") mock_is_port_available.assert_called_once_with(22, "localhost")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.pylintrc_tests(1 hunks)autils/network/ports.py(1 hunks)docs/source/utils.rst(1 hunks)metadata/network/ports.yml(1 hunks)tests/modules/network/ports_funcional.py(1 hunks)tests/modules/network/ports_unit.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- .pylintrc_tests
- docs/source/utils.rst
- metadata/network/ports.yml
🧰 Additional context used
🪛 Ruff (0.12.2)
tests/modules/network/ports_funcional.py
15-15: os imported but unused
Remove unused import: os
(F401)
autils/network/ports.py
71-71: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
tests/modules/network/ports_unit.py
106-109: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: api-stability-tests
- GitHub Check: modules-tests
🔇 Additional comments (6)
tests/modules/network/ports_funcional.py (2)
22-47: LGTM - Well-structured functional testsThe
PortTrackerTestclass provides good coverage of the core functionality including port registration/release and the Borg pattern verification. The use of_reset_retained_ports()is appropriate for test isolation even though it accesses a private method.
49-69: LGTM - Comprehensive port availability testingThe
PortsTestclass effectively validates both positive and negative scenarios for port availability checking using real socket operations. The tests properly demonstrate that ports reported as free can actually be bound to.tests/modules/network/ports_unit.py (2)
6-85: LGTM - Comprehensive unit test coverageThe
PortTrackerTestclass provides thorough testing of thePortTrackerfunctionality including port registration, release, free port finding, and Borg pattern behavior. The use of mocks appropriately isolates the system calls.
87-126: LGTM - Good utility function testingThe
PortsTestclass effectively tests the standalone utility functions with proper error handling and deprecation warning verification.autils/network/ports.py (2)
29-57: LGTM - Robust port availability checkingThe
is_port_availablefunction properly handles bothPermissionErrorandOSErrorexceptions, providing a reliable way to check port availability across different scenarios.
156-221: LGTM - Well-implemented PortTracker classThe
PortTrackerclass correctly implements the Borg pattern for shared state management and provides a clean API for port tracking. The implementation properly handles port registration, finding free ports, and releasing ports.
richtja
force-pushed
the
network_utility
branch
from
9ca731a to
d189b30
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
tests/modules/network/ports_unit.py (1)
34-40: Fix the mock setup in release_port testThe test is incorrectly mocking the method it's trying to test. This replaces
tracker.release_portwith a mock, so the assertion at line 39 just verifies the mock was called, not the actual method behavior.def test_release_port_does_not_poke_system(self): tracker = ports.PortTracker() - tracker.release_port = unittest.mock.MagicMock() + tracker.retained_ports = [22] ports.is_port_free = unittest.mock.MagicMock() tracker.release_port(22) - tracker.release_port.assert_called_once_with(22) + self.assertNotIn(22, tracker.retained_ports) ports.is_port_free.assert_not_called()autils/network/ports.py (3)
71-71: Add stacklevel to deprecation warningInclude
stacklevel=2in the warning to show it at the caller's location rather than this function.- warnings.warn("deprecated, use is_port_available() instead.", DeprecationWarning) + warnings.warn("deprecated, use is_port_available() instead.", DeprecationWarning, stacklevel=2)
189-189: Replace deprecated is_port_free with is_port_availableThe
PortTrackerclass internally uses the deprecatedis_port_freefunction. It should useis_port_availableinstead.- if (port not in self.retained_ports) and is_port_free(port, self.address): + if (port not in self.retained_ports) and is_port_available(port, self.address):
209-209: Replace deprecated is_port_free with is_port_availableThe
PortTrackerclass internally uses the deprecatedis_port_freefunction. It should useis_port_availableinstead.- while (port in self.retained_ports) or (not is_port_free(port, self.address)): + while (port in self.retained_ports) or (not is_port_available(port, self.address)):
🧹 Nitpick comments (1)
tests/modules/network/ports_unit.py (1)
105-111: Combine nested with statements for cleaner code.The test logic is correct but can be simplified by combining the nested
withstatements.def test_is_port_free_deprecation_warning(self): - with self.assertWarns(DeprecationWarning): - with unittest.mock.patch( - "autils.network.ports.is_port_available" - ) as mock_is_port_available: + with ( + self.assertWarns(DeprecationWarning), + unittest.mock.patch("autils.network.ports.is_port_available") as mock_is_port_available, + ): ports.is_port_free(22, "localhost") mock_is_port_available.assert_called_once_with(22, "localhost")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.pylintrc_tests(1 hunks)autils/network/ports.py(1 hunks)docs/source/utils.rst(1 hunks)metadata/network/ports.yml(1 hunks)tests/modules/network/ports_funcional.py(1 hunks)tests/modules/network/ports_unit.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- .pylintrc_tests
- docs/source/utils.rst
- tests/modules/network/ports_funcional.py
- metadata/network/ports.yml
🧰 Additional context used
🪛 Ruff (0.12.2)
autils/network/ports.py
71-71: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
tests/modules/network/ports_unit.py
106-109: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: modules-tests
- GitHub Check: api-stability-tests
🔇 Additional comments (13)
tests/modules/network/ports_unit.py (8)
7-13: LGTM!The test properly mocks the external dependency and verifies both the function call and state change.
15-27: LGTM!Both error condition tests properly verify exception handling for ports in use and already retained ports.
28-32: LGTM!The test correctly verifies that
release_portremoves the port from the retained list.
42-67: LGTM!The
find_free_porttest methods comprehensively cover different scenarios including default behavior, explicit start port, and incrementing when ports are retained.
69-77: LGTM!The test correctly verifies the Borg pattern behavior where multiple instances share state.
79-84: LGTM!The test correctly verifies the string representation format of the PortTracker.
88-103: LGTM!The
is_port_availabletests properly cover normal operation and error handling for both OSError and PermissionError exceptions.
113-125: LGTM!The remaining test methods provide good coverage for
find_free_portandfind_free_portsfunctions, including edge cases like no available ports.autils/network/ports.py (5)
17-26: LGTM!The imports and constants are well-defined, and the Borg import path has been correctly updated.
29-57: LGTM!The
is_port_availablefunction is well-implemented with proper error handling, resource management using context managers, and comprehensive documentation.
76-153: LGTM!Both
find_free_portandfind_free_portsfunctions are well-implemented with comprehensive parameter handling, good documentation, and proper delegation tois_port_available.
156-178: LGTM!The PortTracker class initialization and helper methods are well-implemented with proper Borg pattern usage and good documentation.
195-221: LGTM!The
find_free_portandrelease_portmethods are well-implemented with proper logic for port management and good documentation.
richtja
force-pushed
the
network_utility
branch
from
d189b30 to
44a9625
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
autils/network/ports.py (3)
71-71: Add stacklevel to deprecation warning.The deprecation warning should include
stacklevel=2to point to the caller's location rather than this function's location.- warnings.warn("deprecated, use is_port_available() instead.", DeprecationWarning) + warnings.warn("deprecated, use is_port_available() instead.", DeprecationWarning, stacklevel=2)
179-193: Fix usage of deprecated function in register_port.The
register_portmethod uses the deprecatedis_port_freefunction instead ofis_port_available.- if (port not in self.retained_ports) and is_port_free(port, self.address): + if (port not in self.retained_ports) and is_port_available(port, self.address):
195-212: Fix usage of deprecated function in find_free_port.The
find_free_portmethod uses the deprecatedis_port_freefunction instead ofis_port_available.- while (port in self.retained_ports) or (not is_port_free(port, self.address)): + while (port in self.retained_ports) or (not is_port_available(port, self.address)):tests/modules/network/ports_unit.py (1)
34-41: Fix the mock setup in release_port test.The test incorrectly mocks the method it's trying to test, which prevents testing the actual behavior.
def test_release_port_does_not_poke_system(self): tracker = ports.PortTracker() - tracker.release_port = unittest.mock.MagicMock() + tracker.retained_ports = [22] ports.is_port_free = unittest.mock.MagicMock() tracker.release_port(22) - tracker.release_port.assert_called_once_with(22) + self.assertNotIn(22, tracker.retained_ports) ports.is_port_free.assert_not_called()
🧹 Nitpick comments (1)
tests/modules/network/ports_unit.py (1)
106-111: Consider simplifying nested with statements.The nested
withstatements can be combined for better readability.- def test_is_port_free_deprecation_warning(self): - with self.assertWarns(DeprecationWarning): - with unittest.mock.patch( - "autils.network.ports.is_port_available" - ) as mock_is_port_available: - ports.is_port_free(22, "localhost") - mock_is_port_available.assert_called_once_with(22, "localhost") + def test_is_port_free_deprecation_warning(self): + with ( + self.assertWarns(DeprecationWarning), + unittest.mock.patch( + "autils.network.ports.is_port_available" + ) as mock_is_port_available, + ): + ports.is_port_free(22, "localhost") + mock_is_port_available.assert_called_once_with(22, "localhost")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.pylintrc_tests(1 hunks)autils/network/ports.py(1 hunks)docs/source/utils.rst(1 hunks)metadata/network/ports.yml(1 hunks)tests/modules/network/ports_funcional.py(1 hunks)tests/modules/network/ports_unit.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- .pylintrc_tests
- docs/source/utils.rst
- metadata/network/ports.yml
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/modules/network/ports_funcional.py (1)
autils/network/ports.py (6)
PortTracker(156-221)_reset_retained_ports(175-177)find_free_port(76-107)find_free_port(195-212)release_port(214-221)is_port_available(29-57)
tests/modules/network/ports_unit.py (1)
autils/network/ports.py (9)
PortTracker(156-221)is_port_free(60-72)register_port(179-193)release_port(214-221)_reset_retained_ports(175-177)find_free_port(76-107)find_free_port(195-212)is_port_available(29-57)find_free_ports(111-153)
🪛 Ruff (0.12.2)
autils/network/ports.py
71-71: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
tests/modules/network/ports_unit.py
106-109: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: api-stability-tests
- GitHub Check: modules-tests
🔇 Additional comments (16)
autils/network/ports.py (6)
1-26: LGTM! Good module structure and documentation.The file header, imports, and constants are well-organized. The module docstring clearly describes the purpose, and the defined families and protocols constants provide good reference points for the supported socket types.
29-57: LGTM! Robust port availability checking.The
is_port_availablefunction correctly handles bothPermissionErrorandOSErrorexceptions, providing a conservative approach by returningFalsewhen uncertain. The use of context manager ensures proper socket cleanup.
76-107: LGTM! Well-implemented port finding logic.The
find_free_portfunction properly delegates tofind_free_portsand handles the case where no ports are found by returningNone. The extensive parameter documentation is helpful.
111-153: LGTM! Efficient port range scanning.The
find_free_portsfunction implements a good strategy with optional randomization and early termination when enough ports are found. The logic is sound and well-documented.
156-178: LGTM! Proper Borg pattern implementation.The
PortTrackerclass correctly implements the Borg pattern with proper initialization and state management. The__str__method provides useful debugging information.
214-221: LGTM! Clean port release implementation.The
release_portmethod properly removes the port from the retained list with appropriate safety checking.tests/modules/network/ports_funcional.py (4)
1-19: LGTM! Excellent functional test documentation.The module docstring clearly explains the purpose and scope of functional tests, distinguishing them from unit tests and explaining the rationale for testing real OS interactions.
21-46: LGTM! Well-designed PortTracker functional tests.The tests properly validate the core PortTracker functionality:
- Port registration and release with real state tracking
- Borg pattern state sharing between instances
- Proper cleanup in
setUpmethodThe test logic is sound and tests real behavior without mocks.
48-68: LGTM! Effective port availability validation tests.The tests validate real port behavior:
- Actually binding to a port reported as free
- Confirming that bound ports are reported as unavailable
- Using OS-allocated ports (port 0) for reliable testing
The test design ensures real socket interactions are working correctly.
70-72: LGTM! Standard test runner setup.The
if __name__ == "__main__"block follows standard Python testing conventions.tests/modules/network/ports_unit.py (6)
1-4: LGTM! Clean imports and setup.The imports are minimal and appropriate for unit testing with mocks.
6-27: LGTM! Comprehensive PortTracker registration tests.The tests properly validate:
- Successful port registration when port is free
- Failure when port is in use
- Failure when port is already retained
The mock usage correctly isolates the unit under test.
28-33: LGTM! Simple and effective release test.The test correctly validates that
release_portremoves the port from the retained list.
42-85: LGTM! Thorough PortTracker behavior tests.The tests effectively validate:
- Free port discovery starting from default and custom start ports
- Port incrementing when retained ports exist
- Borg pattern state sharing between instances
- String representation functionality
The mock usage properly isolates dependencies while testing real behavior.
87-126: LGTM! Comprehensive port utility tests.The tests properly validate:
- Basic port availability checking
- Error handling for OSError and PermissionError
- Deprecation warning for
is_port_free- Port finding functionality with edge cases
The mock usage correctly simulates various conditions.
128-130: LGTM! Standard test runner setup.The
if __name__ == "__main__"block follows standard Python testing conventions.
richtja
force-pushed
the
network_utility
branch
from
44a9625 to
db4483b
Compare
This is a migration of network.ports utility from avocado.utils.network.ports to AAutils. Reference: avocado-framework#76, https://github.com/avocado-framework/avocado/blob/master/avocado/utils/network/ports.py Signed-off-by: Jan Richter <jarichte@redhat.com>
Some pylint checks don't make sense in the terms of testing. Signed-off-by: Jan Richter <jarichte@redhat.com>
richtja
force-pushed
the
network_utility
branch
from
db4483b to
016323c
Compare
github-project-automation
bot
moved this from Review Requested
to Done 112
in Default project
Since the network.ports development has been migrated to AAutils we need to inform users and developers about this change. Reference: avocado-framework/aautils#88 Signed-off-by: Jan Richter <jarichte@redhat.com>
Since the network.ports development has been migrated to AAutils we need to inform users and developers about this change. Reference: avocado-framework/aautils#88 Signed-off-by: Jan Richter <jarichte@redhat.com>
Since the network.ports development has been migrated to AAutils we need to inform users and developers about this change. Reference: avocado-framework/aautils#88 Signed-off-by: Jan Richter <jarichte@redhat.com>
Since the network.ports development has been migrated to AAutils we need to inform users and developers about this change. Reference: avocado-framework/aautils#88 Signed-off-by: Jan Richter <jarichte@redhat.com>
2 participants
This is a migration of network.ports utility from avocado.utils.path to AAutils.
Reference: #76,
https://github.com/avocado-framework/avocado/tree/master/avocado/utils/network
Summary by CodeRabbit
New Features
Documentation
Tests
Chores