Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: add more tests in the agent #1572

Merged
merged 6 commits into from
Jan 31, 2025
Merged

test: add more tests in the agent #1572

merged 6 commits into from
Jan 31, 2025

Conversation

gventuri
Copy link
Collaborator

@gventuri gventuri commented Jan 31, 2025

Important

Add comprehensive unit tests for memory, agent, dataframe, logger, and session components in the pandasai library to ensure correct behavior and error handling.

  • Memory Tests:
    • Add test_to_json_empty_memory, test_to_json_with_messages, test_to_json_message_order in test_memory.py to verify JSON conversion.
    • Add test_to_openai_messages_empty, test_to_openai_messages_with_agent_description, test_to_openai_messages_without_agent_description to test OpenAI message formatting.
  • Agent Tests:
    • Add tests in test_agent.py for _process_query, _regenerate_code_after_error, _handle_exception to verify query processing, error handling, and exception logging.
    • Add tests for execute_with_retries, execute_code, generate_code to ensure correct execution and caching behavior.
  • DataFrame Tests:
    • Add test_pull_success, test_pull_missing_api_key, test_pull_api_error in test_pull.py to verify dataset pulling and error handling.
  • Logger Tests:
    • Add test_verbose_setter, test_save_logs_property in test_logger.py to verify logging behavior based on verbosity and save settings.
  • Session Tests:
    • Add test_session_init_without_api_key, test_make_request_success, test_make_request_error_response in test_session.py to verify session initialization and request handling.

This description was created by Ellipsis for 27bab00. It will automatically update as commits are pushed.

Copy link

codecov bot commented Jan 31, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.47%. Comparing base (4ca228f) to head (27bab00).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1572      +/-   ##
==========================================
+ Coverage   82.73%   86.47%   +3.73%     
==========================================
  Files          66       66              
  Lines        2410     2410              
==========================================
+ Hits         1994     2084      +90     
+ Misses        416      326      -90     
Flag Coverage Δ
unittests 86.47% <ø> (+3.73%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Reviewed everything up to e128221 in 1 minute and 14 seconds

More details
  • Looked at 132 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 7 drafted comments based on config settings.
1. tests/unit_tests/agent/test_agent.py:491
  • Draft comment:
    Consider verifying the cache key used in agent._state.cache.set to ensure the cache is set correctly. This is important for cache retrieval consistency.
  • Reason this comment was not posted:
    Comment did not seem useful.
2. tests/unit_tests/agent/test_agent.py:508
  • Draft comment:
    Consider verifying the specific error message in the CodeExecutionError to ensure the error handling logic is correctly identifying and handling the specific error.
  • Reason this comment was not posted:
    Comment did not seem useful.
3. tests/unit_tests/agent/test_agent.py:513
  • Draft comment:
    The import statement for InvalidLLMOutputType is redundant and can be removed since it's already imported at the top of the file.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The import statement for InvalidLLMOutputType is redundant since it's already imported at the top of the file. This can be removed to clean up the code.
4. tests/unit_tests/agent/test_agent.py:471
  • Draft comment:
    Function and method names should follow consistent patterns. Ensure all test method names use underscores consistently, e.g., test_generate_code_with_cache_hit and test_generate_code_with_cache_miss.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The test method names are inconsistent. Some use underscores while others do not. Consistent naming patterns should be followed for better readability and maintenance.
5. tests/unit_tests/agent/test_agent.py:486
  • Draft comment:
    Assertions should have descriptive error messages. Consider adding an error message to this assertion for clarity.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The assertion lacks a descriptive error message, which is important for debugging.
6. tests/unit_tests/agent/test_agent.py:508
  • Draft comment:
    Assertions should have descriptive error messages. Consider adding an error message to this assertion for clarity.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The assertion lacks a descriptive error message, which is important for debugging.
7. tests/unit_tests/agent/test_agent.py:566
  • Draft comment:
    Assertions should have descriptive error messages. Consider adding an error message to this assertion for clarity.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The assertion lacks a descriptive error message, which is important for debugging.

Workflow ID: wflow_nYjQBlg5NUBLrI73


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Incremental review on a0289ee in 20 seconds

More details
  • Looked at 125 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. tests/unit_tests/dataframe/test_pull.py:43
  • Draft comment:
    Consider using more descriptive test function names to follow a consistent naming convention, such as test_pull_with_successful_response. This applies to other test functions as well.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The test function names are not following a consistent naming convention. They should be more descriptive and follow a pattern like test_functionality_condition.
2. tests/unit_tests/dataframe/test_pull.py:75
  • Draft comment:
    Consider using more descriptive test function names to follow a consistent naming convention, such as test_pull_without_api_key. This applies to other test functions as well.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The test function names are not following a consistent naming convention. They should be more descriptive and follow a pattern like test_functionality_condition.
3. tests/unit_tests/dataframe/test_pull.py:83
  • Draft comment:
    Consider using more descriptive test function names to follow a consistent naming convention, such as test_pull_with_api_error. This applies to other test functions as well.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The test function names are not following a consistent naming convention. They should be more descriptive and follow a pattern like test_functionality_condition.
4. tests/unit_tests/dataframe/test_pull.py:94
  • Draft comment:
    Consider using more descriptive test function names to follow a consistent naming convention, such as test_pull_when_file_exists. This applies to other test functions as well.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The test function names are not following a consistent naming convention. They should be more descriptive and follow a pattern like test_functionality_condition.
5. tests/unit_tests/dataframe/test_pull.py:90
  • Draft comment:
    The error message in the assertion should be more descriptive. Consider changing it to:
        with pytest.raises(DatasetNotFound, match="The remote dataset could not be found for pulling!"):
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The test file is well-structured and covers various scenarios for the pull method. However, there is a minor grammar issue in the error message assertion.

Workflow ID: wflow_hhZWolMliVyF80VP


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Incremental review on 198b50f in 28 seconds

More details
  • Looked at 291 lines of code in 4 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. tests/unit_tests/helpers/test_logger.py:62
  • Draft comment:
    The function test_save_logs_property is defined twice. Consider renaming or removing the duplicate function to avoid conflicts.
  • Reason this comment was not posted:
    Comment was on unchanged code.
2. tests/unit_tests/helpers/test_logger.py:62
  • Draft comment:
    Duplicate function name test_save_logs_property. Consider renaming one of them to avoid conflicts.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_IFeDZ1rSx4bj9c9g


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@gventuri gventuri force-pushed the test/increase-coverage branch from 198b50f to 27bab00 Compare January 31, 2025 16:26
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Incremental review on 27bab00 in 1 minute and 2 seconds

More details
  • Looked at 321 lines of code in 4 files
  • Skipped 0 files when reviewing.
  • Skipped posting 10 drafted comments based on config settings.
1. tests/unit_tests/helpers/test_logger.py:63
  • Draft comment:
    The function test_save_logs_property is defined twice. Consider renaming or removing the duplicate function to avoid confusion.
  • Reason this comment was not posted:
    Marked as duplicate.
2. tests/unit_tests/dataframe/test_pull.py:1
  • Draft comment:
    Organize imports by grouping standard library imports, third-party imports, and local imports separately for better readability.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The import statements in tests/unit_tests/dataframe/test_pull.py are not organized. It's a good practice to group imports into standard library imports, third-party imports, and local imports, each separated by a blank line.
3. tests/unit_tests/helpers/test_session.py:148
  • Draft comment:
    The environment variable PANDABI_API_KEY seems inconsistent with PANDASAI_API_KEY used elsewhere. Verify if this is intentional or a typo.
  • Reason this comment was not posted:
    Comment was not on a valid diff hunk.
4. tests/unit_tests/helpers/test_logger.py:63
  • Draft comment:
    The function test_save_logs_property is defined twice. Consider renaming one of them to avoid confusion and adhere to DRY principles.
  • Reason this comment was not posted:
    Comment was on unchanged code.
5. tests/test_memory.py:23
  • Draft comment:
    Add an error message to the assertion for better clarity when the test fails.
    assert memory.to_json() == expected_json, "Expected JSON with messages does not match"
  • Reason this comment was not posted:
    Comment was on unchanged code.
6. tests/test_memory.py:38
  • Draft comment:
    Add an error message to the assertion for better clarity when the test fails.
    assert len(result) == 3, "Expected 3 messages in JSON"
  • Reason this comment was not posted:
    Marked as duplicate.
7. tests/test_memory.py:60
  • Draft comment:
    Add an error message to the assertion for better clarity when the test fails.
    assert memory.to_openai_messages() == expected_messages, "Expected OpenAI messages with agent description do not match"
  • Reason this comment was not posted:
    Marked as duplicate.
8. tests/test_memory.py:75
  • Draft comment:
    Add an error message to the assertion for better clarity when the test fails.
    assert memory.to_openai_messages() == expected_messages, "Expected OpenAI messages without agent description do not match"
  • Reason this comment was not posted:
    Marked as duplicate.
9. tests/unit_tests/dataframe/test_pull.py:7
  • Draft comment:
    Move the pytest import to be grouped with other third-party imports for consistency.
import pytest
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The import order should be consistent and follow PEP 8 guidelines. The pytest import should be grouped with other third-party imports.
10. tests/unit_tests/dataframe/test_pull.py:15
  • Draft comment:
    Move the pytest import to be grouped with other third-party imports for consistency.
import pytest
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The import order should be consistent and follow PEP 8 guidelines. The pytest import should be grouped with other third-party imports.

Workflow ID: wflow_Dt8AKQXgUpWm3kjE


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@gventuri gventuri merged commit d2350a1 into main Jan 31, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant