making testing better #233
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
RoxyFarhad
force-pushed
the
RF/fix-test-error-handling
branch
from
a7cd369 to
bae85cd
Compare
RoxyFarhad
force-pushed
the
RF/fix-test-error-handling
branch
from
53ae413 to
14a32a6
Compare
| break | ||
|
|
||
| try: | ||
| await asyncio.wait_for(poll_for_task_creation(), timeout=30) |
There was a problem hiding this comment.
why do we wrap poll_messages in an asyncio.wait_for if poll_messages already has support for a timeout?
i can see you mentioned in the commit description that we now "break out of the loops when the expected messages have been processed" but it looks like at least in this test case we were also breaking out of the for loops when we found the expected messages before?
were we somehow polling for 30 seconds either way even if we found the task creation message?
| from .deployment_history_list_response import DeploymentHistoryListResponse as DeploymentHistoryListResponse | ||
| from .task_retrieve_by_name_params import TaskRetrieveByNameParams as TaskRetrieveByNameParams | ||
| from .message_list_paginated_params import MessageListPaginatedParams as MessageListPaginatedParams | ||
| from .deployment_history_list_params import DeploymentHistoryListParams as DeploymentHistoryListParams |
There was a problem hiding this comment.
should this just be in main already after the config changes?
There was a problem hiding this comment.
yeah it is weird, for some reason re-basing get rid of this diff for me ... will look into it
smoreinis
left a comment
smoreinis
left a comment
There was a problem hiding this comment.
LG, I'm still not totally clear on what the actual behavior change is when it looks like we were already breaking out of the poll_messages loop when we found the expected messages but I assume I'm missing something here about why this didn't work the way I'm reading it but approving this for now and you can help me understand this when you get a chance.
I think there's also a lint error in 050_agent_chat_guardrails in case you didn't see it.
The diff looks big but it isn't: