-
Notifications
You must be signed in to change notification settings - Fork 589
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
Conversation
All contributors have signed the CLA ✍️ ✅ |
There was a problem hiding this 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 in8
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%
<= threshold80%
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%
<= threshold80%
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%
<= threshold80%
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%
<= threshold80%
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%
<= threshold80%
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%
<= threshold80%
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%
<= threshold80%
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%
<= threshold80%
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%
<= threshold80%
None
11. graphiti_core/llm_client/client.py:43
- Draft comment:
In the functionis_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
andNEO4j_PASSWORD
versusNEO4J_URI
). It would be clearer and more consistent to rename them toNEO4J_USER
andNEO4J_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.
graphiti_core/llm_client/client.py
Outdated
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)): |
There was a problem hiding this comment.
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)
.
if isinstance(exception, (RateLimitError | json.decoder.JSONDecodeError)): | |
if isinstance(exception, (RateLimitError, json.decoder.JSONDecodeError)): |
I have read the CLA Document and I hereby sign the CLA |
Elipsis-Dev Pipe issueWhen 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 issueIn regards to the type: ignore comment in anthropic_client.py, I am getting a pylance/pyright error in my IDE, 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! |
recheck |
Summary:
This PR focuses on streamlining our test suite and ensuring better consistency across test directories.
Key Changes:
Impact:
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_anthropic_client_int.py
fromtests/integrations
totests/llm_client
for consistency.test_anthropic_client.py
andtest_client.py
.test_anthropic_client.py
.test_anthropic_client.py
andtest_client.py
.anthropic_client.py
andclient.py
.This description was created by
for 40fc465. It will automatically update as commits are pushed.