feat: Created tests for endpoint.py to increase test coverage. #1306
Conversation
Signed-off-by: mukundkumarjha <[email protected]>
Signed-off-by: mukundkumarjha <[email protected]>
|
[commit-verification-bot] To achieve verified status, please read: Remember, you require a GPG key and each commit must be signed with: Thank you for contributing! From the Hiero Python SDK Team |
📝 WalkthroughWalkthroughAdded comprehensive unit tests for the Endpoint class, covering fluent interface setters, getters, constructor behavior, protobuf serialization/deserialization with port remapping logic, string representation, and error handling for dictionary creation. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
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: 9
📜 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
42-42: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
98-98: Assertion should be broken down into multiple parts
(PT018)
⏰ 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 (2)
tests/unit/endpoint_test.py (2)
48-66: Excellent test coverage for_from_protowith port mapping.This parametrized test effectively covers the port remapping logic (0→50211, 50111→50211, passthrough) and validates all fields are correctly mapped. The use of MagicMock for the protobuf object is appropriate, and the assertions include clear failure messages.
79-88: LGTM - Clear string representation test.This test effectively validates the string format and includes return type checking for breaking change protection.
| ## [Unreleased] | ||
|
|
||
| ### Added | ||
| - Added unit test for 'endpoint.py' to increase coverage. |
There was a problem hiding this comment.
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)
| pytestmark = pytest.mark.unit | ||
|
|
||
|
|
||
| def test_getter_setter(): # type: ignore |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify or remove the # type: ignore comment.
The # type: ignore comment suppresses type checker warnings but provides no explanation. If this is addressing a legitimate type checking limitation, add a brief comment explaining why. Otherwise, consider removing it.
| def test_getter_setter(): # type: ignore | ||
|
|
||
| """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" |
There was a problem hiding this comment.
🧹 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:
- Wrong types (e.g., passing string instead of bytes to
set_address) - Boundary conditions (e.g., negative port numbers, port > 65535)
- None values passed to setters
This aligns with PRIORITY 2 (Constructor & Setter Behavior) and PRIORITY 3 (Comprehensive Coverage) in the coding guidelines.
Example test structure
def 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" |
There was a problem hiding this comment.
🧹 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"| assert endpoint.get_port() == 8080 | ||
| assert endpoint.get_domain_name() == "example.com" | ||
|
|
||
| @pytest.mark.parametrize("input_port, expected_port", [ |
There was a problem hiding this comment.
Fix pytest.mark.parametrize signature.
The first argument to pytest.mark.parametrize should be a tuple of parameter names when multiple parameters are provided. While the current string format "input_port, expected_port" works, using a tuple is the recommended type-safe approach.
🔎 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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @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 | |
| ]) |
🧰 Tools
🪛 Ruff (0.14.10)
42-42: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
| 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" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM - Good serialization test.
This test properly validates that _to_proto serializes all fields correctly. For enhanced breaking change protection, consider asserting the return type or checking that it's a ServiceEndpoint instance.
| 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" |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
🧹 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:
- Wrong types in the dictionary (e.g., port as string)
- Extra unexpected keys (if validation exists)
- Malformed ip_address_v4 values
| 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" No newline at end of file |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM - Successful dictionary conversion test.
This test effectively validates the from_dict method with proper data. All fields are correctly asserted. For consistency with other tests, consider adding:
assert isinstance(endpoint, Endpoint), "from_dict should return Endpoint instance"1 participant
Description:
Related issue(s):
Fixes #1219
Notes for reviewer:
Checklist