test: enhance NodeAddress tests with additional coverage
#1302
Conversation
…nversion Signed-off-by: MonaaEid <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #1302 +/- ##
==========================================
+ Coverage 91.80% 92.01% +0.21%
==========================================
Files 139 139
Lines 8484 8482 -2
==========================================
+ Hits 7789 7805 +16
+ Misses 695 677 -18 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This pull request enhances test coverage for the NodeAddress class by adding comprehensive unit tests for serialization and deserialization operations. The changes also include a refactoring of the _to_proto method to improve protobuf conversion efficiency.
Key Changes:
- Added five new comprehensive unit tests covering protobuf conversion, dictionary creation, round-trip serialization, and edge cases
- Refactored the
_to_protomethod to directly append endpoints to the protobuf list instead of creating an intermediate list - Updated CHANGELOG.md to document the test enhancements
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/unit/node_address_test.py | Added comprehensive unit tests for NodeAddress serialization/deserialization including test_to_proto(), test_from_dict(), test_from_proto(), test_round_trip(), and test_empty_addresses() |
| src/hiero_sdk_python/address_book/node_address.py | Refactored _to_proto() method to directly append converted endpoints to the protobuf's serviceEndpoint list, eliminating the intermediate list |
| CHANGELOG.md | Added entry documenting the enhanced test coverage for NodeAddress |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds comprehensive unit tests for NodeAddress conversions (dict↔proto↔object), covering edge cases (0x‑prefixed cert hash, None account_id, empty endpoints). Refactors Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches✅ Passed checks (5 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: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.mdsrc/hiero_sdk_python/address_book/node_address.pytests/unit/node_address_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/node_address_test.py
🧬 Code graph analysis (1)
src/hiero_sdk_python/address_book/node_address.py (1)
src/hiero_sdk_python/address_book/endpoint.py (1)
_to_proto(130-142)
🪛 Ruff (0.14.10)
tests/unit/node_address_test.py
4-4: Redefinition of unused pytest from line 2
Remove definition: pytest
(F811)
⏰ 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). (8)
- GitHub Check: Agent
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: run-examples
- GitHub Check: build-and-test (3.13)
- GitHub Check: build-and-test (3.10)
- GitHub Check: build-and-test (3.11)
- GitHub Check: build-and-test (3.12)
- GitHub Check: StepSecurity Harden-Runner
🔇 Additional comments (6)
CHANGELOG.md (1)
128-128: LGTM!Changelog entry appropriately documents the enhanced test coverage.
src/hiero_sdk_python/address_book/node_address.py (1)
104-105: LGTM! Cleaner protobuf population.The direct append to
node_address_proto.serviceEndpointis more idiomatic for protobuf repeated fields and eliminates the intermediate list.tests/unit/node_address_test.py (4)
64-99: LGTM! Comprehensive proto conversion test.This test properly validates all scalar fields, AccountId conversion, and ServiceEndpoint population with clear assertions.
102-124: LGTM! Good dictionary parsing test.The test correctly validates NodeAddress creation from a dictionary, including proper hex cert hash handling. The test verifies all key fields.
126-153: LGTM! Good protobuf parsing test.The test properly validates NodeAddress creation from protobuf with correct field mapping.
187-199: LGTM! Good edge case coverage.This test properly validates the empty addresses scenario, ensuring the protobuf serialization handles empty collections correctly.
…nversion Signed-off-by: MonaaEid <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/unit/node_address_test.py (2)
9-32: Strengthen protection against breaking changes with attribute and type checks.Per PRIORITY 1 guidelines, add assertions to verify that attributes exist and have expected types. This protects against future refactoring that might accidentally remove or rename fields.
🔎 Suggested additions to protect against breaking changes
Add these assertions after line 32:
# Protect against breaking changes - verify attributes exist assert hasattr(node_address, '_public_key') assert hasattr(node_address, '_account_id') assert hasattr(node_address, '_node_id') assert hasattr(node_address, '_cert_hash') assert hasattr(node_address, '_addresses') assert hasattr(node_address, '_description') # Verify types assert isinstance(node_address._account_id, AccountId) assert isinstance(node_address._addresses, list) assert isinstance(node_address._cert_hash, bytes)Based on coding guidelines: PRIORITY 1 - Protect Against Breaking Changes.
34-60: Add return type assertion for str method.Per PRIORITY 1 guidelines, assert that the return type is correct to protect against breaking changes.
🔎 Suggested addition
Add this assertion after line 53:
# Protect against breaking changes - verify return type assert isinstance(result, str)Based on coding guidelines: PRIORITY 1 - Assert return types where relevant.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tests/unit/node_address_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/node_address_test.py
🧬 Code graph analysis (1)
tests/unit/node_address_test.py (3)
src/hiero_sdk_python/account/account_id.py (1)
AccountId(21-198)src/hiero_sdk_python/address_book/endpoint.py (1)
Endpoint(18-168)src/hiero_sdk_python/address_book/node_address.py (4)
NodeAddress(26-155)_to_proto(87-107)_from_dict(131-155)_from_proto(59-85)
⏰ 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). (7)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: run-examples
- GitHub Check: build-and-test (3.10)
- GitHub Check: build-and-test (3.13)
- GitHub Check: build-and-test (3.12)
- GitHub Check: build-and-test (3.11)
- GitHub Check: StepSecurity Harden-Runner
🔇 Additional comments (2)
tests/unit/node_address_test.py (2)
153-189: Excellent addition of addresses verification in round-trip test.The addresses verification (lines 183-188) directly addresses a previous review comment and ensures that the refactored
_to_protomethod correctly preserves endpoint data during serialization/deserialization. This strengthens protection against regressions.
191-203: Good edge case coverage for empty addresses.This test correctly verifies that an empty addresses list results in an empty
serviceEndpointin the protobuf, ensuring proper handling of this edge case.
|
Hi, I think the coderabbit suggestions here are reasonable :) |
|
Hi, this is MergeConflictBot. Please resolve these conflicts locally and push the changes. To assist you, please read: Thank you for contributing! |
…nversion Signed-off-by: MonaaEid <[email protected]>
Signed-off-by: MontyPokemon <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.mdtests/unit/node_address_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/node_address_test.py
⏰ 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). (7)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: run-examples
- GitHub Check: build-and-test (3.10)
- GitHub Check: build-and-test (3.11)
- GitHub Check: build-and-test (3.13)
- GitHub Check: build-and-test (3.12)
- GitHub Check: StepSecurity Harden-Runner
🔇 Additional comments (5)
CHANGELOG.md (1)
129-129: LGTM! Clear changelog entry.The changelog entry appropriately documents the test coverage enhancement with a clear reference to the affected test file.
tests/unit/node_address_test.py (4)
1-6: LGTM! Clean imports.All imports are necessary and properly used throughout the test file. Past review issues regarding duplicate/unused imports have been resolved.
62-100: Excellent comprehensive test with multiple endpoints.This test provides strong coverage of the
_to_protoconversion:
- ✓ Tests multiple endpoints (3), exercising the loop behavior
- ✓ Includes
isinstancecheck at line 81 to protect against breaking changes (PRIORITY 1)- ✓ Validates all scalar fields and nested structures (AccountId, endpoints)
- ✓ Clear, deterministic assertions
Great work addressing previous review feedback!
209-222: LGTM! Good edge case coverage.This test appropriately validates the empty endpoints edge case, ensuring the protobuf serialization handles empty lists correctly. Clear and deterministic.
223-237: Excellent edge case test for None account_id.This test appropriately covers the
Noneaccount_id edge case (addressing previous review feedback). The use ofproto.HasField('nodeAccountId')is the correct way to verify the protobuf field is unset. Great work!
…nversion Signed-off-by: MonaaEid <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/node_address_test.py (1)
9-32: Consider adding isinstance check for consistency.While the constructor return type is clear, adding an isinstance assertion would align with PRIORITY 1 guidelines (protecting against breaking changes) and maintain consistency with other tests in this file.
🔎 Suggested addition after line 24
description="Sample Node" ) +# Protect against breaking changes - verify return type +assert isinstance(node_address, NodeAddress) + # Assert properties are set correctlyBased on coding guidelines: PRIORITY 1 - Assert return types where relevant.
♻️ Duplicate comments (2)
tests/unit/node_address_test.py (2)
177-213: Add isinstance check for deserialized object.The round-trip test comprehensively validates field equality but is missing a return type assertion for
node_address2. This would protect against_from_protoreturningNoneor an incorrect type.🔎 Suggested addition after line 199
# Convert back from proto node_address2 = NodeAddress._from_proto(proto) +# Protect against breaking changes - verify return type +assert isinstance(node_address2, NodeAddress) + # Assert all fields are equalBased on coding guidelines: PRIORITY 1 - Assert return types where relevant.
129-143: Add isinstance check for consistency.For consistency with other conversion tests (e.g.,
test_from_dictline 119,test_from_protoline 167), add a return type assertion after creating theNodeAddressfrom dict.🔎 Suggested addition after line 140
node_address = NodeAddress._from_dict(node_dict) +# Protect against breaking changes - verify return type +assert isinstance(node_address, NodeAddress) + assert node_address._cert_hash == b"sample-cert-hash"Based on coding guidelines: PRIORITY 1 - Assert return types where relevant.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tests/unit/node_address_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/node_address_test.py
🧬 Code graph analysis (1)
tests/unit/node_address_test.py (3)
src/hiero_sdk_python/account/account_id.py (1)
AccountId(21-198)src/hiero_sdk_python/address_book/endpoint.py (1)
Endpoint(18-168)src/hiero_sdk_python/address_book/node_address.py (4)
NodeAddress(26-155)_to_proto(87-107)_from_dict(131-155)_from_proto(59-85)
⏰ 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
|
Thank you MonaaEid, I like the test format you have created |
Thanks a lot for your feedback! |
2 participants
This pull request improves the coverage of the
NodeAddressclass by adding comprehensive unit tests. The main focus is on ensuring correct serialization and deserialization ofNodeAddressobjects, especially with their associated endpoints.tests/unit/node_address_test.pycovering conversion to and from protobuf, dictionary creation, round-trip serialization, and handling of empty endpoints. These tests ensure all fields and edge cases are correctly handled.Code correctness:
_to_protomethod insrc/hiero_sdk_python/address_book/node_address.pyto directly append converted endpoints to the protobuf'sserviceEndpointlist, ensuring proper serialization.Related issue(s):
Fixes #1220
Checklist
tests/unit/node_address_test.py