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

fix(Agent): adding test to improve coverage #1528

Merged
merged 4 commits into from
Jan 17, 2025

Conversation

scaliseraoul
Copy link

@scaliseraoul scaliseraoul commented Jan 17, 2025


Important

Add tests for SQL query execution in Agent and refactor schema validation and SQL execution methods.

  • Tests:
    • Add tests for _execute_local_sql_query and _execute_sql_query in test_agent.py to verify SQL execution success and failure cases.
    • Add tests for Session initialization and API key handling in test_session.py.
    • Add tests for dataset loading and creation in test_pandasai_init.py.
  • Refactoring:
    • Move is_schema_source_same from schema_validator.py to semantic_layer_schema.py.
    • Remove execute_sql_query from DataFrame class in base.py.
  • Misc:
    • Fix type hint for dataframes parameter in chat() function in __init__.py.

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

Copy link

@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 0028798 in 2 minutes and 10 seconds

More details
  • Looked at 308 lines of code in 9 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. pandasai/__init__.py:124
  • Draft comment:
    The change from List[DataFrame] to DataFrame in the chat function's signature is incorrect. The function is designed to accept multiple DataFrames, and the use of *dataframes indicates a variable number of arguments, which should be a list. The original type hint was correct.
def chat(query: str, *dataframes: List[DataFrame]):
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment is actually incorrect. When using *args, the type hint should describe the type of each individual argument, not the resulting collection. The change to just DataFrame is correct because it indicates that each variadic argument should be a DataFrame. The list conversion happens inside the function but doesn't affect the type signature.
    Could there be a reason why the original author used List[DataFrame]? Maybe there's some Python typing specification I'm not considering?
    No, PEP 484 clearly specifies that for *args, the type annotation describes the type of each argument, not the resulting tuple/list. The change is definitely correct.
    The comment should be deleted because it's incorrect. The new type hint of just DataFrame is the proper way to type hint variadic arguments.
2. pandasai/__init__.py:124
  • Draft comment:
    The type hint for *dataframes should be DataFrame instead of List[DataFrame] to correctly represent the variadic arguments.
  • Reason this comment was not posted:
    Marked as duplicate.
3. pandasai/agent/base.py:139
  • Draft comment:
            raise ValueError("No DataFrames are available for query execution.")
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The original message is grammatically correct and clear. The suggested change is also grammatically correct but doesn't meaningfully improve clarity or understanding. The change is purely stylistic and doesn't fix any actual issues.
    I might be undervaluing the importance of consistent error message style across the codebase. There could be a style guide that prefers this phrasing.
    Even if there is a style guide, this change is too minor to warrant a comment. Both versions are grammatically correct and clear.
    Delete this comment as it suggests a purely stylistic change that doesn't meaningfully improve the code or fix any actual issues.
4. pandasai/agent/state.py:12
  • Draft comment:
    Ensure that any documentation referencing is_schema_source_same is updated to reflect its new location in semantic_layer_schema.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The import statement for is_schema_source_same in pandasai/agent/state.py was updated to import from semantic_layer_schema. This change should be reflected in the documentation if it exists.

Workflow ID: wflow_EUEfNSRp5gHXqHCg


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

@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Jan 17, 2025
@gventuri gventuri merged commit c159671 into Sinaptik-AI:release/v3 Jan 17, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants