-
Notifications
You must be signed in to change notification settings - Fork 1
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
Faststream poc #399
base: main
Are you sure you want to change the base?
Faststream poc #399
Conversation
❌ 12 Tests Failed:
View the top 3 failed tests by shortest run timetests.agent.test_agent�test_gx_agent_sends_request_to_create_scheduled_job tests.agent.test_agent�test_gx_agent_updates_cloud_on_job_status tests.agent.test_agent�test_gx_agent_run_handles_client_authentication_error_on_init To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard |
processed_successfully=mock.Mock(), | ||
processed_with_failures=mock.Mock(), | ||
redeliver_message=mock.Mock(), |
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.
Why are the values here set to mock.Mock()
?
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.
+1
@@ -1 +1 @@ | |||
3.10.14 | |||
3.11.9 |
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.
Is it best to bring this change in separately?
# Use Retries = 10, --workers 1 | ||
# Data Context as a Dependency |
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.
I'm not sure what these comments mean, should they stay?
# Use Retries = 10, --workers 1 | |
# Data Context as a Dependency |
@@ -10,9 +10,16 @@ | |||
from functools import partial | |||
from importlib.metadata import version as metadata_version | |||
from typing import TYPE_CHECKING, Any, Callable, Dict, Final, Literal | |||
from unittest import mock |
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.
Shouldn't have any unittest
imports in production
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.
unittest
dep should not be in application code
try: | ||
organization_id = gx_agent.get_organization_id(event_context) | ||
result = gx_agent._handle_event(event_context) | ||
except Exception as e: |
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.
Can this be a more narrow catch?
if result.job_duration | ||
else None, | ||
"job_duration": ( | ||
result.job_duration.total_seconds() if result.job_duration else None |
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.
This seems to be out of scope of your changes, but a job_duration
of 0 seconds would result in None
No description provided.