test(oocana): add comprehensive tests for Mainframe callback management#467
test(oocana): add comprehensive tests for Mainframe callback management#467leavesster wants to merge 1 commit intomainfrom
Conversation
- Test add/remove for report, session, and request_response callbacks - Test non-callable rejection with ValueError - Test subscription/unsubscription lifecycle - Test multiple callbacks for same session - Test removing one callback keeps others intact
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Comment |
There was a problem hiding this comment.
Pull request overview
Adds a new unit test suite to cover Mainframe callback registration/removal behavior without requiring a real MQTT broker.
Changes:
- Introduces 15
unittestcases validating callback add/remove flows for report, session, and request/response callbacks. - Uses a mocked MQTT client to verify subscription/unsubscription interactions.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,159 @@ | |||
| import unittest | |||
| from unittest.mock import Mock, MagicMock, patch | |||
There was a problem hiding this comment.
patch is imported but never used. If the project runs any linting/static checks, this will fail; even without linting it adds noise. Remove the unused import (or use it in the tests if intended).
| from unittest.mock import Mock, MagicMock, patch | |
| from unittest.mock import Mock, MagicMock |
| def test_remove_report_callback_logs_warning_for_missing(self): | ||
| """Test remove_report_callback logs warning for non-existent callback.""" | ||
| callback = Mock() | ||
| # Should not raise, just log warning | ||
| self.mainframe.remove_report_callback(callback) | ||
|
|
There was a problem hiding this comment.
test_remove_report_callback_logs_warning_for_missing currently has no assertion that a warning was logged, so it can pass even if logging is removed or the code logs at a different level. If the intent is to verify the warning behavior, assert it explicitly (e.g., via assertLogs on the module logger or by patching self.mainframe._logger.warning).
| self.mainframe.remove_session_callback(session_id, callback) | ||
| self.mock_mqtt_client.unsubscribe.assert_called_once() | ||
|
|
There was a problem hiding this comment.
These unsubscribe assertions only check the call count (assert_called_once()), not that the correct topic was unsubscribed. Since Mainframe.unsubscribe() passes a topic argument, this test should assert the expected argument(s) (and ideally also assert message_callback_remove was called with the same topic) to avoid false positives.
| self.mainframe.remove_session_callback(session_id, callback) | |
| self.mock_mqtt_client.unsubscribe.assert_called_once() | |
| self.mainframe.remove_session_callback(session_id, callback) | |
| # Ensure unsubscribe and message_callback_remove are each called once | |
| self.mock_mqtt_client.unsubscribe.assert_called_once() | |
| self.mock_mqtt_client.message_callback_remove.assert_called_once() | |
| # Ensure both operations use the same topic argument | |
| unsubscribe_args, _ = self.mock_mqtt_client.unsubscribe.call_args | |
| remove_args, _ = self.mock_mqtt_client.message_callback_remove.call_args | |
| self.assertGreaterEqual(len(unsubscribe_args), 1) | |
| self.assertGreaterEqual(len(remove_args), 1) | |
| self.assertEqual(unsubscribe_args[0], remove_args[0]) |
| self.mock_mqtt_client.reset_mock() | ||
|
|
||
| self.mainframe.remove_request_response_callback(session_id, request_id, callback) | ||
| self.mock_mqtt_client.unsubscribe.assert_called_once() |
There was a problem hiding this comment.
Same issue here: assert_called_once() doesn’t verify which topic was unsubscribed. Consider asserting unsubscribe/message_callback_remove were called with f"session/{session_id}/request/{request_id}/response" so the test actually validates the request/response lifecycle behavior.
| self.mock_mqtt_client.unsubscribe.assert_called_once() | |
| self.mock_mqtt_client.unsubscribe.assert_called_once_with( | |
| f"session/{session_id}/request/{request_id}/response" | |
| ) |
| # Internal check - callback should be in the set | ||
| self.assertIn(callback, self.mainframe._Mainframe__report_callbacks) | ||
|
|
There was a problem hiding this comment.
These tests assert internal state via name-mangled private attributes (e.g. _Mainframe__session_callbacks). This couples the tests to implementation details and will break on refactors (rename, type change, etc.). Prefer asserting externally observable behavior (e.g. message_callback_add/subscribe/unsubscribe calls and callback invocation), or add a small public/intended-for-testing accessor on Mainframe if state inspection is required.
leavesster
changed the title
1 participant
Summary
Add 15 unit tests for Mainframe callback management:
add_report_callback()adds callable and rejects non-callableremove_report_callback()removes existing and logs warning for missingadd_session_callback()adds callable, rejects non-callable, subscribes onceremove_session_callback()removes existing and unsubscribes on lastadd_request_response_callback()full lifecycleProblem
Mainframe callback management had only placeholder tests (commented out).
Solution
Created
tests/test_mainframe_callbacks.pywith mocked MQTT client.Test Results
Files Added
tests/test_mainframe_callbacks.py(159 lines)