making testing better#233
Conversation
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
RoxyFarhadforce-pushed the RF/fix-test-error-handling branch from a7cd369 to bae85cdCompareRoxyFarhadforce-pushed the RF/fix-test-error-handling branch from 53ae413 to 14a32a6Compare| break | ||
| try: | ||
| awaitasyncio.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_responseimportDeploymentHistoryListResponseasDeploymentHistoryListResponse | ||
| from .task_retrieve_by_name_paramsimportTaskRetrieveByNameParamsasTaskRetrieveByNameParams | ||
| from .message_list_paginated_paramsimportMessageListPaginatedParamsasMessageListPaginatedParams | ||
| from .deployment_history_list_paramsimportDeploymentHistoryListParamsasDeploymentHistoryListParams |
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.
RoxyFarhad commented Dec 22, 2025
@smoreinis reverted a lot of these changes back because you were right -> the only thing I added in some of the tests were the early breaking if the messages were all discovered which has sped up some tests |
RoxyFarhadforce-pushed the RF/fix-test-error-handling branch from 31e2305 to b80074aCompare
The diff looks big but it isn't: