Improve backend/apps/slack test coverage #1956
Conversation
Summary by CodeRabbit
WalkthroughThis pull request introduces a comprehensive suite of new unit tests targeting the Slack app's backend in the codebase. The tests cover admin actions, command handlers, event processing, model methods, management commands, and views, aiming to significantly improve code coverage and verification of key behaviors. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected. Possibly related PRs
Suggested labels
Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
backend/tests/apps/slack/views_test.py (1)
14-14: Move import to module level.Importing the view inside the test method is unusual and can make the test harder to understand. Consider moving this import to the top of the file.
+from apps.slack.views import slack_request_handler from django.test import RequestFactory class TestViews: """Tests for the Slack views.""" def test_slack_request_handler_works(self, mocker): """Tests that the view correctly calls the SlackRequestHandler.""" mock_slack_request_handler = mocker.patch("slack_bolt.adapter.django.SlackRequestHandler") mock_handler_instance = mock_slack_request_handler.return_value mock_handler_instance.handle.return_value = "mock_http_response" - from apps.slack.views import slack_request_handler - request = RequestFactory().post("/slack/events/")backend/tests/apps/slack/management/commands/slack_set_conversation_sync_messages_flags_test.py (1)
3-6: Consider formatting the long import statement.The import statement is quite long and could be more readable with better formatting.
-from apps.slack.management.commands.slack_set_conversation_sync_messages_flags import ( - SYNC_MESSAGES_CONVERSATIONS, - Command, -) +from apps.slack.management.commands.slack_set_conversation_sync_messages_flags import ( + Command, + SYNC_MESSAGES_CONVERSATIONS, +)backend/tests/apps/slack/commands/command_test.py (1)
32-33: Test assertion should use exact string matching.The test assumes the command name will be "/command" based on the class name "Command". This creates a tight coupling between the test and implementation details.
Consider making the assertion more explicit or testing the transformation logic separately:
- assert command_instance.command_name == "/command" + expected_name = "/command" # Make expectation explicit + assert command_instance.command_name == expected_namebackend/tests/apps/slack/admin/member_test.py (1)
32-34: Remove unnecessary pytest.approx for string comparison.
pytest.approxis designed for floating-point number comparisons, not strings. Using it for string comparison is unnecessary and potentially confusing.admin_instance.message_user.assert_called_with( - request, pytest.approx(f" assigned user for {mock_member}."), messages.SUCCESS + request, f"Successfully assigned user for {mock_member}.", messages.SUCCESS )backend/tests/apps/slack/models/message_test.py (1)
77-86: Consider testing the save parameter behavior.The test always passes
save=Truebut doesn't verify thatsave=Falsewould prevent the save call.Add a test case for the
save=Falsescenario:def test_update_data_creates_new_message_without_saving(self): slack_data = {"ts": "12345.001"} conversation = Conversation() with ( patch.object(Message, "save") as mock_save, patch.object(Message.objects, "get", side_effect=Message.DoesNotExist), ): message = Message.update_data(slack_data, conversation, save=False) mock_save.assert_not_called() assert isinstance(message, Message)backend/tests/apps/slack/management/commands/slack_sync_messages_test.py (1)
15-16: Remove unused mock variables or use them properly.The
mock_memberandmock_messagevariables are marked with# noqa: F841but aren't used in the test. Either remove them or integrate them into the test logic.- mock_member = mocker.patch(f"{self.target_module}.Member") # noqa: F841 - mock_message = mocker.patch(f"{self.target_module}.Message") # noqa: F841 + # These models are imported but not directly used in this test + mocker.patch(f"{self.target_module}.Member") + mocker.patch(f"{self.target_module}.Message")
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
backend/tests/apps/slack/admin/member_test.py(1 hunks)backend/tests/apps/slack/commands/command_test.py(1 hunks)backend/tests/apps/slack/common/handlers/users_test.py(1 hunks)backend/tests/apps/slack/events/event_test.py(1 hunks)backend/tests/apps/slack/events/member_joined_channel/catch_all_test.py(1 hunks)backend/tests/apps/slack/management/commands/slack_set_conversation_sync_messages_flags_test.py(1 hunks)backend/tests/apps/slack/management/commands/slack_sync_messages_test.py(1 hunks)backend/tests/apps/slack/models/member_test.py(1 hunks)backend/tests/apps/slack/models/message_test.py(1 hunks)backend/tests/apps/slack/views_test.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
backend/tests/apps/slack/events/member_joined_channel/catch_all_test.py (1)
backend/apps/slack/events/member_joined_channel/catch_all.py (1)
catch_all_handler(13-22)
backend/tests/apps/slack/views_test.py (1)
backend/apps/slack/views.py (1)
slack_request_handler(14-24)
backend/tests/apps/slack/management/commands/slack_set_conversation_sync_messages_flags_test.py (1)
backend/apps/slack/models/workspace.py (1)
get_default_workspace(27-34)
backend/tests/apps/slack/common/handlers/users_test.py (1)
backend/apps/slack/common/presentation.py (1)
EntityPresentation(7-25)
backend/tests/apps/slack/events/event_test.py (1)
backend/apps/slack/events/event.py (3)
EventBase(23-265)open_conversation(206-225)configure_events(30-37)
🔇 Additional comments (18)
backend/tests/apps/slack/views_test.py (3)
4-5: LGTM!The test class structure is appropriate for pytest-style testing.
11-12: LGTM!The mocking approach correctly isolates the view logic and verifies the integration with the SlackRequestHandler. Using a simple string as the mock response is appropriate for unit testing.
Also applies to: 20-20
1-2: Missing pytest import for mocker fixture.The test method uses the
mockerfixture butpytestis not imported. This will cause a runtime error.Add the missing import:
+import pytest from django.test import RequestFactoryLikely an incorrect or invalid review comment.
backend/tests/apps/slack/common/handlers/users_test.py (5)
7-24: LGTM!The fixture provides comprehensive and realistic mock user data with all necessary fields for testing the get_blocks function.
28-34: LGTM!The test properly handles the no results edge case with appropriate mocking and comprehensive assertions.
36-46: LGTM!The test comprehensively verifies that user data is correctly formatted into Slack message blocks with proper link formatting and user details.
48-54: LGTM!The parametrized test effectively covers both pagination scenarios with clear test cases and proper assertion logic.
Also applies to: 64-67
63-63: No missingsearch_queryargument in testThe
get_blocksfunction signature inbackend/apps/slack/common/handlers/users.pyis:def get_blocks( page: int = 1, search_query: str = "", limit: int = 10, presentation: EntityPresentation | None = None, ): …Since
search_queryhas a default value of"", callingget_blocks(presentation=presentation)is perfectly valid and intentionally uses an empty search filter. You can ignore the flagged “missing parameter” suggestion.Likely an incorrect or invalid review comment.
backend/tests/apps/slack/events/member_joined_channel/catch_all_test.py (1)
9-13: LGTM!The test effectively verifies the core functionality of the catch_all_handler - ensuring it properly acknowledges the Slack event by calling ack().
backend/tests/apps/slack/management/commands/slack_set_conversation_sync_messages_flags_test.py (1)
10-29: LGTM!The test comprehensively verifies the management command's ORM operations with proper mocking and assertion of the correct sequence of database updates.
backend/tests/apps/slack/models/member_test.py (4)
10-16: LGTM!The test effectively covers both named and unnamed member scenarios for the string representation, ensuring proper fallback behavior.
18-33: LGTM!The test provides comprehensive Slack user data and properly verifies that the from_slack method correctly populates the Member instance fields.
35-47: LGTM!The test properly simulates the new member creation scenario with appropriate exception mocking and verifies both database interaction and data population.
49-59: LGTM!The test effectively covers the existing member update scenario with proper mocking and verification of the update workflow.
backend/tests/apps/slack/models/message_test.py (1)
50-71: Excellent comprehensive test for the from_slack method.This test thoroughly covers all the key transformations and assignments performed by the
from_slackmethod, including timestamp conversion, boolean flag derivation, and object relationships.backend/tests/apps/slack/events/event_test.py (3)
31-37: Excellent property testing approach.These tests properly verify the derived template paths and default behaviors, ensuring the property methods work as expected.
64-78: Comprehensive error handling test for SlackApiError.This test properly verifies that specific Slack API errors are handled gracefully while ensuring the exception handling logic works correctly.
152-170: Well-structured configuration and registration tests.These tests properly verify the event discovery and registration mechanism, ensuring that the Slack app integration works correctly.
backend/tests/apps/slack/management/commands/slack_sync_messages_test.py
Outdated
Show resolved
Hide resolved
|
* Improve Slack app test coverage * improve test for slack admin * coderabbit suggestions * Update code --------- Co-authored-by: Arkadii Yakovets <arkadii.yakovets@owasp.org> Co-authored-by: Arkadii Yakovets <2201626+arkid15r@users.noreply.github.com>
2 participants
Proposed change
Improved the test coverage for the slack app by creating,updating some unit tests .
Resolves #1802
Our main aim was to target 80% + test coverage , So I changed 15 files which includes the files whose test coverage was below 80% . Somehow I wasn't able to Target 80% test coverage for 3 files
apps/slack/commands/events.py,apps/slack/events/member_joined_channel/catch_all.pyandapps/slack/management/commands/slack_sync_messages.py. I have improved the best I can , and for other files test coverage is above 80%.I have followed all the best practices. If any changes are required , I will be happy to cover them.
Updated Test Coverage-:
Overall Test Coverage -> 81.85%
Checklist
make check-testlocally; all checks and tests passed.