feat: unit test for 'endpoint.py' #1290
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #1290 +/- ##
==========================================
+ Coverage 91.79% 91.97% +0.18%
==========================================
Files 139 139
Lines 8466 8484 +18
==========================================
+ Hits 7771 7803 +32
+ Misses 695 681 -14 🚀 New features to boost your workflow:
|
📝 WalkthroughWalkthroughAdds a new unit test file targeting src/hiero_sdk_python/address_book/endpoint.py and updates CHANGELOG.md to record the test addition. Tests exercise setters/getters, proto (de)serialization, port-mapping, string formatting, and from_dict error/success scenarios. Changes
Sequence Diagram(s)(Skipped — changes are test additions and do not introduce new multi-component control flow.) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)tests/unit/**/*⚙️ CodeRabbit configuration file
Files:
🧬 Code graph analysis (1)tests/unit/endpoint_test.py (1)
⏰ 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). (1)
🔇 Additional comments (1)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.mdtests/unit/endpoint_test.py
🧰 Additional context used
📓 Path-based instructions (1)
tests/unit/**/*
⚙️ CodeRabbit configuration file
tests/unit/**/*: You are acting as a senior maintainer reviewing unit tests for the hiero-sdk-python project. Your goal is to ensure tests are extensive, deterministic, and protect against breaking changes.CRITICAL PRINCIPLES - Tests Must Fail Loudly & Deterministically:
- Tests must provide useful error messages when they fail for future debugging.
- No
print()statements - use assertions with descriptive messages.- No timing-dependent or unseeded random assertions.
- No network calls or external dependencies (unit tests are isolated).
- No unjustified TODOs or skipped tests without tracking issues.
PRIORITY 1 - Protect Against Breaking Changes:
- Assert public attributes exist (e.g.,
assert hasattr(obj, 'account_id')).- Assert return types where relevant (e.g.,
assert isinstance(result, AccountId)).- Assert fluent setters return
self(e.g.,assert tx.set_memo("test") is tx).- Assert backward-compatible defaults are maintained.
- If a breaking change is introduced, tests must assert deprecation behavior and test old behavior until removal.
PRIORITY 2 - Constructor & Setter Behavior:
- Test constructor behavior with valid inputs, edge cases, and invalid inputs.
- Test setter behavior including method chaining (fluent interface).
- Verify that setters validate input and raise appropriate exceptions.
- Test that getters return expected values after construction/setting.
PRIORITY 3 - Comprehensive Coverage:
- Unit tests should be extensive - test even if we don't expect users to use it currently.
- Cover happy paths AND unhappy paths/edge cases.
- Test boundary conditions, null/None values, empty collections, etc.
- Avoid brittle ordering assertions unless order is part of the contract.
PRIORITY 4 - No Mocks for Non-Existent Modules:
- All imports must reference actual SDK modules - no hallucinated paths.
- Validate import paths against the actual
src/hiero_sdk_pythonstructure.- Mocks should only be used for external de...
Files:
tests/unit/endpoint_test.py
🔇 Additional comments (1)
CHANGELOG.md (1)
10-10: LGTM - Clear changelog entry.The entry appropriately documents the addition of unit tests for endpoint.py. Optionally, consider being more specific about the file path (e.g., "Added unit tests in
tests/unit/endpoint_test.py...") to help future maintainers locate the changes quickly.
There was a problem hiding this comment.
This is really awesome work, thank you so much for your contribution!! 👍
A few points:
-
Please take a look at the suggestions from coderabbit.
-
For the description, we suggest adding this as it helps us gain insight into your PR. You can add something like "Created tests for endpoint.py to reach 90% test coverage"
Please let us know if you have any questions or need assistance, we are always happy to help :)
Thank you from the team!
Signed-off-by: mukundkumarjha <[email protected]>
|
@aceppaluni thanks for suggestions I have updated the pr as per codearabbit. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
tests/unit/endpoint_test.py (6)
7-18: Add fluent interface verification and attribute existence checks.Per coding guidelines PRIORITY 1 & 2, tests must verify:
- Setters return
selffor method chaining (fluent interface)- Public attributes exist using
hasattrto protect against breaking changes- Constructor behavior with actual values, not just
NoneThese are already flagged in past reviews but remain unaddressed.
🔎 Suggested enhancements
def test_getter_setter(): + """Test Endpoint constructor, getters, and setters with fluent interface.""" - ''' Test for Endpoint constructor''' endpoint = Endpoint(address=None, port=None, domain_name=None) - endpoint.set_address(b'127.0.1.1') - endpoint.set_port(77777) - endpoint.set_domain_name("redpanda.com") + + # Test fluent interface (method chaining) - PRIORITY 1 + result = endpoint.set_address(b'127.0.1.1') + assert result is endpoint, "set_address must return self for method chaining" + + result = endpoint.set_port(77777) + assert result is endpoint, "set_port must return self for method chaining" + + result = endpoint.set_domain_name("redpanda.com") + assert result is endpoint, "set_domain_name must return self for method chaining" + # Protect against breaking changes - PRIORITY 1 + 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" +def test_constructor_with_values(): + """Test Endpoint constructor with actual values - PRIORITY 2.""" + 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"
38-47: Add return type verification and edge case coverage.Per PRIORITY 1, verify the return type to protect against breaking changes. Per PRIORITY 3, test edge cases like
Nonevalues for optional fields. These were already flagged in past reviews.🔎 Suggested enhancements
def test_to_proto(): + """Verifies that an Endpoint instance can be correctly serialized to protobuf.""" - '''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() + + # Protect against breaking changes - PRIORITY 1 + assert proto is not None, "Must return a protobuf object" + assert proto.ipAddressV4 == b'127.0.1.1' assert proto.port == 77777 assert proto.domain_name == "redpanda.com" +def test_to_proto_with_none_values(): + """Test serialization when optional fields are None - PRIORITY 3.""" + endpoint = Endpoint(address=b'127.0.0.1', port=8080, domain_name=None) + proto = endpoint._to_proto() + + assert proto.ipAddressV4 == b'127.0.0.1' + assert proto.port == 8080 + # Verify None domain_name is handled appropriately + assert proto.domain_name == "" or not proto.HasField("domain_name"), \ + "None domain_name must serialize appropriately"
49-54: Add return type verification and test edge cases with None values.Per PRIORITY 1, verify return type with
isinstance. Per PRIORITY 3, test edge cases likeNonevalues for address or port, which could cause runtime errors if__str__doesn't handle them properly. This was already flagged in past reviews.🔎 Suggested enhancements
def test_str(): + """Tests the human-readable string representation of the Endpoint.""" - ''' Tests the human-readable string representation of the Endpoint. ''' endpoint = Endpoint(address=b'127.0.1.1', port=77777, domain_name="redpanda.com") - assert str(endpoint) == '127.0.1.1:77777' + result = str(endpoint) + + # Protect against breaking changes - PRIORITY 1 + assert isinstance(result, str), "String representation must 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 - PRIORITY 3.""" + endpoint = Endpoint(address=None, port=None, domain_name="example.com") + result = str(endpoint) + + assert isinstance(result, str), "Must return string even with None values" + # Verify it doesn't raise an exception + assert result is not None, "Must produce valid string representation"
56-62: Expand error handling tests to cover all validation scenarios.Per PRIORITY 3 (comprehensive coverage), the test should verify error handling for:
- All missing required fields (currently only tests missing
ip_address_v4)- Invalid types for each field
- Boundary conditions (negative ports, out-of-range ports)
This comprehensive error coverage was already requested in past reviews but remains incomplete.
🔎 Suggested comprehensive error tests
-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) +@pytest.mark.parametrize(("invalid_data", "expected_error_type", "error_message_fragment"), [ + # Missing required fields + ({"port": 77777}, ValueError, "JSON data must contain"), + ({"ip_address_v4": "127.0.0.1"}, ValueError, "JSON data must contain"), + + # Invalid types + ({"ip_address_v4": 12345, "port": 77777, "domain_name": "test.com"}, (ValueError, TypeError), ""), + ({"ip_address_v4": "127.0.0.1", "port": "not_a_number", "domain_name": "test.com"}, (ValueError, TypeError), ""), + + # Boundary conditions + ({"ip_address_v4": "127.0.0.1", "port": -1, "domain_name": "test.com"}, (ValueError, TypeError), ""), + ({"ip_address_v4": "127.0.0.1", "port": 65536, "domain_name": "test.com"}, (ValueError, TypeError), ""), +]) +def test_from_dict_invalid_inputs(invalid_data, expected_error_type, error_message_fragment): + """Validates error handling for missing fields, invalid types, and boundary conditions - PRIORITY 3.""" + with pytest.raises(expected_error_type) as exc_info: + Endpoint.from_dict(invalid_data) + + if error_message_fragment: + assert error_message_fragment in str(exc_info.value), \ + f"Error message must contain '{error_message_fragment}' for useful debugging"Note: Adjust expected error types based on actual implementation behavior. Some cases may raise
TypeErrorinstead ofValueError.
64-75: Add return type verification and test optional field handling.Per PRIORITY 1, verify the return type with
isinstanceto protect against breaking changes. Per PRIORITY 3, test scenarios where optional fields likedomain_nameare missing from the dictionary. This was already requested in past reviews.🔎 Suggested enhancements
def test_from_dict_success(): + """Tests successful creation of an Endpoint from a dictionary (JSON format).""" - ''' 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) + # Protect against breaking changes - PRIORITY 1 + assert isinstance(endpoint, Endpoint), "from_dict must return Endpoint instance" + assert endpoint.get_address() == b"127.0.0.1" assert endpoint.get_port() == 77777 assert endpoint.get_domain_name() == "redpanda.com" +def test_from_dict_optional_domain_name(): + """Test from_dict when domain_name is optional/missing - PRIORITY 3.""" + data = { + "ip_address_v4": "192.168.1.1", + "port": 8080 + } + endpoint = Endpoint.from_dict(data) + + assert isinstance(endpoint, Endpoint), "Must return Endpoint instance" + assert endpoint.get_address() == b"192.168.1.1" + assert endpoint.get_port() == 8080 + # Verify optional field handling + assert endpoint.get_domain_name() is None or endpoint.get_domain_name() == "", \ + "Domain name must be None or empty when not provided"
1-75: Critical coverage gaps remain unaddressed from previous reviews.The test file still lacks essential coverage areas required to reach the 90% target per issue #1219:
PRIORITY 2 - Missing setter input validation:
- No tests verify that
set_port,set_address, andset_domain_namevalidate inputs- Should test: negative ports, out-of-range ports, invalid types
PRIORITY 3 - Insufficient boundary condition testing:
- Port mapping tests only cover 3 cases (0, 50111, 80)
- Missing: boundary values (1, 50110, 50212, 65535), full valid range testing
PRIORITY 3 - No roundtrip serialization test:
- Should verify
_from_proto(_to_proto(endpoint))preserves all fields exactlyThese gaps were comprehensively detailed in past reviews but remain unaddressed.
🔎 Suggested additional tests to reach 90% coverage
def test_setter_input_validation(): """Test that setters validate input types and reject invalid values - PRIORITY 2.""" endpoint = Endpoint(address=None, port=None, domain_name=None) # Test invalid port values with pytest.raises((ValueError, TypeError)) as exc_info: endpoint.set_port(-1) assert "port" in str(exc_info.value).lower(), "Error message must mention 'port'" with pytest.raises((ValueError, TypeError)): endpoint.set_port(65536) # Test invalid address type with pytest.raises((ValueError, TypeError)): endpoint.set_address("not bytes") # Test invalid domain_name type with pytest.raises((ValueError, TypeError)): endpoint.set_domain_name(12345) @pytest.mark.parametrize("port", [1, 80, 443, 8080, 50210, 65535]) def test_valid_port_boundaries(port): """Test valid port values are preserved correctly - PRIORITY 3.""" endpoint = Endpoint(address=b"127.0.0.1", port=port, domain_name=None) assert endpoint.get_port() == port, f"Port {port} must be preserved" @pytest.mark.parametrize(("input_port", "expected_port"), [ (50110, 50110), # Just before mapping threshold (50211, 50211), # Maps to itself (50212, 50211), # >= 50111 maps to 50211 ]) def test_from_proto_boundary_ports(input_port, expected_port): """Test port mapping edge cases in _from_proto - PRIORITY 3.""" mock_proto = MagicMock() mock_proto.port = input_port mock_proto.ipAddressV4 = b"127.0.0.1" mock_proto.domain_name = "test.com" endpoint = Endpoint._from_proto(mock_proto) assert endpoint.get_port() == expected_port, \ f"Port {input_port} must map to {expected_port}" def test_roundtrip_serialization(): """Test that to_proto and from_proto roundtrip preserves all data - PRIORITY 3.""" original = Endpoint(address=b"192.168.1.1", port=8080, domain_name="test.com") proto = original._to_proto() restored = Endpoint._from_proto(proto) assert restored.get_address() == original.get_address(), "Address must be preserved" assert restored.get_port() == original.get_port(), "Port must be preserved" assert restored.get_domain_name() == original.get_domain_name(), "Domain name must be preserved"Note: Adjust validation assertions based on actual implementation behavior.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.mdtests/unit/endpoint_test.py
🧰 Additional context used
📓 Path-based instructions (1)
tests/unit/**/*
⚙️ CodeRabbit configuration file
tests/unit/**/*: You are acting as a senior maintainer reviewing unit tests for the hiero-sdk-python project. Your goal is to ensure tests are extensive, deterministic, and protect against breaking changes.CRITICAL PRINCIPLES - Tests Must Fail Loudly & Deterministically:
- Tests must provide useful error messages when they fail for future debugging.
- No
print()statements - use assertions with descriptive messages.- No timing-dependent or unseeded random assertions.
- No network calls or external dependencies (unit tests are isolated).
- No unjustified TODOs or skipped tests without tracking issues.
PRIORITY 1 - Protect Against Breaking Changes:
- Assert public attributes exist (e.g.,
assert hasattr(obj, 'account_id')).- Assert return types where relevant (e.g.,
assert isinstance(result, AccountId)).- Assert fluent setters return
self(e.g.,assert tx.set_memo("test") is tx).- Assert backward-compatible defaults are maintained.
- If a breaking change is introduced, tests must assert deprecation behavior and test old behavior until removal.
PRIORITY 2 - Constructor & Setter Behavior:
- Test constructor behavior with valid inputs, edge cases, and invalid inputs.
- Test setter behavior including method chaining (fluent interface).
- Verify that setters validate input and raise appropriate exceptions.
- Test that getters return expected values after construction/setting.
PRIORITY 3 - Comprehensive Coverage:
- Unit tests should be extensive - test even if we don't expect users to use it currently.
- Cover happy paths AND unhappy paths/edge cases.
- Test boundary conditions, null/None values, empty collections, etc.
- Avoid brittle ordering assertions unless order is part of the contract.
PRIORITY 4 - No Mocks for Non-Existent Modules:
- All imports must reference actual SDK modules - no hallucinated paths.
- Validate import paths against the actual
src/hiero_sdk_pythonstructure.- Mocks should only be used for external de...
Files:
tests/unit/endpoint_test.py
🧬 Code graph analysis (1)
tests/unit/endpoint_test.py (1)
src/hiero_sdk_python/address_book/endpoint.py (7)
Endpoint(18-168)set_address(42-53)set_port(64-75)set_domain_name(86-97)get_port(77-84)get_domain_name(99-106)from_dict(155-168)
🪛 markdownlint-cli2 (0.18.1)
CHANGELOG.md
10-10: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
🪛 Ruff (0.14.10)
tests/unit/endpoint_test.py
20-20: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
⏰ 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). (1)
- GitHub Check: Codacy Static Code Analysis
Signed-off-by: mukundkumarjha <[email protected]>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
tests/unit/endpoint_test.py (3)
43-59: Fix parametrize syntax and verify all fields are mapped from protobuf.Two issues need attention:
- Static analysis issue:
pytest.mark.parametrizeexpects a tuple for the first argument, not a list (line 43).- Incomplete field verification: The test only verifies port mapping but doesn't check that
addressanddomain_nameare also correctly mapped from the protobuf. This was flagged in a previous review but remains unaddressed.Per PRIORITY 1, also add return type verification to protect against breaking changes.
🔎 Proposed fixes
-@pytest.mark.parametrize("input_port, expected_port", [ +@pytest.mark.parametrize(("input_port", "expected_port"), [ (0, 50211), (50111, 50211), (80, 80) ]) def test_from_proto_port_mapping(input_port, expected_port): - """ Tests the logic that converts a Protobuf ServiceEndpoint into an Endpoint object. """ + """Tests port mapping logic when converting Protobuf ServiceEndpoint to Endpoint. + + Port mapping rules: + - Port 0 or 50111 maps to 50211 (legacy/default behavior) + - Other ports pass through unchanged + """ 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 (not just port) + assert endpoint.get_address() == b"127.0.1.1", "Address must be mapped from proto" + assert endpoint.get_domain_name() == "redpanda.com", "Domain name must be mapped from proto" + + # Protect against breaking changes - PRIORITY 1 + assert isinstance(endpoint, Endpoint), "Must return Endpoint instance"Based on coding guidelines, this test should comprehensively verify all protobuf-to-Endpoint mapping, not just the port field.
98-109: Add return type verification to protect against breaking changes.The test correctly verifies field values after calling
from_dict, but per PRIORITY 1 (protect against breaking changes), it should verify the return type withisinstance.🔎 Suggested enhancement
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) + # Protect against breaking changes - verify return type + assert isinstance(endpoint, Endpoint), "from_dict should return Endpoint instance" + assert endpoint.get_address() == b"127.0.0.1" assert endpoint.get_port() == 77777 assert endpoint.get_domain_name() == "redpanda.com"
61-70: Add return type verification to protect against breaking changes.The test correctly verifies field serialization, but per PRIORITY 1 (protect against breaking changes), it should also verify the return type with
isinstance.🔎 Suggested enhancement
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() + + # Verify return type - PRIORITY 1: protect against breaking changes + assert proto is not None, "Should return a protobuf object" + assert proto.ipAddressV4 == b'127.0.1.1' assert proto.port == 77777 assert proto.domain_name == "redpanda.com"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tests/unit/endpoint_test.py
🧰 Additional context used
📓 Path-based instructions (1)
tests/unit/**/*
⚙️ CodeRabbit configuration file
tests/unit/**/*: You are acting as a senior maintainer reviewing unit tests for the hiero-sdk-python project. Your goal is to ensure tests are extensive, deterministic, and protect against breaking changes.CRITICAL PRINCIPLES - Tests Must Fail Loudly & Deterministically:
- Tests must provide useful error messages when they fail for future debugging.
- No
print()statements - use assertions with descriptive messages.- No timing-dependent or unseeded random assertions.
- No network calls or external dependencies (unit tests are isolated).
- No unjustified TODOs or skipped tests without tracking issues.
PRIORITY 1 - Protect Against Breaking Changes:
- Assert public attributes exist (e.g.,
assert hasattr(obj, 'account_id')).- Assert return types where relevant (e.g.,
assert isinstance(result, AccountId)).- Assert fluent setters return
self(e.g.,assert tx.set_memo("test") is tx).- Assert backward-compatible defaults are maintained.
- If a breaking change is introduced, tests must assert deprecation behavior and test old behavior until removal.
PRIORITY 2 - Constructor & Setter Behavior:
- Test constructor behavior with valid inputs, edge cases, and invalid inputs.
- Test setter behavior including method chaining (fluent interface).
- Verify that setters validate input and raise appropriate exceptions.
- Test that getters return expected values after construction/setting.
PRIORITY 3 - Comprehensive Coverage:
- Unit tests should be extensive - test even if we don't expect users to use it currently.
- Cover happy paths AND unhappy paths/edge cases.
- Test boundary conditions, null/None values, empty collections, etc.
- Avoid brittle ordering assertions unless order is part of the contract.
PRIORITY 4 - No Mocks for Non-Existent Modules:
- All imports must reference actual SDK modules - no hallucinated paths.
- Validate import paths against the actual
src/hiero_sdk_pythonstructure.- Mocks should only be used for external de...
Files:
tests/unit/endpoint_test.py
🧬 Code graph analysis (1)
tests/unit/endpoint_test.py (1)
src/hiero_sdk_python/address_book/endpoint.py (7)
Endpoint(18-168)set_address(42-53)set_port(64-75)set_domain_name(86-97)get_port(77-84)get_domain_name(99-106)from_dict(155-168)
🪛 Ruff (0.14.10)
tests/unit/endpoint_test.py
43-43: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
⏰ 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: Title Check
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (4)
tests/unit/endpoint_test.py (4)
1-6: LGTM!Imports are correctly structured and reference actual SDK modules. The use of
MagicMockfor protobuf objects is appropriate for unit testing.
34-40: LGTM!Clean test that verifies the constructor with actual values. Addresses past review feedback effectively.
72-81: LGTM!Excellent test that includes return type verification and clear assertions. Addresses past review feedback effectively.
84-88: LGTM!Good edge case test that verifies the expected behavior when
addressis None. The test correctly expects anAttributeErrorbased on the implementation (which callsdecode('utf-8')on the address).
Signed-off-by: mukundkumarjha <[email protected]>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
tests/unit/endpoint_test.py (4)
124-130: Remove duplicate test - already covered by parametrized test.This test is redundant with
test_from_dict_missing_fields(lines 114-122), which already covers the missingip_address_v4scenario along with other missing fields.Per coding guidelines, avoid duplicate tests. Remove this function.
🔎 Suggested change
-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) -
132-143: Add return type verification per PRIORITY 1.Per coding guidelines and past review feedback, add return type verification to protect against breaking changes.
🔎 Suggested enhancement
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) + # Protect against breaking changes + assert isinstance(endpoint, Endpoint), "from_dict must return Endpoint instance" + assert endpoint.get_address() == b"127.0.0.1" assert endpoint.get_port() == 77777 assert endpoint.get_domain_name() == "redpanda.com"
52-83: Consider adding edge cases around port mapping threshold.The test correctly covers the main port mapping scenarios. To strengthen edge case coverage per PRIORITY 3, consider adding test cases for ports immediately adjacent to the mapping threshold:
- Port 50110 (just before 50111)
- Port 50112 (just after 50111)
This would definitively verify that only 0 and 50111 trigger the mapping, and no adjacent values accidentally map.
🔎 Suggested enhancement
@pytest.mark.parametrize( ("input_port", "expected_port"), [ (0, 50211), (50111, 50211), (80, 80), + (50110, 50110), # Just before threshold - should pass through + (50112, 50112), # Just after threshold - should pass through ], )
85-94: Add return type verification and consider roundtrip test.Per PRIORITY 1 and past review feedback, add verification that
_to_proto()returns a valid protobuf object. Additionally, past reviews suggested a roundtrip test to ensure_from_proto(_to_proto(endpoint))preserves all fields.🔎 Suggested enhancements
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() + + # Verify return type + assert proto is not None, "Should return a protobuf object" + assert proto.ipAddressV4 == b'127.0.1.1' assert proto.port == 77777 assert proto.domain_name == "redpanda.com"Consider adding a separate roundtrip test:
def test_proto_roundtrip(): """Verify that from_proto → to_proto → from_proto preserves all fields.""" original = Endpoint(address=b'10.0.0.1', port=8080, domain_name="test.com") proto = original._to_proto() restored = Endpoint._from_proto(proto) assert restored.get_address() == original.get_address() assert restored.get_port() == original.get_port() assert restored.get_domain_name() == original.get_domain_name()
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tests/unit/endpoint_test.py
🧰 Additional context used
📓 Path-based instructions (1)
tests/unit/**/*
⚙️ CodeRabbit configuration file
tests/unit/**/*: You are acting as a senior maintainer reviewing unit tests for the hiero-sdk-python project. Your goal is to ensure tests are extensive, deterministic, and protect against breaking changes.CRITICAL PRINCIPLES - Tests Must Fail Loudly & Deterministically:
- Tests must provide useful error messages when they fail for future debugging.
- No
print()statements - use assertions with descriptive messages.- No timing-dependent or unseeded random assertions.
- No network calls or external dependencies (unit tests are isolated).
- No unjustified TODOs or skipped tests without tracking issues.
PRIORITY 1 - Protect Against Breaking Changes:
- Assert public attributes exist (e.g.,
assert hasattr(obj, 'account_id')).- Assert return types where relevant (e.g.,
assert isinstance(result, AccountId)).- Assert fluent setters return
self(e.g.,assert tx.set_memo("test") is tx).- Assert backward-compatible defaults are maintained.
- If a breaking change is introduced, tests must assert deprecation behavior and test old behavior until removal.
PRIORITY 2 - Constructor & Setter Behavior:
- Test constructor behavior with valid inputs, edge cases, and invalid inputs.
- Test setter behavior including method chaining (fluent interface).
- Verify that setters validate input and raise appropriate exceptions.
- Test that getters return expected values after construction/setting.
PRIORITY 3 - Comprehensive Coverage:
- Unit tests should be extensive - test even if we don't expect users to use it currently.
- Cover happy paths AND unhappy paths/edge cases.
- Test boundary conditions, null/None values, empty collections, etc.
- Avoid brittle ordering assertions unless order is part of the contract.
PRIORITY 4 - No Mocks for Non-Existent Modules:
- All imports must reference actual SDK modules - no hallucinated paths.
- Validate import paths against the actual
src/hiero_sdk_pythonstructure.- Mocks should only be used for external de...
Files:
tests/unit/endpoint_test.py
🧬 Code graph analysis (1)
tests/unit/endpoint_test.py (1)
src/hiero_sdk_python/address_book/endpoint.py (2)
Endpoint(18-168)from_dict(155-168)
⏰ 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). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (3)
tests/unit/endpoint_test.py (3)
96-105: LGTM - Good return type verification and assertions.The test correctly verifies the string representation with proper return type checking per PRIORITY 1.
108-112: LGTM - Good edge case coverage.Correctly tests error behavior when required fields are None. This aligns with PRIORITY 3 (comprehensive coverage of edge cases).
114-122: LGTM - Comprehensive error handling coverage.Excellent parametrized test covering all required field validation. This addresses past review feedback and aligns with PRIORITY 3 (comprehensive coverage).
Signed-off-by: mukundkumarjha <[email protected]>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
tests/unit/endpoint_test.py (3)
123-134: Add return type verification to protect against breaking changes.Per PRIORITY 1, verify the return type with
isinstanceto ensure the factory method contract is maintained.🔎 Proposed enhancement
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) + # Protect against breaking changes - PRIORITY 1 + assert isinstance(endpoint, Endpoint), "from_dict must return Endpoint instance" + assert endpoint.get_address() == b"127.0.0.1" assert endpoint.get_port() == 77777 assert endpoint.get_domain_name() == "redpanda.com"
76-85: Add return type verification and test None value handling.Per PRIORITY 1, verify the return type to protect against breaking changes. Per PRIORITY 3, test edge cases including None values to document serialization behavior when fields are unset.
🔎 Suggested enhancements
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() + + # Protect against breaking changes + assert proto is not None, "Should return a protobuf object" + assert proto.ipAddressV4 == b'127.0.1.1' assert proto.port == 77777 assert proto.domain_name == "redpanda.com" +def test_to_proto_with_none_values(): + """Test serialization when fields are None.""" + endpoint = Endpoint(address=None, port=None, domain_name=None) + proto = endpoint._to_proto() + + assert proto is not None, "Should return protobuf object even with None values" + assert proto.ipAddressV4 is None + assert proto.port is None + assert proto.domain_name is None or proto.domain_name == ""
43-74: Add boundary cases around port mapping threshold.Per PRIORITY 3 (comprehensive coverage), the test should verify behavior at boundaries around the special port values. Currently tests 0, 50111, and 80, but missing:
- Port 50110 (just before 50111, should NOT map)
- Port 50112 (just after 50111, should NOT map)
- Port 1 (minimum valid port, should NOT map)
- Port 65535 (maximum valid port, should NOT map)
This ensures the port mapping logic
if port == 0 or port == 50111is precise and doesn't inadvertently map nearby ports.🔎 Suggested test expansion
@pytest.mark.parametrize( ("input_port", "expected_port"), [ (0, 50211), (50111, 50211), (80, 80), + (1, 1), # Minimum valid port, no mapping + (50110, 50110), # Just before threshold, no mapping + (50112, 50112), # Just after threshold, no mapping + (65535, 65535), # Maximum valid port, no mapping ], ) def test_from_proto_port_mapping(input_port, expected_port):
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.mdtests/unit/endpoint_test.py
🧰 Additional context used
📓 Path-based instructions (1)
tests/unit/**/*
⚙️ CodeRabbit configuration file
tests/unit/**/*: You are acting as a senior maintainer reviewing unit tests for the hiero-sdk-python project. Your goal is to ensure tests are extensive, deterministic, and protect against breaking changes.CRITICAL PRINCIPLES - Tests Must Fail Loudly & Deterministically:
- Tests must provide useful error messages when they fail for future debugging.
- No
print()statements - use assertions with descriptive messages.- No timing-dependent or unseeded random assertions.
- No network calls or external dependencies (unit tests are isolated).
- No unjustified TODOs or skipped tests without tracking issues.
PRIORITY 1 - Protect Against Breaking Changes:
- Assert public attributes exist (e.g.,
assert hasattr(obj, 'account_id')).- Assert return types where relevant (e.g.,
assert isinstance(result, AccountId)).- Assert fluent setters return
self(e.g.,assert tx.set_memo("test") is tx).- Assert backward-compatible defaults are maintained.
- If a breaking change is introduced, tests must assert deprecation behavior and test old behavior until removal.
PRIORITY 2 - Constructor & Setter Behavior:
- Test constructor behavior with valid inputs, edge cases, and invalid inputs.
- Test setter behavior including method chaining (fluent interface).
- Verify that setters validate input and raise appropriate exceptions.
- Test that getters return expected values after construction/setting.
PRIORITY 3 - Comprehensive Coverage:
- Unit tests should be extensive - test even if we don't expect users to use it currently.
- Cover happy paths AND unhappy paths/edge cases.
- Test boundary conditions, null/None values, empty collections, etc.
- Avoid brittle ordering assertions unless order is part of the contract.
PRIORITY 4 - No Mocks for Non-Existent Modules:
- All imports must reference actual SDK modules - no hallucinated paths.
- Validate import paths against the actual
src/hiero_sdk_pythonstructure.- Mocks should only be used for external de...
Files:
tests/unit/endpoint_test.py
🧬 Code graph analysis (1)
tests/unit/endpoint_test.py (1)
src/hiero_sdk_python/address_book/endpoint.py (7)
Endpoint(18-168)set_address(42-53)set_port(64-75)set_domain_name(86-97)get_port(77-84)get_domain_name(99-106)from_dict(155-168)
⏰ 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). (1)
- GitHub Check: Codacy Static Code Analysis
|
Hi @mukundkumarjha thank you so much for your patience at this! |
Signed-off-by: mukundkumarjha <[email protected]>
Hi @exploreriii thanks for taking time to review, I have updated the test and removed duplicity. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/unit/endpoint_test.py (1)
152-163: Add return type verification per PRIORITY 1.Per coding guidelines PRIORITY 1 (Protect Against Breaking Changes), add
isinstancecheck to ensurefrom_dictreturns anEndpointinstance. This is consistent with similar tests liketest_constructor_with_values(line 51) andtest_from_proto_port_mapping(line 88).🔎 Suggested enhancement
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) + # Protect against breaking changes - PRIORITY 1 + assert isinstance(endpoint, Endpoint), "from_dict must return Endpoint instance" + assert endpoint.get_address() == b"127.0.0.1" assert endpoint.get_port() == 77777 assert endpoint.get_domain_name() == "redpanda.com"As per coding guidelines, PRIORITY 1 requires asserting return types to protect against breaking changes.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tests/unit/endpoint_test.py
🧰 Additional context used
📓 Path-based instructions (1)
tests/unit/**/*
⚙️ CodeRabbit configuration file
tests/unit/**/*: You are acting as a senior maintainer reviewing unit tests for the hiero-sdk-python project. Your goal is to ensure tests are extensive, deterministic, and protect against breaking changes.CRITICAL PRINCIPLES - Tests Must Fail Loudly & Deterministically:
- Tests must provide useful error messages when they fail for future debugging.
- No
print()statements - use assertions with descriptive messages.- No timing-dependent or unseeded random assertions.
- No network calls or external dependencies (unit tests are isolated).
- No unjustified TODOs or skipped tests without tracking issues.
PRIORITY 1 - Protect Against Breaking Changes:
- Assert public attributes exist (e.g.,
assert hasattr(obj, 'account_id')).- Assert return types where relevant (e.g.,
assert isinstance(result, AccountId)).- Assert fluent setters return
self(e.g.,assert tx.set_memo("test") is tx).- Assert backward-compatible defaults are maintained.
- If a breaking change is introduced, tests must assert deprecation behavior and test old behavior until removal.
PRIORITY 2 - Constructor & Setter Behavior:
- Test constructor behavior with valid inputs, edge cases, and invalid inputs.
- Test setter behavior including method chaining (fluent interface).
- Verify that setters validate input and raise appropriate exceptions.
- Test that getters return expected values after construction/setting.
PRIORITY 3 - Comprehensive Coverage:
- Unit tests should be extensive - test even if we don't expect users to use it currently.
- Cover happy paths AND unhappy paths/edge cases.
- Test boundary conditions, null/None values, empty collections, etc.
- Avoid brittle ordering assertions unless order is part of the contract.
PRIORITY 4 - No Mocks for Non-Existent Modules:
- All imports must reference actual SDK modules - no hallucinated paths.
- Validate import paths against the actual
src/hiero_sdk_pythonstructure.- Mocks should only be used for external de...
Files:
tests/unit/endpoint_test.py
🧬 Code graph analysis (1)
tests/unit/endpoint_test.py (1)
src/hiero_sdk_python/address_book/endpoint.py (7)
Endpoint(18-168)set_address(42-53)set_port(64-75)set_domain_name(86-97)get_port(77-84)get_domain_name(99-106)from_dict(155-168)
🪛 Ruff (0.14.10)
tests/unit/endpoint_test.py
90-90: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
⏰ 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). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (1)
tests/unit/endpoint_test.py (1)
1-163: Excellent test coverage - comprehensive and well-structured.This test suite successfully achieves the 90% coverage target for
endpoint.py. The tests are thorough, covering:
- ✅ Constructor behavior with both None and actual values
- ✅ All getters and setters with fluent interface validation
- ✅ Roundtrip serialization (Endpoint → proto → Endpoint)
- ✅ Port mapping logic (0, 50111 → 50211)
- ✅ Edge cases (None values, missing fields)
- ✅ Error handling in
from_dict- ✅ Appropriate use of
isinstancechecks for breaking change protection (PRIORITY 1)- ✅ No test smells: no print statements, timing dependencies, or external calls
The test suite is deterministic, well-documented, and follows project conventions. Great work addressing the previous review feedback!
Signed-off-by: mukundkumarjha <[email protected]>
|
Brilliant work, thank you so much 🥇 |
|
@exploreriii thanks for your kind words. |
3 participants
Description:Created tests for endpoint.py to reach 90% test coverage
Related issue(s):
Fixes #1219
Notes for reviewer:
Checklist