Bot Specific Executor Logs #1572
Conversation
Added and fixed test cases related to the same.
WalkthroughThe pull request introduces several modifications across various files, enhancing the functionality of scheduling jobs and web search actions. Key changes include updating method signatures to accept additional parameters, adding new methods for logging and retrieving executor logs, and adjusting test cases to validate these new functionalities. The updates improve flexibility in handling task scheduling and logging, ensuring better integration with bot instances and other components. Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
🧹 Outside diff range and nitpick comments (12)
kairon/events/executors/base.py (1)
Line range hint
1-24: Overall impact: Increased flexibility with considerations for subclasses.The changes to
ExecutorBaseincrease flexibility without altering the class's core purpose. However, consider the following:
- Review all subclasses of
ExecutorBaseto ensure they handle the newAnytype fordataappropriately.- Update any documentation or type hints in related code to reflect this change.
- Consider adding a class-level docstring to
ExecutorBaseexplaining the expected structure or types fordata, even if it's now more flexible.To ensure consistency across the codebase, please run the following:
#!/bin/bash # Description: Find subclasses of ExecutorBase and their log_task implementations # Find ExecutorBase subclasses echo "ExecutorBase subclasses:" rg --type python -n "class \w+\(ExecutorBase\)" -A 5 # Find log_task implementations in subclasses echo "log_task implementations in subclasses:" rg --type python -n "def log_task" -A 5 -g "!**/base.py"kairon/shared/events/processor.py (3)
6-25: LGTM: Well-implemented method with room for minor enhancements.The
get_executor_logsmethod is well-structured and efficient. It effectively uses pagination and optional filters, and the use of a generator function is appropriate for handling potentially large datasets.Consider the following minor improvements:
- Enhance the docstring to explain the optional
event_classandtask_typekwargs.- Add type hinting for the return value, e.g.,
-> Generator[Dict[str, Any], None, None].- Consider adding input validation for
start_idxandpage_sizeto ensure they are non-negative integers.Example of improved method signature and docstring:
@staticmethod def get_executor_logs(bot: str, start_idx: int = 0, page_size: int = 10, **kwargs) -> Generator[Dict[str, Any], None, None]: """ Get all executor logs data for a specific bot. @param bot: bot id. @param start_idx: start index for pagination (default: 0) @param page_size: number of logs to retrieve per page (default: 10) @param kwargs: Optional filters: - event_class: Filter logs by event class - task_type: Filter logs by task type @return: Generator yielding log entries as dictionaries. """
27-41: LGTM: Well-implemented method with room for minor enhancements.The
get_row_countmethod is well-structured and consistent with theget_executor_logsmethod. It effectively uses optional filters and directly returns the count of matching log entries.Consider the following minor improvements:
- Enhance the docstring to explain the optional
event_classandtask_typekwargs.- Add type hinting for the return value, e.g.,
-> int.- Consider adding input validation for the
botparameter to ensure it's not empty.Example of improved method signature and docstring:
@staticmethod def get_row_count(bot: str, **kwargs) -> int: """ Gets the count of rows in ExecutorLogs for a particular bot. @param bot: bot id @param kwargs: Optional filters: - event_class: Filter logs by event class - task_type: Filter logs by task type @return: Count of matching log entries @raises ValueError: If bot is empty """ if not bot: raise ValueError("Bot ID cannot be empty") # ... rest of the method
1-41: Overall, well-implemented new feature with minor suggestions for improvement.The
ExecutorProcessorclass successfully implements the functionality for retrieving and counting bot-specific executor logs, aligning with the PR objectives. The code is well-structured, efficient, and follows good practices such as using generator functions and consistent query construction.To further enhance the code:
- Consider implementing the suggested improvements for both methods, including more detailed docstrings and type hinting.
- Add error handling for potential exceptions that might occur during database operations.
- If not present elsewhere in the codebase, consider adding unit tests for these new methods to ensure their correctness and maintain them over time.
Great job on implementing this new feature!
kairon/actions/definitions/schedule.py (1)
140-146: LGTM: Method signature updated and task_type added.The changes to
add_schedule_jobmethod improve flexibility by allowing arbitrary keyword arguments and add context by including a task_type. This is consistent with the PR objectives of enhancing bot-specific executor logs.Consider adding a docstring update to reflect the new
**kwargsparameter:async def add_schedule_job(self, date_time: datetime, data: Dict, timezone: Text, **kwargs): """ Add a scheduled job. :param date_time: The date and time for the job to run :param data: Dictionary containing job data :param timezone: The timezone for the job :param kwargs: Additional keyword arguments for the job """ # ... rest of the methodtests/unit_test/cloud_utils_test.py (4)
306-306: LGTM: Model training test updated for new payload structureThe changes in this test method correctly update the payload structure for the model training scenario. The mock API call assertion and the CloudUtility.trigger_lambda call have been modified to use the new list of dictionaries format, ensuring proper validation of the model training case with the updated payload structure.
Consider improving readability by using a multi-line format for the CloudUtility.trigger_lambda call:
resp = CloudUtility.trigger_lambda( EventClass.model_training, [ {"name": "BOT", "value": "test"}, {"name": "USER", "value": "test_user"} ], task_type=TASK_TYPE.EVENT.value )Also applies to: 314-318
334-334: LGTM: Model testing test updated for new payload structureThe changes in this test method correctly update the payload structure for the model testing scenario. The mock API call assertion and the CloudUtility.trigger_lambda call have been modified to use the new list of dictionaries format, ensuring proper validation of the model testing case with the updated payload structure.
Consider improving readability by using a multi-line format for the CloudUtility.trigger_lambda call:
resp = CloudUtility.trigger_lambda( EventClass.model_testing, [ {"name": "BOT", "value": "test"}, {"name": "USER", "value": "test_user"} ], task_type=TASK_TYPE.EVENT.value )Also applies to: 342-346
362-362: LGTM: Data importer test updated for new payload structureThe changes in this test method correctly update the payload structure for the data importer scenario. The mock API call assertion and the CloudUtility.trigger_lambda call have been modified to use the new list of dictionaries format, ensuring proper validation of the data importer case with the updated payload structure.
Consider improving readability by using a multi-line format for the CloudUtility.trigger_lambda call:
resp = CloudUtility.trigger_lambda( EventClass.data_importer, [ {"name": "BOT", "value": "test"}, {"name": "USER", "value": "test_user"} ], task_type=TASK_TYPE.EVENT.value )Also applies to: 370-373
414-414: LGTM: Delete history test updated for new payload structureThe changes in this test method correctly update the payload structure for the delete history scenario. The mock API call assertion and the CloudUtility.trigger_lambda call have been modified to use the new list of dictionaries format, ensuring proper validation of the delete history case with the updated payload structure.
Consider improving readability by using a multi-line format for the CloudUtility.trigger_lambda call:
resp = CloudUtility.trigger_lambda( EventClass.delete_history, [ {"name": "BOT", "value": "test"}, {"name": "USER", "value": "test_user"} ], task_type=TASK_TYPE.EVENT.value )Also applies to: 422-425
kairon/api/app/routers/bot/bot.py (1)
1709-1727: New endpoint for retrieving executor logs addedThe new endpoint
get_executor_logshas been implemented to retrieve executor logs based on provided filters. Here are some observations:
- The endpoint is properly secured with the
TESTER_ACCESSscope.- It accepts optional query parameters for filtering:
start_idx,page_size,event_class, andtask_type.- The implementation uses the
ExecutorProcessorto fetch logs and count.The implementation looks good overall, but there are a few suggestions for improvement:
- Consider adding input validation for
start_idxandpage_sizeto ensure they are non-negative integers.- It might be helpful to add some documentation about the possible values for
event_classandtask_type.- Consider adding error handling for potential exceptions that might occur during log retrieval.
Here's a suggested improvement for input validation:
from fastapi import Query @router.get("/executor/logs", response_model=Response) async def get_executor_logs( start_idx: int = Query(0, ge=0), page_size: int = Query(10, ge=1, le=100), event_class: str = None, task_type: str = None, current_user: User = Security(Authentication.get_current_user_and_bot, scopes=TESTER_ACCESS) ): # ... rest of the functionThis change adds validation to ensure
start_idxis non-negative andpage_sizeis between 1 and 100.🧰 Tools
🪛 Ruff
1713-1713: Do not perform function call
Securityin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
tests/integration_test/services_test.py (1)
2569-2687: Comprehensive test coverage, but could benefit from refactoringThe test function provides thorough coverage of different scenarios for retrieving executor logs. It properly verifies the structure and content of the returned logs for various task types and event classes.
However, there's some code duplication in the assertions that could be refactored to improve maintainability.
Consider creating a helper function to reduce duplication in assertions. For example:
def assert_log_structure(log, expected_task_type, expected_event_class, expected_status, expected_data): assert log["task_type"] == expected_task_type assert log["event_class"] == expected_event_class assert log["status"] == expected_status assert log["data"] == expected_data assert log["executor_log_id"] assert log['bot'] == pytest.bot # In the test function assert_log_structure( actual["data"]["logs"][0], "Callback", "pyscript_evaluator", "Completed", { 'source_code': 'bot_response = "test - this is from callback test"', 'predefined_objects': { # ... (rest of the expected data) } } )This approach would make the test function more concise and easier to maintain.
tests/unit_test/data_processor/executor_processor_test.py (1)
27-36: Ensure consistent use of single or double quotes.Python allows both single
'and double"quotes for strings, but it's good practice to be consistent within a codebase or file.Consider standardizing the string quotations. For example, use double quotes consistently.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
- kairon/actions/definitions/schedule.py (2 hunks)
- kairon/actions/definitions/web_search.py (1 hunks)
- kairon/api/app/routers/bot/bot.py (2 hunks)
- kairon/events/executors/base.py (2 hunks)
- kairon/shared/actions/utils.py (1 hunks)
- kairon/shared/cloud/utils.py (4 hunks)
- kairon/shared/events/processor.py (1 hunks)
- tests/integration_test/action_service_test.py (5 hunks)
- tests/integration_test/services_test.py (2 hunks)
- tests/unit_test/action/action_test.py (11 hunks)
- tests/unit_test/cloud_utils_test.py (10 hunks)
- tests/unit_test/data_processor/executor_processor_test.py (1 hunks)
🔥 Files not summarized due to errors (2)
- tests/integration_test/action_service_test.py: Error: Server error: no LLM provider could handle the message
- tests/integration_test/services_test.py: Error: Server error: no LLM provider could handle the message
🧰 Additional context used
🪛 Ruff
kairon/api/app/routers/bot/bot.py
1713-1713: Do not perform function call
Securityin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
tests/unit_test/action/action_test.py
3316-3317: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
3338-3339: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
tests/unit_test/cloud_utils_test.py
312-313: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
340-341: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
368-369: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
420-421: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
tests/unit_test/data_processor/executor_processor_test.py
12-12: Use capitalized environment variable
SYSTEM_FILEinstead ofsystem_fileReplace
system_filewithSYSTEM_FILE(SIM112)
🪛 Gitleaks
tests/integration_test/services_test.py
2373-2373: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
tests/unit_test/data_processor/executor_processor_test.py
82-82: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (36)
kairon/events/executors/base.py (1)
2-2: LGTM: Import statement forAnytype hint.The addition of
from typing import Anyis appropriate and necessary for the changes made to thelog_taskmethod signature. It follows Python's import style guidelines and is correctly placed at the top of the file.kairon/shared/events/processor.py (1)
4-4: LGTM: Class definition is appropriate.The
ExecutorProcessorclass is well-defined as a utility class for processing executor logs. The use of static methods is appropriate for grouping related functions that don't require instance-specific state.kairon/actions/definitions/schedule.py (2)
28-28: LGTM: New import statement added.The import of
TASK_TYPEfromkairon.shared.data.constantis correctly added and will be used in theadd_schedule_jobmethod.
Line range hint
1-214: Summary: Changes align with PR objectives and enhance functionality.The modifications to this file, including the new import and updates to the
add_schedule_jobmethod, successfully implement the intended enhancements for bot-specific executor logs. The changes improve flexibility in handling task scheduling and provide better context for scheduled jobs.No major issues were found, and the implementation is consistent with the PR objectives.
tests/unit_test/cloud_utils_test.py (4)
176-176: LGTM: Payload structure updated consistentlyThe changes in this test method correctly reflect the new payload structure, updating both the mock API call assertion and the ExecutorLogs assertion. This ensures that the test accurately validates the new format of the payload and logs.
Also applies to: 185-186, 191-192, 196-196
221-221: LGTM: Failure scenario updated for new payload structureThe changes in this test method correctly update the payload structure for the failure scenario. Both the mock API call assertion and the ExecutorLogs assertion have been modified to reflect the new list of dictionaries format, ensuring proper validation of the failure case with the updated payload structure.
Also applies to: 230-231, 235-236, 241-241
262-262: LGTM: Existing log scenario updated for new payload structureThe changes in this test method successfully update the payload structure for the scenario where an executor log already exists. The mock API call assertion, ExecutorLogs assertion, and log_task call have all been modified to use the new list of dictionaries format, ensuring proper validation of this specific case with the updated payload structure.
Also applies to: 271-272, 280-280, 288-288
Line range hint
1-425: Overall assessment: Changes are well-implemented and consistentThe modifications in this file successfully update the payload structure from a dictionary to a list of dictionaries across multiple test methods. The changes are consistent and maintain the integrity of the test suite. Minor suggestions for improving readability have been provided, including the use of multi-line formats for function calls and combining nested
withstatements.Great job on maintaining consistency throughout the file!
🧰 Tools
🪛 Ruff
268-269: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
kairon/shared/actions/utils.py (1)
493-493: LGTM! Enhancement for bot-specific web search.The addition of the "bot" parameter to the
request_bodydictionary is a good improvement. It allows for bot-specific web search functionality, which aligns with the PR objectives of adding bot-specific executor logs. This change enhances the flexibility and context-awareness of the web search feature.tests/integration_test/action_service_test.py (6)
8315-8315: LGTM: Bot-specific parameter added to web search functionThe addition of the 'bot' parameter to the web search function aligns well with the PR objectives of implementing bot-specific executor logs. This change enhances the test case to verify that bot-specific information is correctly passed to the web search functionality.
8440-8440: LGTM: Consistent addition of bot parameter in test responseThe inclusion of the 'bot' parameter in the JSON params matcher for the test response is consistent with the previous changes. This modification ensures that the bot-specific information is correctly included in the web search request parameters, further validating the implementation of bot-specific functionality.
8548-8548: LGTM: Consistent bot parameter addition across test casesThe addition of the 'bot' parameter to the kwargs assertion in this test case maintains consistency with the previous modifications. This change ensures that the bot-specific information is correctly handled across different web search scenarios, strengthening the overall test coverage for the new bot-specific functionality.
8668-8668: LGTM: Continued consistency in bot parameter additionThe inclusion of the 'bot' parameter in the kwargs assertion for this test case maintains the consistency observed in previous modifications. This change further reinforces the proper handling of bot-specific information across various web search scenarios, contributing to a robust test suite for the new functionality.
8788-8788: LGTM: Consistent implementation of bot-specific parameterThe addition of the 'bot' parameter to the kwargs assertion in this final test case completes a consistent pattern of modifications throughout the file. This change, along with all the previous ones, demonstrates a thorough and systematic approach to incorporating bot-specific information in the web search functionality across various test scenarios.
The consistency observed across all modified test cases is commendable and suggests a well-thought-out implementation of the new bot-specific executor logs feature.
Line range hint
8315-8788: Overall: Excellent implementation of bot-specific parameters in web search testsThe changes made to this file demonstrate a systematic and thorough approach to incorporating bot-specific information in the web search functionality tests. Key points:
- Consistent addition of the 'bot' parameter across all modified test cases.
- Enhanced test coverage for the new bot-specific executor logs feature.
- Alignment with the PR objectives of implementing bot-specific functionality.
These modifications significantly improve the test suite's ability to validate the new bot-specific features, ensuring robust and reliable functionality in various scenarios.
tests/integration_test/services_test.py (1)
2357-2687: Overall assessment: Good test coverage with room for improvementThe test file provides comprehensive coverage for executor logs, which is commendable. It tests various scenarios and verifies the structure and content of the logs thoroughly.
However, there are a few areas that could be improved:
- Security: There's an exposed API key in the test data that should be handled more securely.
- Code structure: Both the fixture and the test function are quite long and could benefit from refactoring to improve readability and maintainability.
Addressing these points will enhance the overall quality and security of the test suite.
🧰 Tools
🪛 Gitleaks
2373-2373: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
kairon/shared/cloud/utils.py (2)
128-163: Ensure all possibleevent_classcases are handledThe
get_bot_id_from_env_datamethod handles specificevent_classcases but may not cover all possible values ofEventClass. Consider adding a default case or handling for any newevent_classvalues that may be added in the future to prevent potential issues.
124-124: Potential issue with assigningNonetolog.botDirectly assigning
bot_idtolog.botwithout validation might result inlog.botbeingNone. Iflog.botis expected to always contain a valid bot ID, ensure thatbot_idis notNonebefore assignment or handle theNonecase appropriately.You can use the following script to check for any existing logs where
botisNone:tests/unit_test/action/action_test.py (18)
3081-3081: Variable 'bot' assigned correctlyThe variable
botis correctly assigned with"test_bot"for use in the test.
3119-3119: Updated method call with 'bot' parameterThe call to
ActionUtility.perform_web_searchis correctly updated to include the newbotparameter.
3126-3127: Assertion updated with 'bot' argumentThe assertion now includes the
botparameter incalled_args.args[1], which aligns with the updated method signature.
3133-3133: Variable 'bot' assigned correctlyThe variable
botis correctly assigned with"test_bot"for use in the test.
3176-3176: Updated method call with 'bot' parameterThe call to
ActionUtility.perform_web_searchis correctly updated to include the newbotparameter.
3186-3186: Assertion updated with 'bot' argumentThe assertion now includes the
botparameter incalled_args.args[1], which aligns with the updated method signature.
3245-3245: Variable 'bot' assigned correctlyThe variable
botis correctly assigned with"test_bot"for use in the test.
3257-3257: Assertion updated with 'bot' argumentThe assertion includes the
botparameter, ensuring consistency with the method call.
3262-3262: Updated method call with 'bot' parameterThe call to
ActionUtility.perform_web_searchincludes thebotparameter as expected.
3273-3273: Variable 'bot' assigned correctlyThe variable
botis correctly assigned with"test_bot"for use in the test.
3287-3287: Assertion updated with 'bot' argumentThe assertion includes the
botparameter, maintaining consistency with the updated method signature.
3292-3292: Updated method call with 'bot' parameterThe
perform_web_searchfunction call now correctly includes thebotparameter.
3303-3303: Variable 'bot' assigned correctlyThe variable
botis correctly assigned with"test_bot"for use in the test.
3312-3312: Assertion updated with 'bot' argumentThe assertion now includes the
botparameter, aligning with the method's expected arguments.
3318-3318: Ensure exception is properly testedThe test correctly checks for the
ActionFailureexception when performing web search.
3324-3324: Variable 'bot' assigned correctlyThe variable
botis correctly assigned with"test_bot"for use in the test.
3334-3334: Assertion updated with 'bot' argumentThe assertion includes the
botparameter, ensuring the test reflects the updated function signature.
3340-3340: Ensure exception is properly testedThe test correctly checks for the
ActionFailureexception when performing web search with an error code not equal to zero.
| @@ -13,7 +13,7 @@ class ExecutorBase: | |||
| def execute_task(self, event_class: EventClass, data: dict, **kwargs): | |||
| raise NotImplementedError("Provider not implemented") | |||
|
|
|||
| def log_task(self, event_class: EventClass, task_type: TASK_TYPE, data: dict, status: EVENT_STATUS, **kwargs): | |||
| def log_task(self, event_class: EventClass, task_type: TASK_TYPE, data: Any, status: EVENT_STATUS, **kwargs): | |||
There was a problem hiding this comment.
💡 Codebase verification
Issue Detected: Type Inconsistency in log_task Method
The log_task method signature has been changed from dict to Any, but its implementation in CloudUtility.log_task still expects data as a dict. This inconsistency can lead to type safety issues and potential runtime errors.
Recommendations:
-
Revert the
dataParameter Type:- Change the
dataparameter type back todictin thelog_taskmethod to maintain consistency withCloudUtility.log_task.
- Change the
-
Enhance Type Handling:
- If flexibility is required, update
CloudUtility.log_taskto handleAnytype fordatawith appropriate type checks and validations.
- If flexibility is required, update
-
Update Documentation and Type Annotations:
- Ensure all type annotations across the codebase are consistent and accurately reflect the expected data types.
- Add documentation to clarify the expected types for the
dataparameter.
-
Run Comprehensive Tests:
- Execute unit and integration tests to verify that type changes do not introduce unintended side effects.
🔗 Analysis chain
Approved: log_task method signature update, with considerations.
The change from dict to Any for the data parameter type allows for greater flexibility. However, consider the following points:
- Ensure that all subclasses and calling code are updated to handle this change appropriately.
- Consider adding type checking or documentation to clarify what types of
dataare actually supported. - Verify that
CloudUtility.log_task()can handle various data types safely.
To ensure this change doesn't introduce unexpected behavior, please run the following verification:
| website=action_config.get("website"), | ||
| bot=self.bot) |
There was a problem hiding this comment.
💡 Codebase verification
Some calls to perform_web_search are missing the bot parameter.
Please update all instances of perform_web_search to include the bot parameter to ensure consistency and proper functionality.
Affected locations:
tests/unit_test/action/action_test.pytests/integration_test/action_service_test.py
🔗 Analysis chain
LGTM! Consider updating docstring and verify other method calls.
The addition of the bot parameter to the perform_web_search method call is correct and consistent with the changes mentioned in the summary. This change allows for better flexibility and potentially improved separation of concerns.
However, to ensure completeness:
- Please verify that all other calls to
perform_web_searchin the codebase have been updated to include this new parameter. - Consider updating the docstring of the
executemethod to reflect this change, mentioning that it now passes the bot information to the web search function.
To verify other calls to perform_web_search, you can run the following script:
This will help ensure that all relevant calls have been updated consistently across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all calls to perform_web_search and verify they include the bot parameter
# Search for perform_web_search calls
rg "perform_web_search\(" --type python -g "!test_*" -A 3
Length of output: 90
Script:
#
#!/bin/bash
# Description: Find all calls to perform_web_search and verify they include the bot parameter
# Search for perform_web_search calls without specifying the file type
rg "perform_web_search\s*\(" --glob "!test_*" -A 3
Length of output: 6976
| with patch.dict(Utility.environment, mock_env): | ||
| with mock.patch('botocore.client.BaseClient._make_api_call', new=__mock_make_api_call): |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a single with statement for multiple contexts
To improve code readability and adhere to best practices, consider using a single with statement with multiple contexts instead of nested with statements. This change can be applied to several test methods in this file.
Here's an example of how you can refactor the with statements:
with patch.dict(Utility.environment, mock_env), \
mock.patch('botocore.client.BaseClient._make_api_call', new=__mock_make_api_call):
# Test code hereApply this change to all occurrences of nested with statements in the file.
Also applies to: 340-341, 368-369, 420-421
🧰 Tools
🪛 Ruff
312-313: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
| async def get_executor_logs( | ||
| start_idx: int = 0, page_size: int = 10, | ||
| event_class: str = None, task_type: str = None, | ||
| current_user: User = Security(Authentication.get_current_user_and_bot, scopes=TESTER_ACCESS) |
There was a problem hiding this comment.
Avoid function call in argument default
The static analysis tool Ruff has flagged a potential issue with performing a function call in the argument default for the current_user parameter. This is generally discouraged as it can lead to unexpected behavior with mutable default arguments.
To address this, you can modify the function signature as follows:
from fastapi import Depends
@router.get("/executor/logs", response_model=Response)
async def get_executor_logs(
start_idx: int = 0,
page_size: int = 10,
event_class: str = None,
task_type: str = None,
current_user: User = Depends(Authentication.get_current_user_and_bot)
):
# ... rest of the functionThis change uses FastAPI's Depends function, which is the recommended way to handle dependency injection and security in FastAPI. It will still apply the TESTER_ACCESS scope check but avoids the issue with function calls in default arguments.
🧰 Tools
🪛 Ruff
1713-1713: Do not perform function call
Securityin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
| "displayLabel": "", | ||
| "telemetry-uid": "None", | ||
| "telemetry-sid": "None", | ||
| "is_integration_user": False, | ||
| "bot": pytest.bot, | ||
| "account": 8, | ||
| "channel_type": "chat_client" | ||
| }, | ||
| "intent_ranking": [ | ||
| { | ||
| "name": "pyscript", | ||
| "confidence": 1 | ||
| } | ||
| ] | ||
| }, | ||
| "kairon_user_msg": None, | ||
| "session_started": None | ||
| } | ||
| }, | ||
| executor_log_id=ObjectId().__str__(), status=EVENT_STATUS.INITIATED.value) | ||
| executor.log_task(event_class=EventClass.pyscript_evaluator.value, task_type=TASK_TYPE.CALLBACK.value, | ||
| data={ | ||
| "source_code": "bot_response = \"test - this is from callback test\"", | ||
| "predefined_objects": { | ||
| "req": { | ||
| "type": "GET", | ||
| "body": None, | ||
| "params": {} | ||
| }, | ||
| "req_host": "127.0.0.1", | ||
| "action_name": "clbk1", | ||
| "callback_name": "test", | ||
| "bot": pytest.bot, | ||
| "sender_id": "[email protected]", | ||
| "channel": "unsupported (None)", | ||
| "metadata": { | ||
| "first_name": "rajan", | ||
| "bot": pytest.bot | ||
| }, | ||
| "identifier": "01928eb52813799e81c56803ecf39e6e", | ||
| "callback_url": "http://localhost:5059/callback/d/01928eb52813799e81c56803ecf39e6e/98bxWAMY9HF40La5AKQjEb-0JqaDTKX_Mmmmmmmmmmm", | ||
| "execution_mode": "async", | ||
| "state": 0, | ||
| "is_valid": True | ||
| } | ||
| }, | ||
| executor_log_id=ObjectId().__str__(), status=EVENT_STATUS.COMPLETED.value, | ||
| response={ | ||
| "ResponseMetadata": { | ||
| "RequestId": "0da1cdd1-1702-473b-8109-67a39f990835", | ||
| "HTTPStatusCode": 200, | ||
| "HTTPHeaders": { | ||
| "date": "Tue, 15 Oct 2024 07:38:01 GMT", | ||
| "content-type": "application/json", | ||
| "content-length": "740", | ||
| "connection": "keep-alive", | ||
| "x-amzn-requestid": "0da1cdd1-1702-473b-8109-67a39f9sdlksldksl", | ||
| "x-amzn-remapped-content-length": "0", | ||
| "x-amz-executed-version": "$LATEST", | ||
| "x-amz-log-result": "sdskjdksjdkjskdjskjdksj", | ||
| "x-amzn-trace-id": "Root=1-670e1bd6-25e7667c7c6ce37f09a9afc7;Sampled=1;Lineage=1:d072fc6c:0" | ||
| }, | ||
| "RetryAttempts": 0 | ||
| }, | ||
| "StatusCode": 200, | ||
| "LogResult": "sldklskdlskdlskdlk", | ||
| "ExecutedVersion": "$LATEST", | ||
| "Payload": { | ||
| "statusCode": 200, | ||
| "statusDescription": "200 OK", | ||
| "isBase64Encoded": False, | ||
| "headers": { | ||
| "Content-Type": "text/html; charset=utf-8" | ||
| }, | ||
| "body": { | ||
| "req": { | ||
| "type": "GET", | ||
| "body": None, | ||
| "params": {} | ||
| }, | ||
| "req_host": "127.0.0.1", | ||
| "action_name": "clbk1", | ||
| "callback_name": "test", | ||
| "bot": pytest.bot, | ||
| "sender_id": "[email protected]", | ||
| "channel": "unsupported (None)", | ||
| "metadata": { | ||
| "first_name": "rajan", | ||
| "bot": pytest.bot | ||
| }, | ||
| "identifier": "01928eb52813799e81c56803ecf39e6e", | ||
| "callback_url": "http://localhost:5059/callback/d/01928eb52813799e81c56803ecf39e6e/98bxWAMY9HF40La5AKQjEb-0JqaDTKX_Mmmmmmmmmmm", | ||
| "execution_mode": "async", | ||
| "state": 0, | ||
| "is_valid": True, | ||
| "bot_response": "test - this is from callback test" | ||
| } | ||
| } | ||
| } | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Security concern: Exposed API key in test data
There's a potential security risk in the test data. An API key is exposed in the source code of one of the log entries.
Consider replacing the API key with a placeholder or environment variable to prevent accidental exposure. For example:
-"D360-API-KEY' : '6aYsIpgihlZRvrkU6Bs39R3RAK'"
+"D360-API-KEY': os.getenv('TEST_API_KEY')"Also, consider refactoring this fixture to improve readability. The current implementation is quite long and could be split into smaller, more focused helper functions.
You could create separate functions for each type of log entry, like:
def create_model_training_log(executor):
# Logic for model training log
def create_pyscript_evaluator_log(executor):
# Logic for pyscript evaluator log
# ... and so on
@pytest.fixture()
def get_executor_logs():
executor = ExecutorBase()
create_model_training_log(executor)
create_pyscript_evaluator_log(executor)
# ... call other helper functionsThis approach would make the fixture more maintainable and easier to understand.
🧰 Tools
🪛 Gitleaks
2373-2373: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
| "value": "import requests\n\nurl=\"https://waba-v2.360dialog.io/messages\"\n\nheaders={'Content-Type': 'application/json', 'D360-API-KEY' : '6aYsIpgihlZRvrkU6Bs39R3RAK'}\n\ncontacts = ['919657055022','918210057905']\ncontacts = [\"919515991685\"]\nbody = {\n \"messaging_product\": \"whatsapp\",\n \"recipient_type\": \"individual\",\n \"to\": \"9199991685\",\n \"type\": \"template\",\n \"template\": {\n \"namespace\": \"54500467_f322_4595_becd_41555889bfd8\",\n \"language\": {\n \"policy\": \"deterministic\",\n \"code\": \"en\"\n },\n \"name\": \"schedule_action_test\"\n }\n}\n\nfor contact in contacts:\n body[\"to\"] = contact\n resp = requests.post(url, headers=headers, data=json.dumps(body))\n resp = resp.json()\n print(resp[\"messages\"])\n\nbot_response = 'this from callback pyscript'" | ||
| }, |
There was a problem hiding this comment.
Anonymize personal data such as phone numbers in test code.
The contacts list contains realistic phone numbers, which could be actual personal identifiers. For security and privacy reasons, it's best to replace them with fake or anonymized data.
Apply this diff to anonymize the phone numbers:
-contacts = ['919657055022','918210057905']
+contacts = ['919XXXXXXX1','918XXXXXXX2']Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 Gitleaks
82-82: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
| data=[ | ||
| { | ||
| "name": "SOURCE_CODE", | ||
| "value": "import requests\n\nurl=\"https://waba-v2.360dialog.io/messages\"\n\nheaders={'Content-Type': 'application/json', 'D360-API-KEY' : '6aYsIpgihlZRvrkU6Bs39R3RAK'}\n\ncontacts = ['919657055022','918210057905']\ncontacts = [\"919515991685\"]\nbody = {\n \"messaging_product\": \"whatsapp\",\n \"recipient_type\": \"individual\",\n \"to\": \"9199991685\",\n \"type\": \"template\",\n \"template\": {\n \"namespace\": \"54500467_f322_4595_becd_41555889bfd8\",\n \"language\": {\n \"policy\": \"deterministic\",\n \"code\": \"en\"\n },\n \"name\": \"schedule_action_test\"\n }\n}\n\nfor contact in contacts:\n body[\"to\"] = contact\n resp = requests.post(url, headers=headers, data=json.dumps(body))\n resp = resp.json()\n print(resp[\"messages\"])\n\nbot_response = 'this from callback pyscript'" |
There was a problem hiding this comment.
Remove hardcoded API key to prevent security risks.
Line 82 contains a hardcoded API key '6aYsIpgihlZRvrkU6Bs39R3RAK' within the headers. Exposing API keys in code can lead to unauthorized access and security vulnerabilities. It's important to secure this key and avoid committing it to the repository.
Consider retrieving the API key from environment variables or a secure configuration file used only for testing purposes.
Apply this diff to remove the hardcoded API key:
-"headers={'Content-Type': 'application/json', 'D360-API-KEY' : '6aYsIpgihlZRvrkU6Bs39R3RAK'}\n\ncontacts = ['919657055022','918210057905']\ncontacts = [\"919515991685\"]\nbody = {\n \"messaging_product\": \"whatsapp\",\n \"recipient_type\": \"individual\",\n \"to\": \"9199991685\",\n \"type\": \"template\",\n \"template\": {\n \"namespace\": \"54500467_f322_4595_becd_41555889bfd8\",\n \"language\": {\n \"policy\": \"deterministic\",\n \"code\": \"en\"\n },\n \"name\": \"schedule_action_test\"\n }\n}\n\nfor contact in contacts:\n body[\"to\"] = contact\n resp = requests.post(url, headers=headers, data=json.dumps(body))\n resp = resp.json()\n print(resp[\"messages\"])\n\nbot_response = 'this from callback pyscript'"
+"api_key = os.environ.get('D360_API_KEY', 'your-default-api-key')\nheaders={'Content-Type': 'application/json', 'D360-API-KEY' : api_key}\n\ncontacts = ['919XXXXXXXXX','918XXXXXXXXX']\nbody = {...}\n\nfor contact in contacts:\n body[\"to\"] = contact\n resp = requests.post(url, headers=headers, data=json.dumps(body))\n resp = resp.json()\n print(resp[\"messages\"])\n\nbot_response = 'this from callback pyscript'"Make sure to set the environment variable D360_API_KEY appropriately in your testing environment.
Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 Gitleaks
82-82: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
| @pytest.fixture(autouse=True, scope='class') | ||
| def init_connection(self): | ||
| connect(**Utility.mongoengine_connection()) | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Database connection is set to production environment
The Utility.environment["database"]["url"] is set to the production database (mongodb://localhost:27017/conversations) as the env is set to prod in system.yaml. This may lead to tests interacting with production data. Please ensure that the test environment configuration points to a test database.
🔗 Analysis chain
Validate database connection parameters in init_connection.
Ensure that the database connection parameters retrieved by Utility.mongoengine_connection() are appropriate for the test environment to prevent unintended interactions with production data.
Run the following script to verify the MongoDB connection settings:
Please run the following script to retrieve the database URL from Utility.environment:
Please run the following script to extract the database.url from system.yaml:
Please run the following script to extract the env value from system.yaml:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the MongoDB connection is to a test database.
# Expected: Connection settings point to a test database.
grep -A 5 'def mongoengine_connection' kairon/shared/utils.py
Length of output: 303
Script:
#!/bin/bash
# Description: Retrieve the database URL from Utility.environment
grep -A 5 'Utility.environment' kairon/shared/utils.py
Length of output: 8841
Script:
#!/bin/bash
# Description: Extract the database URL from system.yaml
grep -E '^\s*url\s*:' system.yaml | sed 's/^\s*url\s*:\s*//'
Length of output: 687
Script:
#!/bin/bash
# Description: Extract the env value from system.yaml
grep -E '^\s*env\s*:' system.yaml | sed 's/^\s*env\s*:\s*//'
Length of output: 67
| with mock.patch.dict(Utility.environment, {'web_search': {"trigger_task": False, "url": search_engine_url}}): | ||
| with pytest.raises(ActionFailure, match=re.escape('Failed to execute the url: Got non-200 status code: 500 {"data": []}')): |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify nested 'with' statements into a single context manager
You can combine the nested with statements into a single with statement with multiple contexts for better readability.
Apply this diff to simplify the with statements:
- with patch("kairon.shared.utils.Utility.environment", new=mock_environment):
- with pytest.raises(ActionFailure, match='The requested resource was not found on the server.'):
+ with patch("kairon.shared.utils.Utility.environment", new=mock_environment), \
+ pytest.raises(ActionFailure, match='The requested resource was not found on the server.'):Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 Ruff
3316-3317: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
| with mock.patch.dict(Utility.environment, {'web_search': {"trigger_task": False, "url": search_engine_url}}): | ||
| with pytest.raises(ActionFailure, match=re.escape(f"{result}")): |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify nested 'with' statements into a single context manager
You can combine the nested with statements into a single with statement with multiple contexts for better readability.
Apply this diff to simplify the with statements:
- with patch("kairon.shared.utils.Utility.environment", new=mock_environment):
- with pytest.raises(ActionFailure, match=re.escape(f"{result}")):
+ with patch("kairon.shared.utils.Utility.environment", new=mock_environment), \
+ pytest.raises(ActionFailure, match=re.escape(f"{result}")):Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 Ruff
3338-3339: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
Added and fixed test cases related to the same.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
tests/unit_test/data_processor/executor_processor_test.py (1)
12-12: Use uppercase for environment variable name.It's a Python convention to use uppercase letters for environment variable names. Consider changing
"system_file"to"SYSTEM_FILE"to adhere to this standard.-os.environ["system_file"] = "./tests/testing_data/system.yaml" +os.environ["SYSTEM_FILE"] = "./tests/testing_data/system.yaml"🧰 Tools
🪛 Ruff
12-12: Use capitalized environment variable
SYSTEM_FILEinstead ofsystem_fileReplace
system_filewithSYSTEM_FILE(SIM112)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- tests/unit_test/data_processor/executor_processor_test.py (1 hunks)
🧰 Additional context used
🪛 Ruff
tests/unit_test/data_processor/executor_processor_test.py
12-12: Use capitalized environment variable
SYSTEM_FILEinstead ofsystem_fileReplace
system_filewithSYSTEM_FILE(SIM112)
🔇 Additional comments (1)
tests/unit_test/data_processor/executor_processor_test.py (1)
1-401: Overall, the test file is well-structured and comprehensive.The test file for the ExecutorProcessor class covers a wide range of scenarios and edge cases, which is commendable. It demonstrates a good understanding of the class's functionality and ensures thorough testing.
However, there are a few areas where the code could be improved:
- Use of uppercase for environment variable names
- Refactoring the large
get_executor_logsfixture for better maintainability- Removing or masking sensitive information in test data
- Parameterizing test methods to reduce duplication and improve readability
Addressing these points would enhance the overall quality and maintainability of the test suite.
🧰 Tools
🪛 Ruff
12-12: Use capitalized environment variable
SYSTEM_FILEinstead ofsystem_fileReplace
system_filewithSYSTEM_FILE(SIM112)
| "displayLabel": "", | ||
| "telemetry-uid": "None", | ||
| "telemetry-sid": "None", | ||
| "is_integration_user": False, | ||
| "bot": '66cd84e4f206edf5b776d6d8', | ||
| "account": 8, | ||
| "channel_type": "chat_client" | ||
| }, | ||
| "intent_ranking": [ | ||
| { | ||
| "name": "pyscript", | ||
| "confidence": 1 | ||
| } | ||
| ] | ||
| }, | ||
| "kairon_user_msg": None, | ||
| "session_started": None | ||
| } | ||
| }, | ||
| executor_log_id=ObjectId().__str__(), status=EVENT_STATUS.INITIATED.value) | ||
| executor.log_task(event_class=EventClass.pyscript_evaluator.value, task_type=TASK_TYPE.CALLBACK.value, | ||
| data={ | ||
| "source_code": "bot_response = \"test - this is from callback test\"", | ||
| "predefined_objects": { | ||
| "req": { | ||
| "type": "GET", | ||
| "body": None, | ||
| "params": {} | ||
| }, | ||
| "req_host": "127.0.0.1", | ||
| "action_name": "clbk1", | ||
| "callback_name": "test", | ||
| "bot": '66cd84e4f206edf5b776d6d8', | ||
| "sender_id": "[email protected]", | ||
| "channel": "unsupported (None)", | ||
| "metadata": { | ||
| "first_name": "rajan", | ||
| "bot": '66cd84e4f206edf5b776d6d8' | ||
| }, | ||
| "identifier": "01928eb52813799e81c56803ecf39e6e", | ||
| "callback_url": "http://localhost:5059/callback/d/01928eb52813799e81c56803ecf39e6e/98bxWAMY9HF40La5AKQjEb-0JqaDTKX_Mmmmmmmmmmm", | ||
| "execution_mode": "async", | ||
| "state": 0, | ||
| "is_valid": True | ||
| } | ||
| }, | ||
| executor_log_id=ObjectId().__str__(), status=EVENT_STATUS.COMPLETED.value, | ||
| response={ | ||
| "ResponseMetadata": { | ||
| "RequestId": "0da1cdd1-1702-473b-8109-67a39f990835", | ||
| "HTTPStatusCode": 200, | ||
| "HTTPHeaders": { | ||
| "date": "Tue, 15 Oct 2024 07:38:01 GMT", | ||
| "content-type": "application/json", | ||
| "content-length": "740", | ||
| "connection": "keep-alive", | ||
| "x-amzn-requestid": "0da1cdd1-1702-473b-8109-67a39f9sdlksldksl", | ||
| "x-amzn-remapped-content-length": "0", | ||
| "x-amz-executed-version": "$LATEST", | ||
| "x-amz-log-result": "sdskjdksjdkjskdjskjdksj", | ||
| "x-amzn-trace-id": "Root=1-670e1bd6-25e7667c7c6ce37f09a9afc7;Sampled=1;Lineage=1:d072fc6c:0" | ||
| }, | ||
| "RetryAttempts": 0 | ||
| }, | ||
| "StatusCode": 200, | ||
| "LogResult": "sldklskdlskdlskdlk", | ||
| "ExecutedVersion": "$LATEST", | ||
| "Payload": { | ||
| "statusCode": 200, | ||
| "statusDescription": "200 OK", | ||
| "isBase64Encoded": False, | ||
| "headers": { | ||
| "Content-Type": "text/html; charset=utf-8" | ||
| }, | ||
| "body": { | ||
| "req": { | ||
| "type": "GET", | ||
| "body": None, | ||
| "params": {} | ||
| }, | ||
| "req_host": "127.0.0.1", | ||
| "action_name": "clbk1", | ||
| "callback_name": "test", | ||
| "bot": '66cd84e4f206edf5b776d6d8', | ||
| "sender_id": "[email protected]", | ||
| "channel": "unsupported (None)", | ||
| "metadata": { | ||
| "first_name": "rajan", | ||
| "bot": '66cd84e4f206edf5b776d6d8' | ||
| }, | ||
| "identifier": "01928eb52813799e81c56803ecf39e6e", | ||
| "callback_url": "http://localhost:5059/callback/d/01928eb52813799e81c56803ecf39e6e/98bxWAMY9HF40La5AKQjEb-0JqaDTKX_Mmmmmmmmmmm", | ||
| "execution_mode": "async", | ||
| "state": 0, | ||
| "is_valid": True, | ||
| "bot_response": "test - this is from callback test" | ||
| } | ||
| } | ||
| } | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring the get_executor_logs fixture for better maintainability.
The get_executor_logs fixture is quite long and complex. Consider breaking it down into smaller, more focused fixtures or helper methods. This would improve readability and make it easier to maintain and update individual test scenarios.
Would you like assistance in refactoring this fixture into smaller, more manageable parts?
| data=[ | ||
| { | ||
| "name": "SOURCE_CODE", | ||
| "value": "import requests\n\nurl=\"https://waba-v2.360dialog.io/messages\"\n\nheaders={'Content-Type': 'application/json', 'D360-API-KEY' : 'mks3hj3489348938493849839R3RAK'}\n\ncontacts = ['919657055022','918210057905']\ncontacts = [\"919515991685\"]\nbody = {\n \"messaging_product\": \"whatsapp\",\n \"recipient_type\": \"individual\",\n \"to\": \"9199991685\",\n \"type\": \"template\",\n \"template\": {\n \"namespace\": \"54500467_f322_4595_becd_41555889bfd8\",\n \"language\": {\n \"policy\": \"deterministic\",\n \"code\": \"en\"\n },\n \"name\": \"schedule_action_test\"\n }\n}\n\nfor contact in contacts:\n body[\"to\"] = contact\n resp = requests.post(url, headers=headers, data=json.dumps(body))\n resp = resp.json()\n print(resp[\"messages\"])\n\nbot_response = 'this from callback pyscript'" |
There was a problem hiding this comment.
Remove or mask sensitive information in test data.
The test data contains what appears to be real email addresses, API keys, and URLs with tokens. It's best practice to use dummy data in tests to avoid exposing sensitive information. Consider replacing these with placeholder values.
For example:
-"myuser": "[email protected]",
+"myuser": "[email protected]",
-'D360-API-KEY' : 'mks3hj3489348938493849839R3RAK'
+'D360-API-KEY' : 'dummy_api_key'
-'callback_url': 'http://localhost:5059/callback/d/01928eb52813799e81c56803ecf39e6e/98bxWAMY9HF40La5AKQjEb-0JqaDTKX_Mmmmmmmmmmm'
+'callback_url': 'http://localhost:5059/callback/d/<identifier>/<token>'Also applies to: 90-90, 209-209, 216-216, 260-260, 295-295, 317-317
Added and fixed test cases related to the same.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
tests/integration_test/services_test.py (1)
2569-2687: Suggestion: Improve test readability with helper functionsTo improve the readability and maintainability of the test case, consider extracting the assertion logic into helper functions. This will make the test more concise and easier to understand.
Here's an example of how you could refactor the test:
def assert_executor_logs_response(response, expected_count, expected_task_type, expected_event_class): actual = response.json() assert actual["error_code"] == 0 assert not actual["message"] assert actual["success"] assert len(actual["data"]["logs"]) == actual["data"]["total"] == expected_count assert actual["data"]["logs"][0]["task_type"] == expected_task_type assert actual["data"]["logs"][0]["event_class"] == expected_event_class # Add more assertions as needed def test_get_executor_logs(get_executor_logs): response = client.get( url=f"/api/bot/{pytest.bot}/executor/logs?task_type=Callback", headers={"Authorization": pytest.token_type + " " + pytest.access_token}, ) assert_executor_logs_response(response, 1, "Callback", "pyscript_evaluator") response = client.get( url=f"/api/bot/{pytest.bot}/executor/logs?task_type=Event", headers={"Authorization": pytest.token_type + " " + pytest.access_token}, ) assert_executor_logs_response(response, 2, "Event", "model_testing") response = client.get( url=f"/api/bot/{pytest.bot}/executor/logs?task_type=Event&event_class=model_training", headers={"Authorization": pytest.token_type + " " + pytest.access_token}, ) assert_executor_logs_response(response, 1, "Event", "model_training")This refactoring will make the test more concise and easier to maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- tests/integration_test/services_test.py (2 hunks)
- tests/unit_test/data_processor/executor_processor_test.py (1 hunks)
🔥 Files not summarized due to errors (1)
- tests/integration_test/services_test.py: Error: Server error: no LLM provider could handle the message
🧰 Additional context used
🪛 Gitleaks
tests/integration_test/services_test.py
2373-2373: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 Ruff
tests/unit_test/data_processor/executor_processor_test.py
12-12: Use capitalized environment variable
SYSTEM_FILEinstead ofsystem_fileReplace
system_filewithSYSTEM_FILE(SIM112)
🔇 Additional comments (3)
tests/unit_test/data_processor/executor_processor_test.py (1)
17-22: LGTM: TestExecutorProcessor class and init_connection fixture.The TestExecutorProcessor class declaration and the init_connection fixture are well-structured. The fixture's autouse and class scope ensure that the database connection is set up once for all tests in the class, which is efficient.
tests/integration_test/services_test.py (2)
60-60: LGTM: Import statement updated correctlyThe addition of
TASK_TYPEto the import list is appropriate, as it will likely be used in the new test cases for bot-specific executor logs.
2357-2687: LGTM: Comprehensive test coverage for executor logsThe new fixture
get_executor_logsand test casetest_get_executor_logsprovide good coverage for the bot-specific executor logs functionality. The test case verifies log retrieval for different task types and event classes, ensuring proper filtering and structure of the retrieved logs.🧰 Tools
🪛 Gitleaks
2373-2373: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
| def test_get_row_count(self): | ||
| processor = ExecutorProcessor() | ||
| count = processor.get_row_count("66cd84e4f206edf5b776d6d8") | ||
| assert count == 6 | ||
|
|
||
| count = processor.get_row_count("66cd84e4f206edf5b776d6d8", event_class="pyscript_evaluator") | ||
| assert count == 2 | ||
|
|
||
| count = processor.get_row_count("66cd84e4f206edf5b776d6d8", event_class="pyscript_evaluator", | ||
| task_type="Callback") | ||
| assert count == 1 | ||
|
|
||
| count = processor.get_row_count("66cd84e4f206edf5b776d6d8", task_type="Event") | ||
| assert count == 2 | ||
|
|
||
| count = processor.get_row_count("66cd84e4f206edf5b776d6d8", event_class="model_testing", | ||
| task_type="Event") | ||
| assert count == 1 | ||
|
|
||
| count = processor.get_row_count("66cd84e4f206edf5b776d6d8", task_type="Action") | ||
| assert count == 3 | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider parameterizing the test_get_row_count method.
Similar to the previous suggestion, the test_get_row_count method could benefit from parameterization. This would make the test more concise and easier to extend with new test cases.
Here's an example of how you could parameterize this test:
@pytest.mark.parametrize("event_class, task_type, expected_count", [
(None, None, 6),
("pyscript_evaluator", None, 2),
("pyscript_evaluator", "Callback", 1),
(None, "Event", 2),
("model_testing", "Event", 1),
(None, "Action", 3),
])
def test_get_row_count(self, event_class, task_type, expected_count):
processor = ExecutorProcessor()
count = processor.get_row_count("66cd84e4f206edf5b776d6d8", event_class=event_class, task_type=task_type)
assert count == expected_countThis approach would make the test more maintainable and easier to read.
| import os | ||
|
|
||
| import pytest | ||
| from bson import ObjectId | ||
| from mongoengine import connect | ||
|
|
||
| from kairon.shared.constants import EventClass | ||
| from kairon.shared.data.constant import EVENT_STATUS, TASK_TYPE | ||
| from kairon.shared.events.processor import ExecutorProcessor | ||
| from kairon.shared.utils import Utility | ||
|
|
||
| os.environ["system_file"] = "./tests/testing_data/system.yaml" | ||
| Utility.load_environment() | ||
| Utility.load_system_metadata() | ||
|
|
There was a problem hiding this comment.
Use uppercase for environment variable names.
Python's convention for environment variables is to use uppercase letters. Please change "system_file" to "SYSTEM_FILE" to adhere to this standard.
Apply this diff to correct the environment variable name:
-os.environ["system_file"] = "./tests/testing_data/system.yaml"
+os.environ["SYSTEM_FILE"] = "./tests/testing_data/system.yaml"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import os | |
| import pytest | |
| from bson import ObjectId | |
| from mongoengine import connect | |
| from kairon.shared.constants import EventClass | |
| from kairon.shared.data.constant import EVENT_STATUS, TASK_TYPE | |
| from kairon.shared.events.processor import ExecutorProcessor | |
| from kairon.shared.utils import Utility | |
| os.environ["system_file"] = "./tests/testing_data/system.yaml" | |
| Utility.load_environment() | |
| Utility.load_system_metadata() | |
| import os | |
| import pytest | |
| from bson import ObjectId | |
| from mongoengine import connect | |
| from kairon.shared.constants import EventClass | |
| from kairon.shared.data.constant import EVENT_STATUS, TASK_TYPE | |
| from kairon.shared.events.processor import ExecutorProcessor | |
| from kairon.shared.utils import Utility | |
| os.environ["SYSTEM_FILE"] = "./tests/testing_data/system.yaml" | |
| Utility.load_environment() | |
| Utility.load_system_metadata() |
🧰 Tools
🪛 Ruff
12-12: Use capitalized environment variable
SYSTEM_FILEinstead ofsystem_fileReplace
system_filewithSYSTEM_FILE(SIM112)
| data=[ | ||
| { | ||
| "name": "SOURCE_CODE", | ||
| "value": "import requests\n\nurl=\"https://waba-v2.360dialog.io/messages\"\n\nheaders={'Content-Type': 'application/json', 'D360-API-KEY' : '6aYs394839849384rkU6Bs39R3RAK'}\n\ncontacts = ['919657099999','918210099999']\ncontacts = [\"919515999999\"]\nbody = {\n \"messaging_product\": \"whatsapp\",\n \"recipient_type\": \"individual\",\n \"to\": \"9199991685\",\n \"type\": \"template\",\n \"template\": {\n \"namespace\": \"54500467_f322_4595_becd_41555889bfd8\",\n \"language\": {\n \"policy\": \"deterministic\",\n \"code\": \"en\"\n },\n \"name\": \"schedule_action_test\"\n }\n}\n\nfor contact in contacts:\n body[\"to\"] = contact\n resp = requests.post(url, headers=headers, data=json.dumps(body))\n resp = resp.json()\n print(resp[\"messages\"])\n\nbot_response = 'this from callback pyscript'" |
There was a problem hiding this comment.
Security Issue: Remove hardcoded API key
The source code contains a hardcoded API key, which is a security risk. This key should be removed and replaced with a placeholder or retrieved from a secure configuration.
Replace the hardcoded API key with a placeholder:
- "value": "import requests\n\nurl=\"https://waba-v2.360dialog.io/messages\"\n\nheaders={'Content-Type': 'application/json', 'D360-API-KEY' : '6aYs394839849384rkU6Bs39R3RAK'}\n\ncontacts = ['919657099999','918210099999']\ncontacts = [\"919515999999\"]\nbody = {\n \"messaging_product\": \"whatsapp\",\n \"recipient_type\": \"individual\",\n \"to\": \"9199991685\",\n \"type\": \"template\",\n \"template\": {\n \"namespace\": \"54500467_f322_4595_becd_41555889bfd8\",\n \"language\": {\n \"policy\": \"deterministic\",\n \"code\": \"en\"\n },\n \"name\": \"schedule_action_test\"\n }\n}\n\nfor contact in contacts:\n body[\"to\"] = contact\n resp = requests.post(url, headers=headers, data=json.dumps(body))\n resp = resp.json()\n print(resp[\"messages\"])\n\nbot_response = 'this from callback pyscript'"
+ "value": "import requests\n\nurl=\"https://waba-v2.360dialog.io/messages\"\n\nheaders={'Content-Type': 'application/json', 'D360-API-KEY' : '<API_KEY_PLACEHOLDER>'}\n\ncontacts = ['919657099999','918210099999']\ncontacts = [\"919515999999\"]\nbody = {\n \"messaging_product\": \"whatsapp\",\n \"recipient_type\": \"individual\",\n \"to\": \"9199991685\",\n \"type\": \"template\",\n \"template\": {\n \"namespace\": \"54500467_f322_4595_becd_41555889bfd8\",\n \"language\": {\n \"policy\": \"deterministic\",\n \"code\": \"en\"\n },\n \"name\": \"schedule_action_test\"\n }\n}\n\nfor contact in contacts:\n body[\"to\"] = contact\n resp = requests.post(url, headers=headers, data=json.dumps(body))\n resp = resp.json()\n print(resp[\"messages\"])\n\nbot_response = 'this from callback pyscript'"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "value": "import requests\n\nurl=\"https://waba-v2.360dialog.io/messages\"\n\nheaders={'Content-Type': 'application/json', 'D360-API-KEY' : '6aYs394839849384rkU6Bs39R3RAK'}\n\ncontacts = ['919657099999','918210099999']\ncontacts = [\"919515999999\"]\nbody = {\n \"messaging_product\": \"whatsapp\",\n \"recipient_type\": \"individual\",\n \"to\": \"9199991685\",\n \"type\": \"template\",\n \"template\": {\n \"namespace\": \"54500467_f322_4595_becd_41555889bfd8\",\n \"language\": {\n \"policy\": \"deterministic\",\n \"code\": \"en\"\n },\n \"name\": \"schedule_action_test\"\n }\n}\n\nfor contact in contacts:\n body[\"to\"] = contact\n resp = requests.post(url, headers=headers, data=json.dumps(body))\n resp = resp.json()\n print(resp[\"messages\"])\n\nbot_response = 'this from callback pyscript'" | |
| "value": "import requests\n\nurl=\"https://waba-v2.360dialog.io/messages\"\n\nheaders={'Content-Type': 'application/json', 'D360-API-KEY' : '<API_KEY_PLACEHOLDER>'}\n\ncontacts = ['919657099999','918210099999']\ncontacts = [\"919515999999\"]\nbody = {\n \"messaging_product\": \"whatsapp\",\n \"recipient_type\": \"individual\",\n \"to\": \"9199991685\",\n \"type\": \"template\",\n \"template\": {\n \"namespace\": \"54500467_f322_4595_becd_41555889bfd8\",\n \"language\": {\n \"policy\": \"deterministic\",\n \"code\": \"en\"\n },\n \"name\": \"schedule_action_test\"\n }\n}\n\nfor contact in contacts:\n body[\"to\"] = contact\n resp = requests.post(url, headers=headers, data=json.dumps(body))\n resp = resp.json()\n print(resp[\"messages\"])\n\nbot_response = 'this from callback pyscript'" |
🧰 Tools
🪛 Gitleaks
2373-2373: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- tests/unit_test/data_processor/executor_processor_test.py (1 hunks)
🧰 Additional context used
🪛 Ruff
tests/unit_test/data_processor/executor_processor_test.py
12-12: Use capitalized environment variable
SYSTEM_FILEinstead ofsystem_fileReplace
system_filewithSYSTEM_FILE(SIM112)
🔇 Additional comments (2)
tests/unit_test/data_processor/executor_processor_test.py (2)
379-392: LGTM! Well-structured parameterized test.The
test_get_row_countmethod is well-implemented using pytest's parameterize decorator. It covers multiple scenarios for theget_row_countmethod ofExecutorProcessor, which is a good practice for thorough testing.
12-12:⚠️ Potential issueUse uppercase for environment variable name.
Python's convention for environment variables is to use uppercase letters. Please change
"system_file"to"SYSTEM_FILE"to adhere to this standard.Apply this diff to correct the environment variable name:
-os.environ["system_file"] = "./tests/testing_data/system.yaml" +os.environ["SYSTEM_FILE"] = "./tests/testing_data/system.yaml"Likely invalid or redundant comment.
🧰 Tools
🪛 Ruff
12-12: Use capitalized environment variable
SYSTEM_FILEinstead ofsystem_fileReplace
system_filewithSYSTEM_FILE(SIM112)
| logs = list(processor.get_executor_logs("66cd84e4f206edf5b776d6d8", task_type="Callback")) | ||
| assert len(logs) == 1 | ||
| assert logs[0]["task_type"] == "Callback" | ||
| assert logs[0]["event_class"] == "pyscript_evaluator" | ||
| assert logs[0]["status"] == "Completed" | ||
| assert logs[0]["data"] == { | ||
| 'source_code': 'bot_response = "test - this is from callback test"', | ||
| 'predefined_objects': { | ||
| 'req': {'type': 'GET', 'body': None, 'params': {}}, | ||
| 'req_host': '127.0.0.1', 'action_name': 'clbk1', | ||
| 'callback_name': 'test', 'bot': '66cd84e4f206edf5b776d6d8', | ||
| 'sender_id': '[email protected]', | ||
| 'channel': 'unsupported (None)', | ||
| 'metadata': {'first_name': 'rajan', | ||
| 'bot': '66cd84e4f206edf5b776d6d8'}, | ||
| 'identifier': '01928eb52813799e81c56803ecf39e6e', | ||
| 'callback_url': 'http://localhost:5059/callback/d/01928eb52813799e81c56803ecf39e6e/98bxWAMY9HF40La5AKQjEb-0JqaDTKX_Mmmmmmmmmmm', | ||
| 'execution_mode': 'async', 'state': 0, 'is_valid': True} | ||
| } | ||
| assert logs[0]["response"] == { | ||
| 'ResponseMetadata': {'RequestId': '0da1cdd1-1702-473b-8109-67a39f990835', 'HTTPStatusCode': 200, | ||
| 'HTTPHeaders': {'date': 'Tue, 15 Oct 2024 07:38:01 GMT', | ||
| 'content-type': 'application/json', 'content-length': '740', | ||
| 'connection': 'keep-alive', | ||
| 'x-amzn-requestid': '0da1cdd1-1702-473b-8109-67a39f9sdlksldksl', | ||
| 'x-amzn-remapped-content-length': '0', | ||
| 'x-amz-executed-version': '$LATEST', | ||
| 'x-amz-log-result': 'sdskjdksjdkjskdjskjdksj', | ||
| 'x-amzn-trace-id': 'Root=1-670e1bd6-25e7667c7c6ce37f09a9afc7;Sampled=1;Lineage=1:d072fc6c:0'}, | ||
| 'RetryAttempts': 0}, 'StatusCode': 200, 'LogResult': 'sldklskdlskdlskdlk', | ||
| 'ExecutedVersion': '$LATEST', | ||
| 'Payload': {'statusCode': 200, 'statusDescription': '200 OK', 'isBase64Encoded': False, | ||
| 'headers': {'Content-Type': 'text/html; charset=utf-8'}, | ||
| 'body': {'req': {'type': 'GET', 'body': None, 'params': {}}, 'req_host': '127.0.0.1', | ||
| 'action_name': 'clbk1', 'callback_name': 'test', 'bot': '66cd84e4f206edf5b776d6d8', | ||
| 'sender_id': '[email protected]', 'channel': 'unsupported (None)', | ||
| 'metadata': {'first_name': 'rajan', 'bot': '66cd84e4f206edf5b776d6d8'}, | ||
| 'identifier': '01928eb52813799e81c56803ecf39e6e', | ||
| 'callback_url': 'http://localhost:5059/callback/d/01928eb52813799e81c56803ecf39e6e/98bxWAMY9HF40La5AKQjEb-0JqaDTKX_Mmmmmmmmmmm', | ||
| 'execution_mode': 'async', 'state': 0, 'is_valid': True, | ||
| 'bot_response': 'test - this is from callback test'}}} | ||
| assert logs[0]["executor_log_id"] | ||
| assert logs[0]['bot'] == "66cd84e4f206edf5b776d6d8" | ||
|
|
||
| logs = list(processor.get_executor_logs("66cd84e4f206edf5b776d6d8", task_type="Event")) | ||
| assert len(logs) == 2 | ||
| assert logs[0]["task_type"] == "Event" | ||
| assert logs[0]["event_class"] == "model_testing" | ||
| assert logs[0]["status"] == "Initiated" | ||
| assert logs[0]["data"] == [ | ||
| { | ||
| 'name': 'BOT', | ||
| 'value': "66cd84e4f206edf5b776d6d8" | ||
| }, | ||
| { | ||
| 'name': 'USER', | ||
| 'value': 'test_user' | ||
| } | ||
| ] | ||
| assert logs[0]["executor_log_id"] | ||
| assert logs[0]['from_executor'] is True | ||
| assert logs[0]['bot'] == "66cd84e4f206edf5b776d6d8" | ||
|
|
||
| assert logs[1]["task_type"] == "Event" | ||
| assert logs[1]["event_class"] == "model_training" | ||
| assert logs[1]["status"] == "Initiated" | ||
| assert logs[1]["data"] == [ | ||
| { | ||
| 'name': 'BOT', | ||
| 'value': "66cd84e4f206edf5b776d6d8" | ||
| }, | ||
| { | ||
| 'name': 'USER', | ||
| 'value': 'test_user' | ||
| } | ||
| ] | ||
| assert logs[1]["executor_log_id"] | ||
| assert logs[1]['from_executor'] is True | ||
| assert logs[1]['bot'] == "66cd84e4f206edf5b776d6d8" | ||
|
|
||
| logs = list(processor.get_executor_logs("66cd84e4f206edf5b776d6d8", task_type="Event", | ||
| event_class="model_training")) | ||
| assert len(logs) == 1 | ||
| assert logs[0]["task_type"] == "Event" | ||
| assert logs[0]["event_class"] == "model_training" | ||
| assert logs[0]["status"] == "Initiated" | ||
| assert logs[0]["data"] == [ | ||
| { | ||
| 'name': 'BOT', | ||
| 'value': "66cd84e4f206edf5b776d6d8" | ||
| }, | ||
| { | ||
| 'name': 'USER', | ||
| 'value': 'test_user' | ||
| } | ||
| ] | ||
| assert logs[0]["executor_log_id"] | ||
| assert logs[0]['from_executor'] is True | ||
| assert logs[0]['bot'] == "66cd84e4f206edf5b776d6d8" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider parameterizing the test_get_executor_logs method.
The test_get_executor_logs method tests multiple scenarios with similar assertion structures. To improve maintainability and reduce code duplication, consider using pytest's parameterize decorator to run the same test logic with different inputs.
Here's an example of how you could parameterize this test:
@pytest.mark.parametrize("task_type, event_class, expected_count, expected_event_class", [
("Callback", None, 1, "pyscript_evaluator"),
("Event", None, 2, "model_testing"),
("Event", "model_training", 1, "model_training"),
])
def test_get_executor_logs(self, get_executor_logs, task_type, event_class, expected_count, expected_event_class):
processor = ExecutorProcessor()
logs = list(processor.get_executor_logs("66cd84e4f206edf5b776d6d8", task_type=task_type, event_class=event_class))
assert len(logs) == expected_count
assert logs[0]["task_type"] == task_type
assert logs[0]["event_class"] == expected_event_class
assert logs[0]["status"] in ["Initiated", "Completed"]
assert logs[0]["executor_log_id"]
assert logs[0]['bot'] == "66cd84e4f206edf5b776d6d8"
# Add more specific assertions for each case if neededThis approach would make it easier to add new test cases in the future and improve the overall readability of the test.
| "displayLabel": "", | ||
| "telemetry-uid": "None", | ||
| "telemetry-sid": "None", | ||
| "is_integration_user": False, | ||
| "bot": '66cd84e4f206edf5b776d6d8', | ||
| "account": 8, | ||
| "channel_type": "chat_client" | ||
| }, | ||
| "intent_ranking": [ | ||
| { | ||
| "name": "pyscript", | ||
| "confidence": 1 | ||
| } | ||
| ] | ||
| }, | ||
| "kairon_user_msg": None, | ||
| "session_started": None | ||
| } | ||
| }, | ||
| executor_log_id=ObjectId().__str__(), status=EVENT_STATUS.INITIATED.value) | ||
| executor.log_task(event_class=EventClass.pyscript_evaluator.value, task_type=TASK_TYPE.CALLBACK.value, | ||
| data={ | ||
| "source_code": "bot_response = \"test - this is from callback test\"", | ||
| "predefined_objects": { | ||
| "req": { | ||
| "type": "GET", | ||
| "body": None, | ||
| "params": {} | ||
| }, | ||
| "req_host": "127.0.0.1", | ||
| "action_name": "clbk1", | ||
| "callback_name": "test", | ||
| "bot": '66cd84e4f206edf5b776d6d8', | ||
| "sender_id": "[email protected]", | ||
| "channel": "unsupported (None)", | ||
| "metadata": { | ||
| "first_name": "rajan", | ||
| "bot": '66cd84e4f206edf5b776d6d8' | ||
| }, | ||
| "identifier": "01928eb52813799e81c56803ecf39e6e", | ||
| "callback_url": "http://localhost:5059/callback/d/01928eb52813799e81c56803ecf39e6e/98bxWAMY9HF40La5AKQjEb-0JqaDTKX_Mmmmmmmmmmm", | ||
| "execution_mode": "async", | ||
| "state": 0, | ||
| "is_valid": True | ||
| } | ||
| }, | ||
| executor_log_id=ObjectId().__str__(), status=EVENT_STATUS.COMPLETED.value, | ||
| response={ | ||
| "ResponseMetadata": { | ||
| "RequestId": "0da1cdd1-1702-473b-8109-67a39f990835", | ||
| "HTTPStatusCode": 200, | ||
| "HTTPHeaders": { | ||
| "date": "Tue, 15 Oct 2024 07:38:01 GMT", | ||
| "content-type": "application/json", | ||
| "content-length": "740", | ||
| "connection": "keep-alive", | ||
| "x-amzn-requestid": "0da1cdd1-1702-473b-8109-67a39f9sdlksldksl", | ||
| "x-amzn-remapped-content-length": "0", | ||
| "x-amz-executed-version": "$LATEST", | ||
| "x-amz-log-result": "sdskjdksjdkjskdjskjdksj", | ||
| "x-amzn-trace-id": "Root=1-670e1bd6-25e7667c7c6ce37f09a9afc7;Sampled=1;Lineage=1:d072fc6c:0" | ||
| }, | ||
| "RetryAttempts": 0 | ||
| }, | ||
| "StatusCode": 200, | ||
| "LogResult": "sldklskdlskdlskdlk", | ||
| "ExecutedVersion": "$LATEST", | ||
| "Payload": { | ||
| "statusCode": 200, | ||
| "statusDescription": "200 OK", | ||
| "isBase64Encoded": False, | ||
| "headers": { | ||
| "Content-Type": "text/html; charset=utf-8" | ||
| }, | ||
| "body": { | ||
| "req": { | ||
| "type": "GET", | ||
| "body": None, | ||
| "params": {} | ||
| }, | ||
| "req_host": "127.0.0.1", | ||
| "action_name": "clbk1", | ||
| "callback_name": "test", | ||
| "bot": '66cd84e4f206edf5b776d6d8', | ||
| "sender_id": "[email protected]", | ||
| "channel": "unsupported (None)", | ||
| "metadata": { | ||
| "first_name": "rajan", | ||
| "bot": '66cd84e4f206edf5b776d6d8' | ||
| }, | ||
| "identifier": "01928eb52813799e81c56803ecf39e6e", | ||
| "callback_url": "http://localhost:5059/callback/d/01928eb52813799e81c56803ecf39e6e/98bxWAMY9HF40La5AKQjEb-0JqaDTKX_Mmmmmmmmmmm", | ||
| "execution_mode": "async", | ||
| "state": 0, | ||
| "is_valid": True, | ||
| "bot_response": "test - this is from callback test" | ||
| } | ||
| } | ||
| } | ||
| ) |
There was a problem hiding this comment.
Replace sensitive information with dummy data in test fixtures.
The test fixture contains what appears to be real email addresses, API keys, and URLs with tokens. It's best practice to use dummy data in tests to avoid exposing sensitive information.
Consider replacing sensitive data with placeholder values. For example:
-"myuser": "[email protected]",
+"myuser": "[email protected]",
-'D360-API-KEY' : 'mks3hj3489348938493849839R3RAK'
+'D360-API-KEY' : 'dummy_api_key'
-'callback_url': 'http://localhost:5059/callback/d/01928eb52813799e81c56803ecf39e6e/98bxWAMY9HF40La5AKQjEb-0JqaDTKX_Mmmmmmmmmmm'
+'callback_url': 'http://localhost:5059/callback/d/<identifier>/<token>'Make sure to replace all instances of sensitive data throughout the fixture.
Committable suggestion was skipped due to low confidence.
* Added code related to bot specific executor logs. Added and fixed test cases related to the same. * Added code related to bot specific executor logs. Added and fixed test cases related to the same. * Added code related to bot specific executor logs. Added and fixed test cases related to the same. * changed tests according to code rabbit suggestions.
Summary by CodeRabbit
Release Notes
New Features
Improvements
botparameter, improving interaction with bot contexts.Tests
ExecutorProcessorclass and updated existing tests for action utilities to validate new parameters.