-
Notifications
You must be signed in to change notification settings - Fork 105
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(ci): make test suites v3 ready #1063
base: main
Are you sure you want to change the base?
Conversation
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.
Disclaimer: Experimental PR review
PR Summary
This PR updates the test suite for v3 compatibility by implementing retry logic for HTTP requests, transitioning to queue-based ingestion, and modifying test metadata structures.
- Added new
HTTPClientWithRetries
class intests/utils.py
with exponential backoff (4 retries within 3s) for non-500 errors - Updated CI environment variables in
.github/workflows/ci.yml
to support queue-based ingestion with configurable delays - Changed mock metadata in
tests/test_decorators.py
from strings to dictionaries for more realistic testing scenarios - Removed direct database access environment variables from CI workflow in favor of queue-based processing
3 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile
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.
Disclaimer: Experimental PR review
PR Summary
(updates since last review)
Based on the recent changes not covered in the previous review, here's a focused summary of the additional modifications:
Updates test suites for v3 API compatibility with significant changes to test infrastructure and timing handling.
- Replaced
HTTPClientWithRetries
with simplesleep(1)
delays in API calls to handle race conditions more consistently - Updated test assertions to expect empty dict ({}) instead of None for metadata across test files
- Migrated imports to use new langchain package structure (e.g.
langchain_openai
instead oflangchain_community
) - Added parallel test execution support with pytest
-n auto
flag in CI workflow - Added new test cases for multimodal inputs and structured outputs in OpenAI integration tests
The changes focus on test reliability and v3 API compatibility while simplifying the HTTP request handling approach.
10 file(s) reviewed, 4 comment(s)
Edit PR Review Bot Settings | Greptile
url = f"{self.BASE_URL}/api/public/observations/{observation_id}" | ||
response = httpx.get(url, auth=self.auth) | ||
return response.json() |
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.
logic: no error handling for failed requests - should catch httpx.HTTPError and handle appropriately
@@ -11,23 +12,27 @@ def __init__(self, username=None, password=None, base_url=None): | |||
self.BASE_URL = base_url if base_url else os.environ["LANGFUSE_HOST"] | |||
|
|||
def get_observation(self, observation_id): | |||
sleep(1) | |||
url = f"{self.BASE_URL}/api/public/observations/{observation_id}" | |||
response = httpx.get(url, auth=self.auth) |
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.
logic: missing timeout parameter on request - could hang indefinitely
assert observation.name == str(i) | ||
assert observation.metadata == {"count": str(i)} | ||
assert observation.metadata == {"count": i} |
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.
logic: metadata value is now an integer instead of string, which could break existing code expecting string values
assert trace["scores"][0]["value"] == 0 | ||
assert trace["scores"][0]["stringValue"] == "high score" |
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.
logic: categorical score value changed from None to 0, which could affect code that checks for null values
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.
Disclaimer: Experimental PR review
PR Summary
(updates since last review)
Based on the recent changes not covered in previous reviews, here's a focused summary of the modifications:
Increased API call delay timing in test utilities to improve test stability and reliability.
- Increased sleep duration from 1s to 1.5s in
tests/utils.py
get_api() function to better handle race conditions - No other significant changes found since last review
The change is minimal but important for test reliability, allowing more time for API operations to complete before subsequent test assertions.
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
Important
Update CI and test suite for version 3 compatibility, including environment variable adjustments and test refactoring.
.github/workflows/ci.yml
to setLANGFUSE_INGESTION_QUEUE_DELAY_MS
andLANGFUSE_INGESTION_CLICKHOUSE_WRITE_INTERVAL_MS
.-n auto
.langfuse/callback/langchain.py
for better organization.sleep(1)
intests/api_wrapper.py
andtests/test_core_sdk.py
to ensure data consistency.tests/test_core_sdk.py
to check for specific metadata and input/output values.get_api()
calls in multiple test files to include a delay for API readiness.tests/test_langchain.py
andtests/test_openai.py
to handle new API response structures.This description was created by for d0b3a2b. It will automatically update as commits are pushed.