-
Notifications
You must be signed in to change notification settings - Fork 151
feat: Created tests for endpoint.py to increase test coverage. #1306
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,119 @@ | ||||||||||||||
| import pytest | ||||||||||||||
| from unittest.mock import MagicMock | ||||||||||||||
| from src.hiero_sdk_python.address_book.endpoint import Endpoint | ||||||||||||||
|
|
||||||||||||||
| pytestmark = pytest.mark.unit | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| def test_getter_setter(): # type: ignore | ||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial Clarify or remove the The |
||||||||||||||
|
|
||||||||||||||
| """Test for Endpoint constructor, getters, and setters with fluent interface.""" | ||||||||||||||
|
|
||||||||||||||
| endpoint = Endpoint(address=None, port=None, domain_name=None) | ||||||||||||||
|
|
||||||||||||||
| # Test fluent interface (method chaining) | ||||||||||||||
| result = endpoint.set_address(b'127.0.1.1') | ||||||||||||||
| assert result is endpoint, "set_address should return self for method chaining" | ||||||||||||||
|
|
||||||||||||||
| result = endpoint.set_port(77777) | ||||||||||||||
| assert result is endpoint, "set_port should return self for method chaining" | ||||||||||||||
|
|
||||||||||||||
| result = endpoint.set_domain_name("redpanda.com") | ||||||||||||||
| assert result is endpoint, "set_domain_name should return self for method chaining" | ||||||||||||||
|
|
||||||||||||||
| # Protect against breaking changes - verify attributes exist | ||||||||||||||
| assert hasattr(endpoint, 'get_address'), "Missing get_address method" | ||||||||||||||
| assert hasattr(endpoint, 'get_port'), "Missing get_port method" | ||||||||||||||
| assert hasattr(endpoint, 'get_domain_name'), "Missing get_domain_name method" | ||||||||||||||
|
|
||||||||||||||
| assert endpoint.get_address() == b'127.0.1.1' | ||||||||||||||
| assert endpoint.get_port() == 77777 | ||||||||||||||
| assert endpoint.get_domain_name() == "redpanda.com" | ||||||||||||||
|
Comment on lines
+8
to
+31
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial Consider adding input validation tests. While this test covers the happy path well, consider adding test cases for input validation to ensure setters and constructors handle invalid inputs appropriately:
This aligns with PRIORITY 2 (Constructor & Setter Behavior) and PRIORITY 3 (Comprehensive Coverage) in the coding guidelines. Example test structuredef test_set_address_type_validation():
"""Test that set_address validates input type."""
endpoint = Endpoint()
with pytest.raises((TypeError, ValueError)):
endpoint.set_address("not bytes") # Should fail if type checking exists
def test_set_port_boundary_conditions():
"""Test port number boundaries."""
endpoint = Endpoint()
# Test negative port
with pytest.raises((ValueError, TypeError)):
endpoint.set_port(-1)
# Test port > 65535
with pytest.raises((ValueError, TypeError)):
endpoint.set_port(65536) |
||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| def test_constructor_with_values(): | ||||||||||||||
| """Test Endpoint constructor with actual values.""" | ||||||||||||||
| endpoint = Endpoint(address=b'192.168.1.1', port=8080, domain_name="example.com") | ||||||||||||||
|
|
||||||||||||||
| assert endpoint.get_address() == b'192.168.1.1' | ||||||||||||||
| assert endpoint.get_port() == 8080 | ||||||||||||||
| assert endpoint.get_domain_name() == "example.com" | ||||||||||||||
|
Comment on lines
+34
to
+40
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial LGTM - Good constructor test coverage. This test effectively validates the constructor with real values. For consistency with breaking change protection, consider adding a return type assertion: assert isinstance(endpoint, Endpoint), "Constructor should return Endpoint instance" |
||||||||||||||
|
|
||||||||||||||
| @pytest.mark.parametrize("input_port, expected_port", [ | ||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix The first argument to 🔎 Proposed fix-@pytest.mark.parametrize("input_port, expected_port", [
+@pytest.mark.parametrize(("input_port", "expected_port"), [
(0, 50211), # Default port mapping when port is 0
(50111, 50211), # Legacy port 50111 maps to new port 50211
(80, 80) # Standard ports pass through unchanged
])📝 Committable suggestion
Suggested change
🧰 Tools🪛 Ruff (0.14.10)42-42: Wrong type passed to first argument of Use a (PT006) |
||||||||||||||
| (0, 50211), # Default port mapping when port is 0 | ||||||||||||||
| (50111, 50211), # Legacy port 50111 maps to new port 50211 | ||||||||||||||
| (80, 80) # Standard ports pass through unchanged | ||||||||||||||
| ]) | ||||||||||||||
|
|
||||||||||||||
| def test_from_proto_port_mapping(input_port, expected_port): | ||||||||||||||
| """Tests the logic that converts a Protobuf ServiceEndpoint into an Endpoint object.""" | ||||||||||||||
|
|
||||||||||||||
| mock_proto = MagicMock() | ||||||||||||||
| mock_proto.port = input_port | ||||||||||||||
| mock_proto.ipAddressV4 = b"127.0.1.1" | ||||||||||||||
| mock_proto.domain_name = "redpanda.com" | ||||||||||||||
|
|
||||||||||||||
| endpoint = Endpoint._from_proto(mock_proto) | ||||||||||||||
|
|
||||||||||||||
| # Verify port mapping | ||||||||||||||
| assert endpoint.get_port() == expected_port | ||||||||||||||
|
|
||||||||||||||
| # Verify all fields are mapped correctly | ||||||||||||||
| assert endpoint.get_address() == b"127.0.1.1", "Address should be mapped from proto" | ||||||||||||||
| assert endpoint.get_domain_name() == "redpanda.com", "Domain name should be mapped from proto" | ||||||||||||||
|
|
||||||||||||||
| # Protect against breaking changes - verify return type | ||||||||||||||
| assert isinstance(endpoint, Endpoint), "Should return Endpoint instance" | ||||||||||||||
|
|
||||||||||||||
| def test_to_proto(): | ||||||||||||||
|
|
||||||||||||||
| '''Verifies that an Endpoint instance can be correctly serialized back into | ||||||||||||||
| a Protobuf ServiceEndpoint object with all fields intact.''' | ||||||||||||||
|
|
||||||||||||||
| endpoint = Endpoint(address=b'127.0.1.1', port=77777, domain_name="redpanda.com") | ||||||||||||||
| proto = endpoint._to_proto() | ||||||||||||||
| assert proto.ipAddressV4 == b'127.0.1.1' | ||||||||||||||
| assert proto.port == 77777 | ||||||||||||||
| assert proto.domain_name == "redpanda.com" | ||||||||||||||
|
Comment on lines
+68
to
+77
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial LGTM - Good serialization test. This test properly validates that |
||||||||||||||
|
|
||||||||||||||
| def test_str(): | ||||||||||||||
|
|
||||||||||||||
| """Tests the human-readable string representation of the Endpoint.""" | ||||||||||||||
|
|
||||||||||||||
| endpoint = Endpoint(address=b'127.0.1.1', port=77777, domain_name="redpanda.com") | ||||||||||||||
| result = str(endpoint) | ||||||||||||||
|
|
||||||||||||||
| # Verify return type | ||||||||||||||
| assert isinstance(result, str), "String representation should return a string" | ||||||||||||||
| assert result == '127.0.1.1:77777' | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| def test_str_with_none_values(): | ||||||||||||||
| """Test string representation when address or port is None.""" | ||||||||||||||
| endpoint = Endpoint(address=b"", port=None, domain_name="example.com") | ||||||||||||||
| result = str(endpoint) | ||||||||||||||
|
|
||||||||||||||
| assert isinstance(result, str), "Should return string even with None values" | ||||||||||||||
| # Verify it doesn't raise an exception and returns something meaningful | ||||||||||||||
| assert result is not None and len(result) > 0, "Should produce non-empty string representation" | ||||||||||||||
|
Comment on lines
+91
to
+98
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Split compound assertion and clarify expected behavior. Line 98 contains a compound assertion that makes debugging harder when tests fail. Additionally, the assertion for "something meaningful" is vague and doesn't specify the expected format. 🔎 Proposed fix- assert result is not None and len(result) > 0, "Should produce non-empty string representation"
+ assert result is not None, "Should return a string, not None"
+ assert len(result) > 0, "Should produce non-empty string representation"
+ # Be more specific about expected format if possible
+ assert ":" in result, "String representation should contain ':' separator"🧰 Tools🪛 Ruff (0.14.10)98-98: Assertion should be broken down into multiple parts (PT018) |
||||||||||||||
|
|
||||||||||||||
| def test_from_dict_error(): | ||||||||||||||
|
|
||||||||||||||
| '''Validates 'Guard Clause' error handling''' | ||||||||||||||
|
|
||||||||||||||
| invalid_data = {"port": 77777} | ||||||||||||||
| with pytest.raises(ValueError, match="JSON data must contain"): | ||||||||||||||
| Endpoint.from_dict(invalid_data) | ||||||||||||||
|
Comment on lines
+100
to
+106
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial Good error handling test - consider additional invalid input cases. This test properly validates the guard clause for missing required keys. Consider adding tests for other invalid scenarios:
|
||||||||||||||
|
|
||||||||||||||
| def test_from_dict_success(): | ||||||||||||||
| ''' Tests successful creation of an Endpoint from a dictionary (JSON format) ''' | ||||||||||||||
| data = { | ||||||||||||||
| "ip_address_v4": "127.0.0.1", | ||||||||||||||
| "port": 77777, | ||||||||||||||
| "domain_name": "redpanda.com" | ||||||||||||||
| } | ||||||||||||||
| endpoint = Endpoint.from_dict(data) | ||||||||||||||
|
|
||||||||||||||
| assert endpoint.get_address() == b"127.0.0.1" | ||||||||||||||
| assert endpoint.get_port() == 77777 | ||||||||||||||
| assert endpoint.get_domain_name() == "redpanda.com" | ||||||||||||||
|
Comment on lines
+108
to
+119
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial LGTM - Successful dictionary conversion test. This test effectively validates the assert isinstance(endpoint, Endpoint), "from_dict should return Endpoint instance" |
||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: Remove trailing space.
Static analysis detected a trailing space at the end of this line.
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
10-10: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)