feat: Improved unit test coverage for TransactionId class. #1358
Conversation
|
Hi, this is WorkflowBot.
|
Adityarya11
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #1358 +/- ##
==========================================
+ Coverage 92.29% 92.44% +0.15%
==========================================
Files 139 139
Lines 8515 8528 +13
==========================================
+ Hits 7859 7884 +25
+ Misses 656 644 -12 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive unit test coverage for the TransactionId class to address previously untested code paths, specifically focusing on error handling in string parsing, hash implementation, and scheduled transaction flag behavior.
- Adds 6 new test functions covering core TransactionId functionality
- Tests error handling for invalid string formats in the
from_stringmethod - Validates hash and equality implementations for set/dictionary usage
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/unit/transaction_id_test.py | New test file with 6 test functions covering TransactionId parsing, hashing, equality, protobuf conversion, and generation |
| CHANGELOG.md | Documents the addition of improved unit test coverage for TransactionId class |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
📝 WalkthroughWalkthroughTransactionId.from_string now accepts an optional "?scheduled" suffix and preserves it in construction and string output; error messages improved. New unit tests covering parsing (including scheduled), error handling, hashing/equality, proto round-trips, generation, and string conversion were added. CHANGELOG updated. 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: 7
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.mdtests/unit/transaction_id_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/transaction_id_test.py
🧬 Code graph analysis (1)
tests/unit/transaction_id_test.py (2)
src/hiero_sdk_python/transaction/transaction_id.py (1)
TransactionId(8-156)src/hiero_sdk_python/account/account_id.py (1)
AccountId(21-198)
🪛 Ruff (0.14.10)
tests/unit/transaction_id_test.py
27-27: pytest.raises(ValueError) is too broad, set the match parameter or use a more specific exception
(PT011)
⏰ 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). (11)
- GitHub Check: check-linked-issue
- GitHub Check: Title Check
- GitHub Check: changelog-check
- GitHub Check: Agent
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: run-examples
- GitHub Check: build-and-test (3.11)
- GitHub Check: build-and-test (3.13)
- GitHub Check: build-and-test (3.12)
- GitHub Check: build-and-test (3.10)
- GitHub Check: StepSecurity Harden-Runner
🔇 Additional comments (1)
CHANGELOG.md (1)
84-84: LGTM!The changelog entry accurately documents the test coverage improvements and follows the project's changelog conventions.
Signed-off-by: Adityarya11 <arya050411@gmail.com>
Adityarya11
force-pushed
the
feat/improve-txn-id-coverage#1309
branch
from
795abfe to
314df29
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/unit/transaction_id_test.py (1)
46-53: Add explicit tests for None comparison and ne method.The test should explicitly verify behavior when comparing with
Noneand directly test the__ne__method to ensure comprehensive equality/inequality coverage.🔎 Suggested additions
def test_equality(): """Test __eq__ implementation.""" tx_id1 = TransactionId.from_string("0.0.1@100.1") tx_id2 = TransactionId.from_string("0.0.1@100.1") assert tx_id1 == tx_id2 assert tx_id1 != "some_string" assert tx_id1 != TransactionId.from_string("0.0.1@100.2") + + # Test None comparison explicitly + assert tx_id1 != None + assert not (tx_id1 == None) + + # Test __ne__ explicitly + assert not (tx_id1 != tx_id2)Based on coding guidelines: "Cover happy paths AND unhappy paths/edge cases" and "Test boundary conditions, null/None values."
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.mdtests/unit/transaction_id_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/transaction_id_test.py
🧬 Code graph analysis (1)
tests/unit/transaction_id_test.py (3)
src/hiero_sdk_python/transaction/transaction_id.py (1)
TransactionId(8-156)src/hiero_sdk_python/account/account_id.py (1)
AccountId(21-198)src/hiero_sdk_python/transaction/transaction_receipt.py (1)
account_id(88-100)
🪛 Ruff (0.14.10)
tests/unit/transaction_id_test.py
27-27: pytest.raises(ValueError) is too broad, set the match parameter or use a more specific exception
(PT011)
⏰ 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: build-and-test (3.13)
- GitHub Check: build-and-test (3.11)
- GitHub Check: build-and-test (3.10)
- GitHub Check: build-and-test (3.12)
- GitHub Check: run-examples
- GitHub Check: StepSecurity Harden-Runner
🔇 Additional comments (2)
CHANGELOG.md (1)
84-84: LGTM! Clear and accurate changelog entry.The changelog entry accurately documents the improved test coverage and follows project conventions.
tests/unit/transaction_id_test.py (1)
68-83: Excellent test coverage with proper breaking-change protection.This test properly follows the coding guidelines by:
- Asserting return type with
isinstance- Asserting public attributes exist with
hasattr- Verifying backward-compatible defaults (scheduled=False)
- Testing boundary conditions (seconds > 0, nanos >= 0)
Based on coding guidelines: "Assert public attributes exist," "Assert return types where relevant," and "Assert backward-compatible defaults are maintained."
Signed-off-by: Adityarya11 <arya050411@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
tests/unit/transaction_id_test.py (4)
1-7: Add pytest.mark.unit marker for consistency.Missing
pytestmark = pytest.mark.unitafter the imports to maintain consistency with other test files in this directory.🔎 Proposed fix
import pytest import re from hiero_sdk_python import ( TransactionId, AccountId ) + +pytestmark = pytest.mark.unitBased on coding guidelines: Consistency with project test structure.
61-72: Incomplete proto verification - check all fields.The test only verifies the
scheduledflag in the protobuf, but doesn't confirm thataccount_idandvalid_startare properly set. Comprehensive verification protects against regressions in the_to_proto()method.🔎 Proposed enhancement
# Default is False proto_default = tx_id._to_proto() assert proto_default.scheduled is False + assert proto_default.accountID.accountNum == 123 + assert proto_default.transactionValidStart.seconds == 100 + assert proto_default.transactionValidStart.nanos == 1 # Set to True tx_id.scheduled = True proto_scheduled = tx_id._to_proto() assert proto_scheduled.scheduled is True + assert proto_scheduled.accountID.accountNum == 123 + assert proto_scheduled.transactionValidStart.seconds == 100 + assert proto_scheduled.transactionValidStart.nanos == 1Based on coding guidelines: "Unit tests should be extensive" and past review comment.
37-50: Missing test coverage: scheduled flag doesn't affect hash.The
__hash__implementation includes thescheduledfield in the hash tuple (line 154 in transaction_id.py), but this test doesn't verify that TransactionIds with differentscheduledvalues produce different hashes. This is a coverage gap for the hash implementation.🔎 Proposed fix
# Verify usage in sets unique_ids = {tx_id1, tx_id2, tx_id3} assert len(unique_ids) == 2 + + # Verify scheduled flag affects hash + tx_id1_scheduled = TransactionId.from_string("0.0.1@100.1") + tx_id1_scheduled.scheduled = True + assert hash(tx_id1) != hash(tx_id1_scheduled), "Hash should differ when scheduled flag differs"Based on coding guidelines: "Unit tests should be extensive" and past review comment noting this gap.
52-59: Missing test coverage: scheduled flag doesn't affect equality.The
__eq__implementation checks thescheduledfield (line 144 in transaction_id.py), but this test doesn't verify that TransactionIds with differentscheduledvalues are unequal. A past review comment noted this gap and was marked as "✅ Addressed in commit 314df29", but the test is not present in the current code.🔎 Proposed fix
assert tx_id1 == tx_id2 assert tx_id1 != "some_string" assert tx_id1 != TransactionId.from_string("0.0.1@100.2") + + # Test scheduled flag affects equality + tx_id1_scheduled = TransactionId.from_string("0.0.1@100.1") + tx_id1_scheduled.scheduled = True + assert tx_id1 != tx_id1_scheduled, "TransactionIds with different scheduled flags should not be equal"Based on coding guidelines: "Comprehensive coverage" and past review comment indicating this should be tested.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/hiero_sdk_python/transaction/transaction_id.pytests/unit/transaction_id_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/transaction_id_test.py
🧬 Code graph analysis (1)
tests/unit/transaction_id_test.py (2)
src/hiero_sdk_python/transaction/transaction_id.py (6)
TransactionId(8-163)from_string(57-86)_to_proto(97-111)generate(38-54)to_string(88-95)_from_proto(114-127)src/hiero_sdk_python/account/account_id.py (1)
AccountId(21-198)
⏰ 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: build-and-test (3.10)
- GitHub Check: build-and-test (3.12)
- GitHub Check: build-and-test (3.11)
- GitHub Check: build-and-test (3.13)
- GitHub Check: run-examples
- GitHub Check: StepSecurity Harden-Runner
🔇 Additional comments (6)
src/hiero_sdk_python/transaction/transaction_id.py (2)
95-95: LGTM - Proper string formatting with scheduled suffix.The conditional appending of "?scheduled" creates proper symmetry with the parsing logic and follows the documented format.
109-110: LGTM - Correct protobuf scheduled flag assignment.The conditional assignment is correct; protobuf fields default to False, so explicitly setting only when True is appropriate.
tests/unit/transaction_id_test.py (4)
74-89: Excellent test with comprehensive breaking-change protection.The test properly verifies the
generate()method with attribute existence checks, type assertions, and backward-compatible defaults. Well done.Based on coding guidelines: "Assert public attributes exist" and "Assert backward-compatible defaults are maintained."
105-109: LGTM - Proper delegation verification.The test correctly verifies that
__str__delegates toto_string().
112-126: LGTM - Comprehensive _from_proto() verification.The test properly verifies protobuf deserialization with type checking and field-by-field assertions.
Based on coding guidelines: Type assertions and comprehensive field verification.
129-150: Excellent coverage of round-trip conversion and scheduled parsing.Both tests are well-structured:
test_round_trip_conversionensures data integrity through proto serialization/deserializationtest_from_string_with_scheduled_flagprovides critical coverage of the new scheduled suffix parsing featureBased on coding guidelines: Comprehensive coverage of new functionality and data integrity verification.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
CHANGELOG.md
⏰ 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: build-and-test (3.11)
- GitHub Check: build-and-test (3.10)
- GitHub Check: build-and-test (3.12)
- GitHub Check: build-and-test (3.13)
- GitHub Check: run-examples
- GitHub Check: StepSecurity Harden-Runner
Signed-off-by: Adityarya11 <arya050411@gmail.com>
Adityarya11
force-pushed
the
feat/improve-txn-id-coverage#1309
branch
from
1d8a3fb to
805659f
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
CHANGELOG.md (1)
87-87: Consider expanding the changelog entry for completeness.The current entry captures the core areas but could be more comprehensive. The test file includes coverage for:
- Parsing with error handling (invalid suffixes and formats)
- Hashing and equality behavior
- Protobuf conversion with scheduled flag preservation
- ID generation via
generate()Consider revising to:
- Improved unit test coverage for `TransactionId` class, including parsing with error handling, hashing/equality, protobuf conversion with scheduled flag, and ID generation.This gives readers a fuller picture of the coverage improvements.
tests/unit/transaction_id_test.py (1)
75-86: Verify all protobuf fields, not just the scheduled flag.While the test correctly validates the scheduled flag, it should also verify that other fields (
account_id,valid_start) are properly set in the protobuf to ensure comprehensive coverage of the_to_proto()method.🔎 Suggested enhancement
def test_to_proto_sets_scheduled(): """Test that _to_proto sets the scheduled flag correctly.""" tx_id = TransactionId.from_string("0.0.123@100.1") # Default is False proto_default = tx_id._to_proto() assert proto_default.scheduled is False + assert proto_default.accountID.accountNum == 123 + assert proto_default.transactionValidStart.seconds == 100 + assert proto_default.transactionValidStart.nanos == 1 # Set to True tx_id.scheduled = True proto_scheduled = tx_id._to_proto() assert proto_scheduled.scheduled is True + assert proto_scheduled.accountID.accountNum == 123 + assert proto_scheduled.transactionValidStart.seconds == 100 + assert proto_scheduled.transactionValidStart.nanos == 1Based on coding guidelines: "Unit tests should be extensive."
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.mdsrc/hiero_sdk_python/transaction/transaction_id.pytests/unit/transaction_id_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/transaction_id_test.py
🧬 Code graph analysis (2)
tests/unit/transaction_id_test.py (2)
src/hiero_sdk_python/transaction/transaction_id.py (6)
TransactionId(8-176)from_string(57-99)_to_proto(110-124)generate(38-54)to_string(101-108)_from_proto(127-140)src/hiero_sdk_python/account/account_id.py (1)
AccountId(21-198)
src/hiero_sdk_python/transaction/transaction_id.py (2)
src/hiero_sdk_python/account/account_id.py (2)
AccountId(21-198)from_string(53-85)src/hiero_sdk_python/timestamp.py (1)
Timestamp(8-166)
🪛 Ruff (0.14.10)
tests/unit/transaction_id_test.py
34-34: pytest.raises(ValueError) is too broad, set the match parameter or use a more specific exception
(PT011)
src/hiero_sdk_python/transaction/transaction_id.py
77-77: Abstract raise to an inner function
(TRY301)
77-77: Avoid specifying long messages outside the exception class
(TRY003)
81-81: Abstract raise to an inner function
(TRY301)
81-81: Avoid specifying long messages outside the exception class
(TRY003)
90-90: Abstract raise to an inner function
(TRY301)
90-90: Avoid specifying long messages outside the exception class
(TRY003)
98-98: Use raise without specifying exception name
Remove exception name
(TRY201)
99-99: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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). (6)
- GitHub Check: build-and-test (3.13)
- GitHub Check: build-and-test (3.12)
- GitHub Check: build-and-test (3.10)
- GitHub Check: build-and-test (3.11)
- GitHub Check: run-examples
- GitHub Check: StepSecurity Harden-Runner
🔇 Additional comments (2)
src/hiero_sdk_python/transaction/transaction_id.py (1)
70-99: LGTM! Solid parsing implementation with strict validation.The scheduled suffix parsing logic correctly:
- Uses
split("?", 1)for strict one-time splitting- Validates the suffix must be exactly "scheduled" (prevents
?scheduledxyzor?invalid)- Validates presence of required
@and.separators- Preserves the original input string for clear error messages
- Differentiates suffix-related errors from general format errors
This addresses the past review feedback requesting strict validation over loose substring matching.
Note on static analysis warnings: The Ruff TRY301/TRY003 warnings about exception message style are low-priority style preferences. The current approach is clear and readable, which is appropriate for user-facing error messages.
tests/unit/transaction_id_test.py (1)
1-162: Excellent comprehensive test coverage with a few essential additions needed.This test file successfully achieves the PR objectives of exceeding 90% coverage and testing previously uncovered logic including:
✓ Parsing error handling (both invalid suffixes and invalid formats)
✓ Hash implementation (__hash__method)
✓ Proto scheduled flag handling (_to_protoand_from_proto)
✓ ID generation (generate()method)
✓ Breaking-change protection withhasattrassertions
✓ Round-trip proto conversion
✓ String conversions (to_string(),__str__)Good practices observed:
- Line 8:
pytestmark = pytest.mark.unitadded (addresses past review feedback)- Breaking-change assertions throughout (e.g.,
assert isinstance(...),assert hasattr(...))- Error message validation with
matchparameter (line 48)- Comprehensive edge case coverage
Essential improvements identified in separate comments:
- Fix indentation at line 27 and add match parameter at line 34
- Add test cases verifying the scheduled flag affects hash values
- Add test cases verifying the scheduled flag affects equality
- Verify all protobuf fields in
test_to_proto_sets_scheduledBased on coding guidelines: "Unit tests protect our future selves - be defensive and forward-looking."
MonaaEid
left a comment
MonaaEid
left a comment
There was a problem hiding this comment.
Nice work on expanding the TransactionId functionality. I appreciate the stronger validation and clearer error messages, as well as the comprehensive unit tests
AntonioCeppellini
left a comment
AntonioCeppellini
left a comment
There was a problem hiding this comment.
Seems a nice job :D
|
Hi @Adityarya11 |
i shall be implementing those |
Signed-off-by: Adityarya11 <arya050411@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/hiero_sdk_python/transaction/transaction_id.pytests/unit/transaction_id_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/transaction_id_test.py
🧬 Code graph analysis (2)
tests/unit/transaction_id_test.py (2)
src/hiero_sdk_python/transaction/transaction_id.py (6)
TransactionId(8-181)from_string(57-104)_to_proto(115-129)generate(38-54)to_string(106-113)_from_proto(132-145)src/hiero_sdk_python/account/account_id.py (1)
AccountId(21-198)
src/hiero_sdk_python/transaction/transaction_id.py (1)
src/hiero_sdk_python/account/account_id.py (2)
AccountId(21-198)from_string(53-85)
🪛 Ruff (0.14.10)
tests/unit/transaction_id_test.py
34-34: pytest.raises(ValueError) is too broad, set the match parameter or use a more specific exception
(PT011)
src/hiero_sdk_python/transaction/transaction_id.py
82-82: Abstract raise to an inner function
(TRY301)
82-82: Avoid specifying long messages outside the exception class
(TRY003)
86-86: Abstract raise to an inner function
(TRY301)
86-86: Avoid specifying long messages outside the exception class
(TRY003)
95-95: Abstract raise to an inner function
(TRY301)
95-95: Avoid specifying long messages outside the exception class
(TRY003)
103-103: Use raise without specifying exception name
Remove exception name
(TRY201)
104-104: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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: build-and-test (3.11)
- GitHub Check: build-and-test (3.12)
- GitHub Check: build-and-test (3.13)
- GitHub Check: run-examples
- GitHub Check: build-and-test (3.10)
- GitHub Check: StepSecurity Harden-Runner
🔇 Additional comments (9)
src/hiero_sdk_python/transaction/transaction_id.py (2)
78-83: LGTM! Strict suffix validation implemented correctly.The split-based approach validates that the suffix is exactly "scheduled" and fails fast on invalid suffixes, addressing the concerns raised in past reviews about loose validation.
85-86: Good defensive validation with clear error messages.The explicit checks for "@" and "." delimiters prevent confusing split errors and provide clear error messages that include the original input string.
Also applies to: 94-95
tests/unit/transaction_id_test.py (7)
10-25: Comprehensive test with breaking-change protection.The test properly verifies parsing, includes attribute existence checks (Priority 1), and asserts the scheduled default. Consider adding a round-trip assertion for even stronger coverage.
Optional: Add round-trip verification
assert tx_id.valid_start.seconds == 1234567890 assert tx_id.valid_start.nanos == 123456789 assert tx_id.scheduled is False + # Verify round-trip string conversion + assert tx_id.to_string() == tx_id_strBased on coding guidelines: Priority 3 - Comprehensive coverage.
38-49: Excellent error validation with precise match patterns.Good use of the
matchparameter withre.escape()for precise error message validation. Covers multiple invalid format cases comprehensively.
51-69: Comprehensive hash testing including scheduled flag impact.Excellent coverage of hash behavior, including the important verification that the scheduled flag affects the hash value (lines 65-69). The set usage test (lines 62-64) provides additional confidence.
71-84: Thorough equality testing including scheduled flag.Good coverage of equality semantics, including type checking (line 77) and verification that the scheduled flag affects equality (lines 80-84).
99-114: Excellent generate() test with breaking-change protection.Comprehensive test that includes attribute existence checks (Priority 1), verifies backward-compatible defaults (line 114), and validates all key properties.
116-127: Good string conversion test; consider exact assertion for scheduled case.The test correctly verifies both cases. For more precise verification, consider asserting the exact output string when the scheduled flag is set.
Optional: Assert exact scheduled string
# Test with scheduled flag tx_id.scheduled = True result_scheduled = tx_id.to_string() assert "?scheduled" in result_scheduled + assert result_scheduled == "0.0.123@1234567890.123456789?scheduled"Based on coding guidelines: Priority 3 - Comprehensive coverage.
130-134: Excellent coverage of remaining TransactionId methods.These tests comprehensively cover:
__str__()delegation toto_string()(130-134)- Protobuf deserialization with all fields verified (137-151)
- Round-trip conversion preservation (154-163)
- Scheduled flag parsing from string (165-173)
All tests follow best practices and address previously identified coverage gaps.
Also applies to: 137-151, 154-163, 165-173
5 participants
Description:
This PR adds a dedicated unit test file for
TransactionIdto improve code coverage. It specifically targets previously uncovered logic in:from_string(thetry-exceptblock).__hash__method implementation.scheduledflag assignment in protobuf conversion.Related issue(s):
Fixes #1309
Checklist: