Skip to content

Tests: Refactor and Fix Test Suite #372

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

Closed
wants to merge 9 commits into from

Conversation

evanmschultz
Copy link
Contributor

@evanmschultz evanmschultz commented Apr 17, 2025

Summary:

This PR focuses on streamlining our test suite and ensuring better consistency across test directories.
Key Changes:

  • Moved the Anthropic integration test to the LLM client directory to align with the structure used in other tests.
  • Fixed malformed tests and resolved the issue with the mock LLM client naming.
  • Updated type hints and performed minor code cleaning to enhance code clarity and lint compliance.

Impact:

  • These changes aim to fix integration test failures and improve type hinting.
    Please review the changes, and let me know if any adjustments or further improvements are needed.

Important

Refactor and fix test suite by moving tests, fixing issues, and updating type hints for consistency and clarity.

  • Test Structure:
    • Move test_anthropic_client_int.py from tests/integrations to tests/llm_client for consistency.
  • Test Fixes:
    • Fix malformed tests in test_anthropic_client.py and test_client.py.
    • Resolve mock LLM client naming issues in test_anthropic_client.py.
  • Type Hints and Code Cleaning:
    • Add type hints to test_anthropic_client.py and test_client.py.
    • Minor code cleaning for lint compliance in anthropic_client.py and client.py.

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

@danielchalef
Copy link
Member

danielchalef commented Apr 17, 2025

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

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.

❌ Changes requested. Reviewed everything up to 40fc465 in 1 minute and 52 seconds

More details
  • Looked at 431 lines of code in 8 files
  • Skipped 0 files when reviewing.
  • Skipped posting 14 drafted comments based on config settings.
1. graphiti_core/llm_client/anthropic_client.py:222
  • Draft comment:
    Consider handling type conversion for content_item.input more explicitly to avoid using type: ignore. Verify that the input is always a dict or properly serialized.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 80%
    None
2. tests/llm_client/test_anthropic_client.py:41
  • Draft comment:
    Type annotations for fixtures and test functions improve clarity and consistency. Ensure similar patterns are followed in other test files.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 80%
    None
3. tests/llm_client/test_client.py:26
  • Draft comment:
    Renaming test class to 'MockLLMClient' avoids pytest collection issues. This is a good refactoring step.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 80%
    None
4. tests/test_graphiti_int.py:66
  • Draft comment:
    Ensure that test setup uses proper type: ignore hints where necessary. Variable naming (NEO4J_URI vs NEO4j_USER) is consistent with existing conventions.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 80%
    None
5. tests/utils/maintenance/test_temporal_operations_int.py:98
  • Draft comment:
    Destructuring of data from create_test_data() looks good. Ensure that changes for handling multiple return values are consistently applied.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 80%
    None
6. graphiti_core/llm_client/anthropic_client.py:223
  • Draft comment:
    Consider resolving the underlying type discrepancy instead of using a type: ignore comment if possible.
  • Reason this comment was not posted:
    Confidence changes required: 80% <= threshold 80%
    None
7. graphiti_core/llm_client/client.py:43
  • Draft comment:
    Use a tuple of exception classes for isinstance checks; e.g., (RateLimitError, json.decoder.JSONDecodeError) instead of using the union operator.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 80% vs. threshold = 80%
    Let me analyze this step by step. 1) The code change adds type hints and uses the | operator for union types. 2) The comment suggests using tuple syntax instead. 3) Both syntaxes are valid Python, but tuple syntax is more conventional for isinstance(). 4) This is a style improvement that makes the code more idiomatic. 5) The suggestion is clear and actionable.
    The union operator syntax is technically valid Python and works correctly. The suggestion is more about style/convention than correctness.
    While both syntaxes work, the tuple syntax is the more conventional and widely used pattern for isinstance() checks in Python, making the code more idiomatic and easier to read for most Python developers.
    Keep the comment as it suggests a valid improvement to make the code more idiomatic Python, and the suggestion is clear and actionable.
8. tests/llm_client/test_anthropic_client.py:41
  • Draft comment:
    Good use of type hints in fixtures and tests improves clarity; ensure this pattern is maintained across tests.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 80%
    None
9. tests/llm_client/test_client.py:27
  • Draft comment:
    Refactoring with a dedicated MockLLMClient improves test clarity. Ensure that using protected methods (with type: ignore) remains acceptable for testing purposes.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 80%
    None
10. tests/utils/maintenance/test_temporal_operations_int.py:98
  • Draft comment:
    Using '_' to ignore unused tuple items improves clarity in unpacking test data.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 80%
    None
11. graphiti_core/llm_client/client.py:43
  • Draft comment:
    In the function is_server_or_retry_error, the isinstance check on line 43 uses the expression (RateLimitError | json.decoder.JSONDecodeError). This use of the bitwise OR operator (|) is likely unintended; it doesn't create a tuple of exception types as required by isinstance. Please consider replacing it with a comma, for example: if isinstance(exception, (RateLimitError, json.decoder.JSONDecodeError)): to correctly specify multiple exception types.
  • Reason this comment was not posted:
    Marked as duplicate.
12. tests/llm_client/test_anthropic_client_int.py:17
  • Draft comment:
    The comment on line 17 still references the old file path (tests/integrations/test_anthropic_client_int.py). Please update it to the new location (tests/llm_client/test_anthropic_client_int.py) to avoid confusion.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
13. tests/test_graphiti_int.py:38
  • Draft comment:
    Typographical: The environment variable names for the Neo4j credentials are inconsistently cased (e.g., NEO4j_USER and NEO4j_PASSWORD versus NEO4J_URI). It would be clearer and more consistent to rename them to NEO4J_USER and NEO4J_PASSWORD.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
14. tests/utils/maintenance/test_temporal_operations_int.py:215
  • Draft comment:
    Typographical error: In the fact string for the new_edge in test_get_edge_contradictions_temporal_update, 'Bob no longer works at at Company XYZ' contains a duplicate 'at'. Please remove one instance to read 'Bob no longer works at Company XYZ'.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_Hi3JH8EXoKMHhCxL


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

def is_server_or_retry_error(exception):
if isinstance(exception, RateLimitError | json.decoder.JSONDecodeError):
def is_server_or_retry_error(exception: BaseException) -> bool:
if isinstance(exception, (RateLimitError | json.decoder.JSONDecodeError)):
Copy link
Contributor

Choose a reason for hiding this comment

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

Incorrect use of union operator in isinstance. Replace (RateLimitError | json.decoder.JSONDecodeError) with a tuple (RateLimitError, json.decoder.JSONDecodeError).

Suggested change
if isinstance(exception, (RateLimitError | json.decoder.JSONDecodeError)):
if isinstance(exception, (RateLimitError, json.decoder.JSONDecodeError)):

@evanmschultz
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

danielchalef added a commit that referenced this pull request Apr 17, 2025
@evanmschultz
Copy link
Contributor Author

evanmschultz commented Apr 17, 2025

Elipsis-Dev Pipe issue

When addressing the elipisis-dev comment about pipes in isinstance I get a ruff error:

Use `X | Y` in `isinstance` call instead of `(X, Y)`Ruff[UP038](https://docs.astral.sh/ruff/rules/non-pep604-isinstance)
make lint 
poetry run ruff check
graphiti_core/llm_client/client.py:43:8: UP038 Use `X | Y` in `isinstance` call instead of `(X, Y)`
   |
42 | def is_server_or_retry_error(exception: BaseException) -> bool:
43 |     if isinstance(exception, (RateLimitError, json.decoder.JSONDecodeError)):
   |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ UP038
44 |         return True
   |
   = help: Convert to `X | Y`

Found 1 error.
No fixes available (1 hidden fix can be enabled with the `--unsafe-fixes` option).
make: *** [lint] Error 1

How would you like me to deal with this?

Elipsis-Dev type: ignore issue

In regards to the type: ignore comment in anthropic_client.py, I am getting a pylance/pyright error in my IDE, Type of "input" is partially unknown\n Type of "input" is "dict[Unknown, Unknown]". The anthropic sdk defines the model as:

class ToolUseBlock(BaseModel):
    id: str

    input: object

    name: str

    type: Literal["tool_use"]

If I try any other method of solving this pyright error, for instance explicitly casting it as a ToolUseBlock, the issue is solved with the unknown attribute error, but then I get an error for unnecessary casting, because pyright already knows it is a ToolUseBlock. How would you like me to address this?

Thank you!

@evanmschultz
Copy link
Contributor Author

recheck

@getzep getzep locked and limited conversation to collaborators May 2, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants