tests(kafka): add unit tests for KafkaTopicHealthTool and KafkaConsumerGroupTool#1264
tests(kafka): add unit tests for KafkaTopicHealthTool and KafkaConsumerGroupTool#1264
Conversation
…erGroupTool Covers contract, is_available, extract_params, and run paths (happy, error, and not-configured) for both tools. Mocks applied at the tool-module boundary to correctly intercept imported names. Closes #994
Greptile SummaryThis PR adds unit test coverage for Confidence Score: 4/5Safe to merge — tests are logically correct, mock targets are accurate, and all critical paths are covered; remaining gaps are non-blocking style suggestions. Only P2 findings: missing Both new test files warrant a second look on the SASL-SSL and Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Test calls tool function] --> B{bootstrap_servers empty?}
B -- Yes --> C[KafkaConfig.is_configured == False]
C --> D[Integration returns early\navailable=False, no broker contact]
B -- No --> E[KafkaConfig built from params]
E --> F{Mock patched at\ntool-module boundary?}
F -- Yes\nhappy/error tests --> G[Mocked integration function returns\nfixture dict]
G --> H[Tool returns dict to test\nassertions run on result]
F -- No\nnot-configured tests --> I[Real integration called\nearly return before confluent_kafka]
I --> H
Reviews (1): Last reviewed commit: "tests(kafka): add unit tests for KafkaTo..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| assert params["bootstrap_servers"] == "broker:9092" | ||
|
|
||
|
|
||
| # --------------------------------------------------------------------------- |
There was a problem hiding this comment.
limit parameter
get_kafka_topic_health accepts a limit parameter (default 20) that is forwarded directly to get_topic_health. None of the tests pass a non-default limit or assert that the argument is forwarded to the integration call. A call-args assertion similar to the topic forwarding test (test_happy_path_specific_topic_forwards_topic_arg) would close this gap and prevent a silent regression if the wiring is broken.
|
|
||
| assert result["available"] is True | ||
| assert result["topics"][0]["name"] == "payments" | ||
| # Verify the topic argument was forwarded to the integration function. | ||
| _, call_kwargs = mock_fn.call_args | ||
| assert call_kwargs.get("topic") == "payments" | ||
|
|
||
| def test_happy_path_sasl_ssl_connection(self) -> None: | ||
| with patch( | ||
| "app.tools.KafkaTopicHealthTool.get_topic_health", | ||
| return_value=_TOPIC_HEALTH_RESPONSE, | ||
| ): | ||
| result = get_kafka_topic_health( | ||
| bootstrap_servers="broker1:9093", | ||
| security_protocol="SASL_SSL", | ||
| sasl_mechanism="PLAIN", |
There was a problem hiding this comment.
test_happy_path_sasl_ssl_connection verifies only that result["available"] is True, which is fully determined by the mock return value. It doesn't verify that security_protocol, sasl_mechanism, sasl_username, or sasl_password were actually constructed into the KafkaConfig and forwarded to get_topic_health. Inspecting mock_fn.call_args for those fields (like the topic-forwarding test does) would make this test meaningful rather than a pass-through check.
| ): | ||
| result = get_kafka_consumer_group_lag( | ||
| bootstrap_servers="broker1:9092", | ||
| group_id="events-consumer", | ||
| ) | ||
|
|
||
| assert result["available"] is True | ||
| assert result["total_lag"] == 0 | ||
| assert result["partitions"][0]["lag"] == 0 | ||
| assert result["partitions"][0]["committed_offset"] == result["partitions"][0]["high_watermark"] | ||
|
|
||
| def test_happy_path_forwards_group_id_to_integration(self) -> None: | ||
| with patch( | ||
| "app.tools.KafkaConsumerGroupTool.get_consumer_group_lag", | ||
| return_value=_CONSUMER_GROUP_ZERO_LAG_RESPONSE, |
There was a problem hiding this comment.
Same concern as in the topic-health file: test_happy_path_sasl_ssl_connection mocks the integration and only checks result["available"] is True, which is driven entirely by the mock fixture. The actual SASL credentials passed to the tool (security_protocol, sasl_mechanism, sasl_username, sasl_password) are never verified to reach the integration call. Asserting on mock_fn.call_args[0][0] (the KafkaConfig positional argument) would confirm the wiring is intact.
There was a problem hiding this comment.
Pull request overview
This PR adds missing unit tests for the two Kafka diagnostic tool wrappers so their tool registration, availability, parameter extraction, and mocked run paths are covered alongside the existing Kafka integration tests.
Changes:
- Added a new test module for
KafkaTopicHealthTool. - Added a new test module for
KafkaConsumerGroupTool. - Covered tool contract metadata,
is_available,extract_params, and several mockedrunscenarios.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
tests/tools/test_kafka_topic_health_tool.py |
Adds wrapper-level tests for Kafka topic health tool metadata, availability, param extraction, and mocked run behavior. |
tests/tools/test_kafka_consumer_group_tool.py |
Adds wrapper-level tests for Kafka consumer group lag tool metadata, availability, param extraction, and mocked run behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - run happy path: all topics, specific topic, limit param | ||
| - run error path: integration returns available=False |
| fake_error = { | ||
| "source": "kafka", | ||
| "available": False, | ||
| "error": "Connection refused by broker.", | ||
| } | ||
| with patch("app.tools.KafkaTopicHealthTool.get_topic_health", return_value=fake_error): | ||
| result = get_kafka_topic_health(bootstrap_servers="bad-host:9092") | ||
|
|
||
| assert result["available"] is False | ||
| assert "error" in result | ||
| assert result["source"] == "kafka" |
| def test_error_path_returns_unavailable_dict(self) -> None: | ||
| fake_error = { | ||
| "source": "kafka", | ||
| "available": False, | ||
| "error": "Group 'stale-consumer' does not exist.", | ||
| } | ||
| with patch("app.tools.KafkaConsumerGroupTool.get_consumer_group_lag", return_value=fake_error): |
| def test_happy_path_sasl_ssl_connection(self) -> None: | ||
| with patch( | ||
| "app.tools.KafkaTopicHealthTool.get_topic_health", | ||
| return_value=_TOPIC_HEALTH_RESPONSE, | ||
| ): | ||
| result = get_kafka_topic_health( |
| def test_happy_path_sasl_ssl_connection(self) -> None: | ||
| with patch( | ||
| "app.tools.KafkaConsumerGroupTool.get_consumer_group_lag", | ||
| return_value=_CONSUMER_GROUP_LAG_RESPONSE, | ||
| ): | ||
| result = get_kafka_consumer_group_lag( |
| result = get_kafka_topic_health(bootstrap_servers="") | ||
| assert result["available"] is False | ||
| assert result["source"] == "kafka" |
| # get_consumer_group_lag returns early before any confluent_kafka import. | ||
| result = get_kafka_consumer_group_lag( | ||
| bootstrap_servers="", | ||
| group_id="payments-consumer", | ||
| ) | ||
| assert result["available"] is False | ||
| assert result["source"] == "kafka" |
…ring, exception propagation, short-circuit guard - Add test for limit argument forwarding to KafkaTopicHealthTool - Expand SASL-SSL tests in both tools to assert KafkaConfig fields (security_protocol, sasl_mechanism, sasl_username, sasl_password) are correctly wired via call_args inspection - Add exception-propagation tests to both tools: if the integration raises instead of returning an error dict, the tool must not swallow it - Update not-configured tests to use wraps= so the integration call is tracked, confirming the early-exit branch was entered not bypassed
yashksaini-coder
force-pushed
the
feat/kafka-tool-tests
branch
from
ae8d079 to
3f9ae8c
Compare
|
All 10 inline review threads from @greptile-apps and @copilot-pull-request-reviewer were addressed in commit
The latest commit ( |
|
All five concerns are properly resolved. Here's a quick verification against the resolution map: 1. 2 & 3. SASL-SSL config propagation (both tools): Both tests extract 4 & 5. Exception propagation (both tools): Switched from returning a fake error dict to Not-configured short-circuit (both tools): The One minor note on style: the inline The test suite is in good shape. All original concerns addressed, exception behavior properly validated, and SASL credential wiring verified at the struct level rather than just the return value. LGTM. Tip: You can customize Greptile's behavior for this repo with |
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Devesh36
left a comment
Devesh36
left a comment
There was a problem hiding this comment.
ran test locally , looks good 👍
|
🧠 @yashksaini-coder opened a PR. Maintainers feared them. CI genuflected. It merged. 🚨
👋 Join us on Discord - OpenSRE : hang out, contribute, or hunt for features and issues. Everyone's welcome. |
Summary
Adds full unit-test coverage for both Kafka tools:
KafkaTopicHealthTool—tests/tools/test_kafka_topic_health_tool.py(27 tests)KafkaConsumerGroupTool—tests/tools/test_kafka_consumer_group_tool.py(16 tests)What's tested
Each file covers four layers:
BaseToolContract)is_availablereturns bool,extract_paramsreturns dictis_availableconnection_verifiedis set, false for all missing/falsy variantsextract_paramsrunTechnical notes
Mocks are applied at the tool-module boundary (
app.tools.KafkaTopicHealthTool.get_topic_health), not at the integration module, because both tool files usefrom app.integrations.kafka import ...which binds the name locally.confluent_kafkais not installed in the dev venv. All happy-path tests mock the integration function before confluent_kafka is reached; the not-configured path returns before any broker contact.Results
Closes #1267