Skip to content

Commit 9aba0b3

Browse files
committed
fix: address CodeRabbit review on
- Replace remaining logging.error(...) (root logger) with logger.error() - Add exc_info=True to file_contents_query.py for traceback parity - Strengthen tests: seed valid IDs, assert emitting logger identity (record.name == module), assert traceback is attached (record.exc_info) Signed-off-by: iron-prog <dt915725@gmail.com>
1 parent 0979449 commit 9aba0b3

9 files changed

Lines changed: 93 additions & 62 deletions

src/hiero_sdk_python/file/file_contents_query.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,7 @@ def _make_request(self) -> query_pb2.Query:
8181

8282
return query
8383
except Exception as e:
84-
logger.error("Exception in _make_request: %s", e)
84+
logger.error("Exception in _make_request: %s", e, exc_info=True)
8585
raise
8686

8787
def _get_method(self, channel: _Channel) -> _Method:

src/hiero_sdk_python/file/file_info_query.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ def _make_request(self) -> query_pb2.Query:
7878

7979
return query
8080
except Exception as e:
81-
logging.error("Exception in _make_request: %s", e, exc_info=True)
81+
logger.error("Exception in _make_request: %s", e, exc_info=True)
8282
raise
8383

8484
def _get_method(self, channel: _Channel) -> _Method:

src/hiero_sdk_python/query/account_info_query.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ def _make_request(self):
7272

7373
return query
7474
except Exception as e:
75-
logging.error("Exception in _make_request: %s", e)
75+
logger.error("Exception in _make_request: %s", e, exc_info=True)
7676
raise
7777

7878
def _get_method(self, channel: _Channel) -> _Method:

src/hiero_sdk_python/query/token_info_query.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ def _make_request(self) -> query_pb2.Query:
7474

7575
return query
7676
except Exception as e:
77-
logging.error("Exception in _make_request: %s", e)
77+
logger.error("Exception in _make_request: %s", e, exc_info=True)
7878
raise
7979

8080
def _get_method(self, channel: _Channel) -> _Method:

src/hiero_sdk_python/query/token_nft_info_query.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ def _make_request(self) -> query_pb2.Query:
7272

7373
return query
7474
except Exception as e:
75-
logging.error("Exception in _make_request: %s", e, exc_info=True)
75+
logger.error("Exception in _make_request: %s", e, exc_info=True)
7676
raise
7777

7878
def _get_method(self, channel: _Channel) -> _Method:

src/hiero_sdk_python/query/topic_info_query.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,7 @@ def _make_request(self) -> query_pb2.Query:
102102
return query
103103

104104
except Exception as e:
105-
logging.error("Exception in _make_request: %s", e, exc_info=True)
105+
logger.error("Exception in _make_request: %s", e, exc_info=True)
106106
raise
107107

108108
def _get_method(self, channel: _Channel) -> _Method:

src/hiero_sdk_python/query/transaction_get_receipt_query.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -187,7 +187,7 @@ def _make_request(self) -> query_pb2.Query:
187187

188188
return query
189189
except Exception as e:
190-
logging.error("Exception in _make_request: %s", e, exc_info=True)
190+
logger.error("Exception in _make_request: %s", e, exc_info=True)
191191
raise
192192

193193
def _get_method(self, channel: _Channel) -> _Method:

src/hiero_sdk_python/schedule/schedule_info_query.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ def _make_request(self) -> query_pb2.Query:
7474

7575
return query
7676
except Exception as e:
77-
logging.error("Exception in _make_request: %s", e, exc_info=True)
77+
logger.error("Exception in _make_request: %s", e, exc_info=True)
7878
raise
7979

8080
def _get_method(self, channel: _Channel) -> _Method:

tests/unit/query_error_logging_test.py

Lines changed: 85 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -7,91 +7,119 @@
77
import pytest
88
import requests
99

10+
from hiero_sdk_python.account.account_id import AccountId
1011
from hiero_sdk_python.account.account_records_query import AccountRecordsQuery
1112
from hiero_sdk_python.client.network import Network
13+
from hiero_sdk_python.consensus.topic_id import TopicId
1214
from hiero_sdk_python.contract.contract_bytecode_query import ContractBytecodeQuery
1315
from hiero_sdk_python.contract.contract_call_query import ContractCallQuery
16+
from hiero_sdk_python.contract.contract_id import ContractId
1417
from hiero_sdk_python.contract.contract_info_query import ContractInfoQuery
1518
from hiero_sdk_python.file.file_contents_query import FileContentsQuery
19+
from hiero_sdk_python.file.file_id import FileId
1620
from hiero_sdk_python.file.file_info_query import FileInfoQuery
1721
from hiero_sdk_python.query.account_balance_query import CryptoGetAccountBalanceQuery
1822
from hiero_sdk_python.query.account_info_query import AccountInfoQuery
1923
from hiero_sdk_python.query.token_info_query import TokenInfoQuery
2024
from hiero_sdk_python.query.token_nft_info_query import TokenNftInfoQuery
2125
from hiero_sdk_python.query.topic_info_query import TopicInfoQuery
2226
from hiero_sdk_python.query.transaction_get_receipt_query import TransactionGetReceiptQuery
27+
from hiero_sdk_python.schedule.schedule_id import ScheduleId
2328
from hiero_sdk_python.schedule.schedule_info_query import ScheduleInfoQuery
29+
from hiero_sdk_python.tokens.nft_id import NftId
30+
from hiero_sdk_python.tokens.token_id import TokenId
31+
from hiero_sdk_python.transaction.transaction_id import TransactionId
2432

2533

2634
pytestmark = pytest.mark.unit
2735

28-
QUERY_CLASSES = [
29-
AccountInfoQuery,
30-
FileContentsQuery,
31-
TokenInfoQuery,
32-
AccountRecordsQuery,
33-
ContractBytecodeQuery,
34-
ContractCallQuery,
35-
ContractInfoQuery,
36-
FileInfoQuery,
37-
CryptoGetAccountBalanceQuery,
38-
TokenNftInfoQuery,
39-
TopicInfoQuery,
40-
TransactionGetReceiptQuery,
41-
ScheduleInfoQuery,
36+
_ACCOUNT_ID = AccountId(0, 0, 100)
37+
_TOKEN_ID = TokenId(0, 0, 200)
38+
39+
QUERY_FACTORIES = [
40+
(AccountInfoQuery, lambda: AccountInfoQuery(_ACCOUNT_ID), "_make_request_header"),
41+
(FileContentsQuery, lambda: FileContentsQuery(FileId(0, 0, 101)), "_make_request_header"),
42+
(TokenInfoQuery, lambda: TokenInfoQuery(_TOKEN_ID), "_make_request_header"),
43+
(AccountRecordsQuery, lambda: AccountRecordsQuery(_ACCOUNT_ID), "_make_request_header"),
44+
(
45+
ContractBytecodeQuery,
46+
lambda: ContractBytecodeQuery(ContractId(0, 0, 102)),
47+
"_make_request_header",
48+
),
49+
(ContractCallQuery, lambda: ContractCallQuery(ContractId(0, 0, 102)), "_make_request_header"),
50+
(ContractInfoQuery, lambda: ContractInfoQuery(ContractId(0, 0, 102)), "_make_request_header"),
51+
(FileInfoQuery, lambda: FileInfoQuery(FileId(0, 0, 101)), "_make_request_header"),
52+
(
53+
CryptoGetAccountBalanceQuery,
54+
lambda: CryptoGetAccountBalanceQuery(_ACCOUNT_ID),
55+
"_make_request_header",
56+
),
57+
(TokenNftInfoQuery, lambda: TokenNftInfoQuery(NftId(_TOKEN_ID, 1)), "_make_request_header"),
58+
(TopicInfoQuery, lambda: TopicInfoQuery(TopicId(0, 0, 103)), "_make_request_header"),
59+
(
60+
TransactionGetReceiptQuery,
61+
lambda: TransactionGetReceiptQuery(TransactionId(_ACCOUNT_ID)),
62+
"transaction_id._to_proto",
63+
),
64+
(ScheduleInfoQuery, lambda: ScheduleInfoQuery(ScheduleId(0, 0, 104)), "_make_request_header"),
4265
]
4366

67+
_IDS = [cls.__name__ for cls, _, _ in QUERY_FACTORIES]
4468

45-
@pytest.mark.parametrize("query_cls", QUERY_CLASSES, ids=[c.__name__ for c in QUERY_CLASSES])
46-
def test_make_request_source_has_no_print_or_traceback_dump(query_cls):
47-
"""
48-
Static regression check preventing accidental reintroduction of
49-
print() or traceback.print_exc() in _make_request().
50-
"""
69+
70+
@pytest.mark.parametrize("query_cls,factory,patch_target", QUERY_FACTORIES, ids=_IDS)
71+
def test_make_request_source_has_no_print_or_traceback_dump(query_cls, factory, patch_target):
72+
"""Ensure _make_request does not use print() or traceback.print_exc()."""
5173
source = inspect.getsource(query_cls._make_request)
5274
assert "print(" not in source, f"{query_cls.__name__}._make_request still calls print()"
5375
assert "traceback.print_exc()" not in source, (
5476
f"{query_cls.__name__}._make_request still calls traceback.print_exc()"
5577
)
5678

5779

58-
@pytest.mark.parametrize("query_cls", QUERY_CLASSES, ids=[c.__name__ for c in QUERY_CLASSES])
59-
def test_make_request_logs_exception_via_logging_module(query_cls, caplog):
60-
"""
61-
Verify that _make_request routes exceptions through the logging module
62-
instead of printing directly to stdout or stderr.
80+
@pytest.mark.parametrize("query_cls,factory,patch_target", QUERY_FACTORIES, ids=_IDS)
81+
def test_make_request_logs_exception_via_module_logger(query_cls, factory, patch_target, caplog):
82+
"""Ensure _make_request logs exceptions with the module logger and exc_info."""
83+
query = factory()
6384

64-
The request header is mocked to raise an exception so the error logging
65-
path can be exercised consistently across query implementations.
66-
"""
67-
query = query_cls()
85+
# Patch either the query helper or an inline header builder.
86+
if "." in patch_target:
87+
attr_name, method_name = patch_target.split(".")
88+
patch_obj = type(getattr(query, attr_name))
89+
patch_ctx = patch.object(patch_obj, method_name, side_effect=ValueError("boom"))
90+
else:
91+
patch_ctx = patch.object(query_cls, patch_target, side_effect=ValueError("boom"))
6892

69-
# Some tests disable the SDK's root logger via the shared client fixture.
70-
# Override the logger level here so caplog can capture ERROR records.
93+
# Override the parent logger level so ERROR logs are captured.
7194
with (
72-
patch.object(query_cls, "_make_request_header", side_effect=ValueError("boom")),
95+
patch_ctx,
7396
caplog.at_level(logging.ERROR, logger="hiero_sdk_python"),
7497
caplog.at_level(logging.ERROR),
7598
pytest.raises(ValueError),
7699
):
77100
query._make_request()
78101

79-
assert any(
80-
record.levelno == logging.ERROR and "Exception in _make_request" in record.message for record in caplog.records
81-
), f"No ERROR-level 'Exception in _make_request' log record captured for {query_cls.__name__}"
102+
matching = [
103+
record
104+
for record in caplog.records
105+
if record.levelno == logging.ERROR and "Exception in _make_request" in record.message
106+
]
107+
assert matching, f"No ERROR-level 'Exception in _make_request' log record captured for {query_cls.__name__}"
108+
109+
record = matching[0]
110+
assert record.name == query_cls.__module__, (
111+
f"Expected log emitted by '{query_cls.__module__}', got '{record.name}' "
112+
"(likely used logging.error(...) instead of logger.error(...))"
113+
)
114+
assert record.exc_info is not None, f"{query_cls.__name__} logged the error without exc_info -- traceback is lost"
82115

83116

84-
def test_fetch_nodes_from_mirror_node_source_has_no_print(caplog):
85-
"""
86-
Static + dynamic check for the missing-mirror-URL fallback path:
87-
no print(), and the warning is captured by the logging module.
88-
"""
117+
def test_fetch_nodes_from_mirror_node_logs_missing_url(caplog):
118+
"""Ensure missing mirror URL is logged and an empty list is returned."""
89119
source = inspect.getsource(Network._fetch_nodes_from_mirror_node)
90120
assert "print(" not in source
91121

92-
# Bypass __init__ (which would itself raise ValueError for an unknown
93-
# network with no mirror URL and no default nodes) -- we only need
94-
# `self.network` set to exercise this specific private method.
122+
# Bypass __init__; only self.network is needed for this private method.
95123
network = Network.__new__(Network)
96124
network.network = "not-a-known-network"
97125

@@ -102,17 +130,17 @@ def test_fetch_nodes_from_mirror_node_source_has_no_print(caplog):
102130
result = network._fetch_nodes_from_mirror_node()
103131

104132
assert result == []
105-
assert any(
106-
record.levelno == logging.WARNING and "No known mirror node URL" in record.message for record in caplog.records
107-
)
133+
matching = [
134+
record
135+
for record in caplog.records
136+
if record.levelno == logging.WARNING and "No known mirror node URL" in record.message
137+
]
138+
assert matching
139+
assert matching[0].name == Network.__module__
108140

109141

110142
def test_fetch_nodes_from_mirror_node_logs_request_exception(caplog, monkeypatch):
111-
"""
112-
Static + dynamic check for the mirror-node-fetch-failure fallback path:
113-
no print(), and the error is captured by the logging module, with the
114-
method still falling back to an empty list rather than raising.
115-
"""
143+
"""Ensure request failures are logged and fall back to an empty list."""
116144
source = inspect.getsource(Network._fetch_nodes_from_mirror_node)
117145
assert "print(" not in source
118146

@@ -131,7 +159,10 @@ def raise_request_exception(*args, **kwargs):
131159
result = network._fetch_nodes_from_mirror_node()
132160

133161
assert result == []
134-
assert any(
135-
record.levelno == logging.ERROR and "Error fetching nodes from mirror node API" in record.message
162+
matching = [
163+
record
136164
for record in caplog.records
137-
)
165+
if record.levelno == logging.ERROR and "Error fetching nodes from mirror node API" in record.message
166+
]
167+
assert matching
168+
assert matching[0].name == Network.__module__

0 commit comments

Comments
 (0)