Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add retries to tests relying on Http bin to improve test resiliency. #5692

Closed
wants to merge 2 commits into from
Closed

Add retries to tests relying on Http bin to improve test resiliency. #5692

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4ffee7f
Add retries to tests relying on Http bin to improve test resiliency.
ahsonkhan Jun 4, 2024
368cfc8
Print exception message before retrying.
ahsonkhan Jun 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 54 additions & 14 deletions sdk/core/azure-core/test/ut/curl_connection_pool_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -599,12 +599,31 @@ namespace Azure { namespace Core { namespace Test {
// Simulate connection lost (like server disconnection).
connection->Shutdown();

auto failedCounter = 0;
for (auto i = 0; i < _detail::AzureSdkHttpbinRetryCount; i++)
{
// Check that CURLE_SEND_ERROR is produced when trying to use the connection.
auto session
= std::make_unique<Azure::Core::Http::CurlSession>(req, std::move(connection), options);
auto r = session->Perform(Azure::Core::Context::ApplicationContext);
EXPECT_EQ(CURLE_SEND_ERROR, r);
GTEST_LOG_(INFO) << "resiliencyOnConnectionClosed test iteration " << i << ".";
try
{
// Check that CURLE_SEND_ERROR is produced when trying to use the connection.
auto session = std::make_unique<Azure::Core::Http::CurlSession>(
req, std::move(connection), options);
auto r = session->Perform(Azure::Core::Context::ApplicationContext);
EXPECT_EQ(CURLE_SEND_ERROR, r);
break; // Test passed, no need to retry.
}
catch (Azure::Core::Http::TransportException const& ex)
{
// CURL returns a connection error which triggers a transport exception.
GTEST_LOG_(INFO) << "resiliencyOnConnectionClosed test iteration " << i
<< " failed with a TransportException. " << ex.what();
failedCounter++;
// We allow 1 intermittent failure, due to networking issues.
if (failedCounter > 1)
{
throw;
}
}
}
}

Expand All @@ -617,17 +636,38 @@ namespace Azure { namespace Core { namespace Test {
auto connection
= CurlConnectionPool::g_curlConnectionPool.ExtractOrCreateCurlConnection(req, options);

auto failedCounter = 0;
for (auto i = 0; i < _detail::AzureSdkHttpbinRetryCount; i++)
{
// Check we can use the connection to retrieve headers in the response, but the connection
// is still flagged as shutdown.
auto session
= std::make_unique<Azure::Core::Http::CurlSession>(req, std::move(connection), options);
GTEST_LOG_(INFO) << "forceConnectionClosed test iteration " << i << ".";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of these appear to be essentially identical. Is it possible to centralize this logic in a single function that encapsulates the retry logic and includes the test-specific behavior in a lambda?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to centralize the logic, but I prefer tests to be very simple/isolated and find the readability of lambdas a bit cognitively burdensome. If I can't find a simpler way to achieve that, will fall back to the lambda approach.

try
{
// Check we can use the connection to retrieve headers in the response, but the connection
// is still flagged as shutdown.
auto session = std::make_unique<Azure::Core::Http::CurlSession>(
req, std::move(connection), options);

auto r = session->Perform(Azure::Core::Context::ApplicationContext);
EXPECT_EQ(CURLE_OK, r);
auto response = session->ExtractResponse();
EXPECT_EQ(
response->GetStatusCode(), Azure::Core::Http::HttpStatusCode::SwitchingProtocols);
EXPECT_EQ("close", response->GetHeaders().find("Connection")->second);
break; // Test passed, no need to retry.
}
catch (Azure::Core::Http::TransportException const& ex)
{

auto r = session->Perform(Azure::Core::Context::ApplicationContext);
EXPECT_EQ(CURLE_OK, r);
auto response = session->ExtractResponse();
EXPECT_EQ(response->GetStatusCode(), Azure::Core::Http::HttpStatusCode::SwitchingProtocols);
EXPECT_EQ("close", response->GetHeaders().find("Connection")->second);
// CURL returns a connection error which triggers a transport exception.
ahsonkhan marked this conversation as resolved.
Show resolved Hide resolved
GTEST_LOG_(INFO) << "forceConnectionClosed test iteration " << i
<< " failed with a TransportException. " << ex.what();
failedCounter++;
// We allow 1 intermittent failure, due to networking issues.
if (failedCounter > 1)
{
throw;
}
}
}
}

Expand Down
99 changes: 95 additions & 4 deletions sdk/core/azure-core/test/ut/curl_options_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,30 @@ namespace Azure { namespace Core { namespace Test {
Azure::Core::Http::Request request(Azure::Core::Http::HttpMethod::Get, url);

std::unique_ptr<Azure::Core::Http::RawResponse> response;
EXPECT_NO_THROW(response = pipeline.Send(request, Azure::Core::Context::ApplicationContext));

auto failedCounter = 0;
for (auto i = 0; i < _detail::AzureSdkHttpbinRetryCount; i++)
{
GTEST_LOG_(INFO) << "noRevoke test iteration " << i << ".";
try
{
EXPECT_NO_THROW(
response = pipeline.Send(request, Azure::Core::Context::ApplicationContext));
break; // Test passed, no need to retry.
}
catch (Azure::Core::Http::TransportException const& ex)
{
// CURL returns a connection error which triggers a transport exception.
GTEST_LOG_(INFO) << "noRevoke test iteration " << i << " failed with a TransportException. "
<< ex.what();
failedCounter++;
// We allow 1 intermittent failure, due to networking issues.
if (failedCounter > 1)
{
throw;
}
}
}
if (response)
{
auto responseCode = response->GetStatusCode();
Expand Down Expand Up @@ -220,7 +243,30 @@ namespace Azure { namespace Core { namespace Test {
Azure::Core::Http::Request request(Azure::Core::Http::HttpMethod::Get, url);

std::unique_ptr<Azure::Core::Http::RawResponse> response;
EXPECT_NO_THROW(response = pipeline.Send(request, Azure::Core::Context::ApplicationContext));

auto failedCounter = 0;
for (auto i = 0; i < _detail::AzureSdkHttpbinRetryCount; i++)
{
GTEST_LOG_(INFO) << "sslVerifyOff test iteration " << i << ".";
try
{
EXPECT_NO_THROW(
response = pipeline.Send(request, Azure::Core::Context::ApplicationContext));
break; // Test passed, no need to retry.
}
catch (Azure::Core::Http::TransportException const& ex)
{
// CURL returns a connection error which triggers a transport exception.
GTEST_LOG_(INFO) << "sslVerifyOff test iteration " << i
<< " failed with a TransportException. " << ex.what();
failedCounter++;
// We allow 1 intermittent failure, due to networking issues.
if (failedCounter > 1)
{
throw;
}
}
}
auto responseCode = response->GetStatusCode();
int expectedCode = 200;
EXPECT_PRED2(
Expand Down Expand Up @@ -313,7 +359,30 @@ namespace Azure { namespace Core { namespace Test {
Azure::Core::Http::Request request(Azure::Core::Http::HttpMethod::Get, url);

std::unique_ptr<Azure::Core::Http::RawResponse> response;
EXPECT_NO_THROW(response = pipeline.Send(request, Azure::Core::Context::ApplicationContext));

auto failedCounter = 0;
for (auto i = 0; i < _detail::AzureSdkHttpbinRetryCount; i++)
{
GTEST_LOG_(INFO) << "httpsDefault test iteration " << i << ".";
try
{
EXPECT_NO_THROW(
response = pipeline.Send(request, Azure::Core::Context::ApplicationContext));
break; // Test passed, no need to retry.
}
catch (Azure::Core::Http::TransportException const& ex)
{
// CURL returns a connection error which triggers a transport exception.
GTEST_LOG_(INFO) << "httpsDefault test iteration " << i
<< " failed with a TransportException. " << ex.what();
failedCounter++;
// We allow 1 intermittent failure, due to networking issues.
if (failedCounter > 1)
{
throw;
}
}
}
auto responseCode = response->GetStatusCode();
int expectedCode = 200;
EXPECT_PRED2(
Expand Down Expand Up @@ -350,7 +419,29 @@ namespace Azure { namespace Core { namespace Test {
Azure::Core::Http::Request request(Azure::Core::Http::HttpMethod::Get, url);

std::unique_ptr<Azure::Core::Http::RawResponse> response;
EXPECT_NO_THROW(response = pipeline.Send(request, Azure::Core::Context::ApplicationContext));
auto failedCounter = 0;
for (auto i = 0; i < _detail::AzureSdkHttpbinRetryCount; i++)
{
GTEST_LOG_(INFO) << "disableKeepAlive test iteration " << i << ".";
try
{
EXPECT_NO_THROW(
response = pipeline.Send(request, Azure::Core::Context::ApplicationContext));
break; // Test passed, no need to retry.
}
catch (Azure::Core::Http::TransportException const& ex)
{
// CURL returns a connection error which triggers a transport exception.
GTEST_LOG_(INFO) << "disableKeepAlive test iteration " << i
<< " failed with a TransportException. " << ex.what();
failedCounter++;
// We allow 1 intermittent failure, due to networking issues.
if (failedCounter > 1)
{
throw;
}
}
}
auto responseCode = response->GetStatusCode();
int expectedCode = 200;
EXPECT_PRED2(
Expand Down
4 changes: 4 additions & 0 deletions sdk/core/azure-core/test/ut/transport_adapter_base_test.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ namespace Azure { namespace Core { namespace Test {
namespace _detail {
constexpr static const char AzureSdkHttpbinServerSchema[] = "https";
constexpr static const char AzureSdkHttpbinServer[] = "azuresdkforcpp.azurewebsites.net";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this logic be moved into a single source rather than being defined in two different places?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's a good suggestion. I will see how to remove the existing duplication here.


// If we see intermittent test failures due to Httpbin reliability, even with a couple retries
// we'd revisit the use of this for tests, rather than increasing retries.
constexpr static const int AzureSdkHttpbinRetryCount = 2;
} // namespace _detail

struct AzureSdkHttpbinServer final
Expand Down
11 changes: 8 additions & 3 deletions sdk/core/azure-core/test/ut/transport_policy_options.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,10 @@ namespace Azure { namespace Core { namespace Test {
namespace {
constexpr static const char AzureSdkHttpbinServerSchema[] = "https";
constexpr static const char AzureSdkHttpbinHost[] = "azuresdkforcpp.azurewebsites.net";

// If we see intermittent test failures due to Httpbin reliability, even with a couple retries
// we'd revisit the use of this for tests, rather than increasing retries.
constexpr static const int AzureSdkHttpbinRetryCount = 2;
} // namespace
class TransportAdapterOptions : public ::testing::Test {

Expand Down Expand Up @@ -433,7 +437,7 @@ namespace Azure { namespace Core { namespace Test {

// HTTP Connections.
auto failedCounter = 0;
for (auto i = 0; i < 3; i++)
for (auto i = 0; i < AzureSdkHttpbinRetryCount; i++)
{
GTEST_LOG_(INFO) << "DisableCrlValidation test iteration " << i << ".";
try
Expand All @@ -448,12 +452,13 @@ namespace Azure { namespace Core { namespace Test {
auto request = Azure::Core::Http::Request(Azure::Core::Http::HttpMethod::Get, testUrl);
auto response = pipeline.Send(request, Azure::Core::Context::ApplicationContext);
EXPECT_EQ(response->GetStatusCode(), Azure::Core::Http::HttpStatusCode::Ok);
break; // Test passed, no need to retry.
}
catch (Azure::Core::Http::TransportException const&)
catch (Azure::Core::Http::TransportException const& ex)
{
// CURL returns a connection error which triggers a transport exception.
LarryOsterman marked this conversation as resolved.
Show resolved Hide resolved
GTEST_LOG_(INFO) << "DisableCrlValidation test iteration " << i
<< " failed with a TransportException.";
<< " failed with a TransportException. " << ex.what();
failedCounter++;
// We allow 1 intermittent failure, due to networking issues.
if (failedCounter > 1)
Expand Down