fix streaming initial response for windows clients #3347
Conversation
sbera87
changed the title
tests/aws-cpp-sdk-bedrock-runtime-integration-tests/IntegrationTests.cpp
Show resolved
Hide resolved
| { | ||
| if (::testing::Test::HasFailure()) | ||
| { | ||
| std::cout << "Test traces: " << testTrace << "\n"; |
There was a problem hiding this comment.
dont use std::cout use a AWS_LOG macro or google test assertion. additionally where is testTrace updated? can this be remove entirely? i sont see this string updated anywhere?
There was a problem hiding this comment.
This is a pattern I used in the transcribe streaming tests where I wanted to keep test debug traces, but don't pollute output in case of success.
Likely to be a copy error from the test used as a template, and likely to be removed.
There was a problem hiding this comment.
shall remove the debug messages
| streamHandler->SetInitialResponseCallbackEx([&](const Aws::BedrockRuntime::Model::ConverseStreamInitialResponse& , const Aws::Utils::Event::InitialResponseType awsResponseType) | ||
| { | ||
| std::unique_lock<std::mutex> lock(mutex); | ||
| cv.wait(lock, [&] { |
There was a problem hiding this comment.
never use wait in tests, always use wait_for or wait_until with reasonable timeouts. we have had too many instances of tests deadlocking on CI servers
additionally do want to wait here? seems like we actually arent waiting as this will never be woken up if the condition is not true the first time. seems like more so we just want to assert that awsResponseType is ON_RESPONSE and the mutex will handle the synchronized update of responseReceived with no wait needed
There was a problem hiding this comment.
I have simplified it in the next commit.
| @@ -0,0 +1,36 @@ | |||
| add_project(aws-cpp-sdk-bedrock-runtime-integration-tests | |||
| "Tests for Bedrock Runtime C++ SDK" | |||
| #aws-cpp-sdk-access-management | |||
There was a problem hiding this comment.
remove commented out dep on access-management
sbera87
force-pushed
the
winclient
branch
2 times, most recently
from
c720ed7 to
c11cac0
Compare
| m_client = Aws::MakeShared<BedrockRuntimeClient>(ALLOCATION_TAG, config); | ||
| } | ||
|
|
||
| void TearDown() |
There was a problem hiding this comment.
nit: should remove TearDown if its a empty function
Issue #, if available:
Fix initial response handling for windows http client
Description of changes:
Check all that applies:
Check which platforms you have built SDK on to verify the correctness of this PR.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.