test(core): Add support for retrying requests in network_reader_with_valid_http_header_kv_pairs on read failures to mitigate server-side errors.
#613
Conversation
WalkthroughThe pull request introduces modifications to the test cases for the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
components/core/tests/test-NetworkReader.cpp (4)
196-198: Improve comment clarity and grammarThe comment could be more professionally worded. Consider revising for clarity and grammar.
- // Since the http request sometimes crash due to arbitrary reasons, so we just try it - // multiple times to avoid such cases + // Implement retry logic to handle transient HTTP request failures
209-240: Consider adding timeout between retries and improve error reportingThe retry logic could be enhanced to:
- Add a small delay between retries to prevent overwhelming the server
- Log the specific failure reason for each attempt to aid debugging
bool is_pass{false}; for (size_t i{0}; i < cNumMaxTrials; ++i) { + if (i > 0) { + // Add exponential backoff delay + std::this_thread::sleep_for(std::chrono::milliseconds{100 * (1 << i)}); + } clp::NetworkReader reader{ "https://httpbin.org/headers", 0, false, clp::CurlDownloadHandler::cDefaultOverallTimeout, clp::CurlDownloadHandler::cDefaultConnectionTimeout, clp::NetworkReader::cDefaultBufferPoolSize, clp::NetworkReader::cDefaultBufferSize, valid_http_header_kv_pairs }; auto const content{get_content(reader)}; if (false == assert_curl_error_code(CURLE_OK, reader)) { + WARN(fmt::format("Attempt {} failed: CURL error", i + 1)); continue; } auto const parsed_content = nlohmann::json::parse(content); auto const& headers{parsed_content.at("headers")}; bool are_headers_same{true}; for (auto const& [key, value] : valid_http_header_kv_pairs) { if (value != headers.at(key).get<std::string_view>()) { are_headers_same = false; + WARN(fmt::format("Attempt {} failed: Header mismatch for key {}", i + 1, key)); break; } } if (!are_headers_same) { continue; } is_pass = true; break; }
244-246: Improve comment clarity and grammarSimilar to the previous test case, the comment should be revised for clarity and professionalism.
- // Since the http request sometimes crash due to arbitrary reasons, so we just try it - // multiple times to avoid such cases + // Implement retry logic to handle transient HTTP request failures
Consider adding test configuration for httpbin.org endpoint
The test suite would benefit from the following changes:
- Add a test configuration mechanism in
components/core/tests/to allow configuring external service endpoints- Define a timeout configuration alongside other network timeouts in
CurlDownloadHandler- Implement a test skip mechanism when the external service is unreachable
Locations requiring attention:
components/core/tests/test-NetworkReader.cpp: Add configuration for httpbin.org endpointcomponents/core/src/clp/CurlDownloadHandler: Add external service timeout configurationThe codebase shows no existing test configuration mechanism for external services, despite having various other configuration systems (GlobalMetadataDBConfig, RegexToWildcardTranslatorConfig). The NetworkReader tests are tightly coupled to httpbin.org, making them potentially fragile and dependent on external network conditions.
🔗 Analysis chain
Line range hint
196-279: Consider adding test configuration for httpbin.org endpointThe tests rely on an external service (httpbin.org). Consider:
- Making the endpoint configurable to allow testing against a local instance
- Adding a timeout configuration for the external service
- Adding a skip mechanism when the service is unavailable
Let me check if there are any test utilities or configuration mechanisms in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if httpbin.org is used elsewhere in the codebase rg --type cpp "httpbin.org" . # Check if there are any existing test configurations fd -e hpp -e cpp -e h "test.*config" .Length of output: 404
Script:
#!/bin/bash # Check for test utilities or helper files fd -e hpp -e cpp -e h "test.*util|test.*helper" . # Check for any configuration or environment setup in test files rg -g '*.cpp' -g '*.hpp' -g '*.h' "TEST_SETUP|ENVIRONMENT|Config" .Length of output: 13494
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
components/core/tests/test-NetworkReader.cpp(3 hunks)
🧰 Additional context used
📓 Learnings (1)
components/core/tests/test-NetworkReader.cpp (1)
Learnt from: LinZhihao-723
PR: y-scope/clp#593
File: components/core/tests/test-NetworkReader.cpp:216-219
Timestamp: 2024-11-15T03:15:45.919Z
Learning: In the `network_reader_with_valid_http_header_kv_pairs` test case in `components/core/tests/test-NetworkReader.cpp`, additional error handling for JSON parsing failures is not necessary, as the current error message is considered sufficient.
| bool is_pass{false}; | ||
| for (size_t i{0}; i < cNumMaxTrials; ++i) { | ||
| clp::NetworkReader reader{ | ||
| "https://httpbin.org/headers", | ||
| 0, | ||
| false, | ||
| clp::CurlDownloadHandler::cDefaultOverallTimeout, | ||
| clp::CurlDownloadHandler::cDefaultConnectionTimeout, | ||
| clp::NetworkReader::cDefaultBufferPoolSize, | ||
| clp::NetworkReader::cDefaultBufferSize, | ||
| illegal_header_kv_pairs | ||
| }; | ||
| auto const content = get_content(reader); | ||
| if (false == content.empty()) { | ||
| continue; | ||
| } | ||
| if (false == assert_curl_error_code(CURLE_BAD_FUNCTION_ARGUMENT, reader)) { | ||
| continue; | ||
| } | ||
| is_pass = true; | ||
| break; | ||
| } | ||
| REQUIRE(is_pass); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider extracting retry logic to a helper function
Both test cases use similar retry logic. Consider extracting this pattern into a helper function to improve code reuse and maintainability.
namespace {
template<typename TestFunc>
bool retry_test(TestFunc test_func, int max_trials = 10) {
for (size_t i{0}; i < max_trials; ++i) {
if (i > 0) {
std::this_thread::sleep_for(std::chrono::milliseconds{100 * (1 << i)});
}
if (test_func(i)) {
return true;
}
}
return false;
}
} // namespace
// Usage example:
bool is_pass = retry_test([&](size_t attempt) {
clp::NetworkReader reader{/*...*/};
auto const content = get_content(reader);
if (!content.empty()) {
WARN(fmt::format("Attempt {} failed: Non-empty content", attempt + 1));
return false;
}
if (!assert_curl_error_code(CURLE_BAD_FUNCTION_ARGUMENT, reader)) {
WARN(fmt::format("Attempt {} failed: Unexpected error code", attempt + 1));
return false;
}
return true;
});
There was a problem hiding this comment.
I think this comment makes sense. Let's add a template helper function to reduce code duplication
There was a problem hiding this comment.
@LinZhihao-723, glad to hear that you agree. Here's how you can add a template helper function to reduce code duplication:
First, define the helper function retry_test in the anonymous namespace at the top of your test file:
namespace {
template<typename TestFunc>
bool retry_test(TestFunc test_func, int max_trials = 10) {
for (size_t i{0}; i < max_trials; ++i) {
if (test_func(i)) {
return true;
}
// Optional: Add a short delay between retries if needed
// std::this_thread::sleep_for(std::chrono::milliseconds{100});
}
return false;
}
} // namespaceThen, modify your test cases to utilize this helper function.
For the network_reader_with_valid_http_header_kv_pairs test case:
TEST_CASE("network_reader_with_valid_http_header_kv_pairs", "[NetworkReader]") {
std::unordered_map<std::string, std::string> valid_http_header_kv_pairs;
constexpr int cNumHttpHeaderKeyValuePairs{10};
for (size_t i{0}; i < cNumHttpHeaderKeyValuePairs; ++i) {
valid_http_header_kv_pairs.emplace(
fmt::format("Unit-Test-Key{}", i),
fmt::format("Unit-Test-Value{}", i)
);
}
bool is_pass = retry_test([&](size_t attempt) {
clp::NetworkReader reader{
"https://httpbin.org/headers",
0,
false,
clp::CurlDownloadHandler::cDefaultOverallTimeout,
clp::CurlDownloadHandler::cDefaultConnectionTimeout,
clp::NetworkReader::cDefaultBufferPoolSize,
clp::NetworkReader::cDefaultBufferSize,
valid_http_header_kv_pairs
};
auto const content{get_content(reader)};
if (!assert_curl_error_code(CURLE_OK, reader)) {
return false;
}
auto const parsed_content = nlohmann::json::parse(content);
auto const& headers{parsed_content.at("headers")};
for (auto const& [key, value] : valid_http_header_kv_pairs) {
if (value != headers.at(key).get<std::string>()) {
return false;
}
}
return true;
});
REQUIRE(is_pass);
}And for the network_reader_with_illegal_http_header_kv_pairs test case:
TEST_CASE("network_reader_with_illegal_http_header_kv_pairs", "[NetworkReader]") {
auto illegal_header_kv_pairs = GENERATE(
std::unordered_map<std::string, std::string>{{"Range", "bytes=100-"}},
std::unordered_map<std::string, std::string>{{"RAnGe", "bytes=100-"}},
std::unordered_map<std::string, std::string>{{"Cache-Control", "no-cache"}},
std::unordered_map<std::string, std::string>{{"Pragma", "no-cache"}},
std::unordered_map<std::string, std::string>{{"Legal-Name", "CRLF\r\n"}}
);
bool is_pass = retry_test([&](size_t attempt) {
clp::NetworkReader reader{
"https://httpbin.org/headers",
0,
false,
clp::CurlDownloadHandler::cDefaultOverallTimeout,
clp::CurlDownloadHandler::cDefaultConnectionTimeout,
clp::NetworkReader::cDefaultBufferPoolSize,
clp::NetworkReader::cDefaultBufferSize,
illegal_header_kv_pairs
};
auto const content = get_content(reader);
if (!content.empty()) {
return false;
}
if (!assert_curl_error_code(CURLE_BAD_FUNCTION_ARGUMENT, reader)) {
return false;
}
return true;
});
REQUIRE(is_pass);
}By introducing the retry_test helper function, you encapsulate the retry logic, making your tests cleaner and more maintainable. This approach reduces code duplication and simplifies future modifications to the retry mechanism.
There was a problem hiding this comment.
I think this comment makes sense. Let's add a template helper function to reduce code duplication
Do we still need to do this after I get rid of the retry in illegal header unit test? Now it only retries when conduction regular header test.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
I think this comment makes sense. Let's add a template helper function to reduce code duplication
Do we still need to do this after I get rid of the retry in illegal header unit test? Now it only retries when conduction regular header test.
Sure, let's use the current implementation for now
| bool is_pass{false}; | ||
| for (size_t i{0}; i < cNumMaxTrials; ++i) { | ||
| clp::NetworkReader reader{ | ||
| "https://httpbin.org/headers", | ||
| 0, | ||
| false, | ||
| clp::CurlDownloadHandler::cDefaultOverallTimeout, | ||
| clp::CurlDownloadHandler::cDefaultConnectionTimeout, | ||
| clp::NetworkReader::cDefaultBufferPoolSize, | ||
| clp::NetworkReader::cDefaultBufferSize, | ||
| illegal_header_kv_pairs | ||
| }; | ||
| auto const content = get_content(reader); | ||
| if (false == content.empty()) { | ||
| continue; | ||
| } | ||
| if (false == assert_curl_error_code(CURLE_BAD_FUNCTION_ARGUMENT, reader)) { | ||
| continue; | ||
| } | ||
| is_pass = true; | ||
| break; | ||
| } | ||
| REQUIRE(is_pass); |
There was a problem hiding this comment.
I think this comment makes sense. Let's add a template helper function to reduce code duplication
| break; | ||
| } | ||
| } | ||
| if (!are_headers_same) { |
There was a problem hiding this comment.
| if (!are_headers_same) { | |
| if (false == are_headers_same) { |
| @@ -193,6 +193,9 @@ TEST_CASE("network_reader_illegal_offset", "[NetworkReader]") { | |||
| } | |||
|
|
|||
| TEST_CASE("network_reader_with_valid_http_header_kv_pairs", "[NetworkReader]") { | |||
| // Since the http request sometimes crash due to arbitrary reasons, so we just try it | |||
| // multiple times to avoid such cases | |||
| constexpr int cNumMaxTrials{10}; | |||
There was a problem hiding this comment.
| constexpr int cNumMaxTrials{10}; | |
| constexpr size_t cNumMaxTrials{10}; |
Avoid using int for counter like variables
| // Since the http request sometimes crash due to arbitrary reasons, so we just try it | ||
| // multiple times to avoid such cases |
There was a problem hiding this comment.
| // Since the http request sometimes crash due to arbitrary reasons, so we just try it | |
| // multiple times to avoid such cases | |
| // Retry the unit test a limited number of times to handle transient server-side HTTP errors. | |
| // This ensures the test is not marked as failed due to temporary issues beyond our control. |
| if (value != headers.at(key).get<std::string_view>()) { | ||
| are_headers_same = false; | ||
| break; | ||
| } |
There was a problem hiding this comment.
We should assert the returned headers are the same if we get a valid response from the server right?
There was a problem hiding this comment.
I think don't need to asset here:
- If not the same, it breaks the for loop then
are_headers_samewill be false, then continue to the next trial - If using assert here, then there won't be next trial any more, it will exit due to assertion failure exception
There was a problem hiding this comment.
But exiting due to assertion failure should be the expected behaviour in this case.
Adding this retry is to avoid server-end errors, which is caught in the previous continue already.
When the control flow reaches here, the server's response should be successful. If then we have unmatched headers, it's no longer the server's network issue, and we shouldn't swallow this mismatch.
| if (false == content.empty()) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
We don't need to retry for this unit test, right?
The header validation happens before sending HTTP requests. This unit test should always work regardless of server-end failures.
There was a problem hiding this comment.
yea, right, will get rid of it
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
components/core/tests/test-NetworkReader.cpp (1)
209-220: Add exponential backoff between retry attempts.Consider adding a delay between retries to prevent overwhelming the server and improve test reliability.
for (size_t i{0}; i < cNumMaxTrials; ++i) { + if (i > 0) { + // Exponential backoff: 100ms, 200ms, 400ms, etc. + std::this_thread::sleep_for(std::chrono::milliseconds{100 * (1 << i)}); + } clp::NetworkReader reader{
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
components/core/tests/test-NetworkReader.cpp(1 hunks)
🧰 Additional context used
📓 Learnings (1)
components/core/tests/test-NetworkReader.cpp (1)
Learnt from: LinZhihao-723
PR: y-scope/clp#593
File: components/core/tests/test-NetworkReader.cpp:216-219
Timestamp: 2024-11-15T03:15:45.919Z
Learning: In the `network_reader_with_valid_http_header_kv_pairs` test case in `components/core/tests/test-NetworkReader.cpp`, additional error handling for JSON parsing failures is not necessary, as the current error message is considered sufficient.
🔇 Additional comments (1)
components/core/tests/test-NetworkReader.cpp (1)
196-198: LGTM! Clear documentation and proper type usage.
The comments effectively explain the retry mechanism's purpose, and using size_t for the trial counter follows best practices.
| auto const content{get_content(reader)}; | ||
| if (false == assert_curl_error_code(CURLE_OK, reader)) { | ||
| continue; | ||
| } | ||
| auto const parsed_content = nlohmann::json::parse(content); | ||
| auto const& headers{parsed_content.at("headers")}; | ||
| for (auto const& [key, value] : valid_http_header_kv_pairs) { | ||
| is_pass = value == headers.at(key).get<std::string_view>(); | ||
| REQUIRE(is_pass); | ||
| } | ||
| break; | ||
| } | ||
| REQUIRE(is_pass); |
There was a problem hiding this comment.
Improve header validation logic to handle all cases correctly.
The current implementation has several issues:
- The
is_passflag is overwritten in each iteration of the header validation loop - The
REQUIREinside the loop makes the finalREQUIRE(is_pass)redundant - A failure in any header validation will exit the test immediately, making the retry mechanism ineffective for header mismatches
- for (auto const& [key, value] : valid_http_header_kv_pairs) {
- is_pass = value == headers.at(key).get<std::string_view>();
- REQUIRE(is_pass);
- }
- break;
- }
- REQUIRE(is_pass);
+ bool are_headers_same{true};
+ for (auto const& [key, value] : valid_http_header_kv_pairs) {
+ if (value != headers.at(key).get<std::string_view>()) {
+ are_headers_same = false;
+ break;
+ }
+ }
+ if (are_headers_same) {
+ is_pass = true;
+ break;
+ }
+ }
+ REQUIRE(is_pass);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| auto const content{get_content(reader)}; | |
| if (false == assert_curl_error_code(CURLE_OK, reader)) { | |
| continue; | |
| } | |
| auto const parsed_content = nlohmann::json::parse(content); | |
| auto const& headers{parsed_content.at("headers")}; | |
| for (auto const& [key, value] : valid_http_header_kv_pairs) { | |
| is_pass = value == headers.at(key).get<std::string_view>(); | |
| REQUIRE(is_pass); | |
| } | |
| break; | |
| } | |
| REQUIRE(is_pass); | |
| auto const content{get_content(reader)}; | |
| if (false == assert_curl_error_code(CURLE_OK, reader)) { | |
| continue; | |
| } | |
| auto const parsed_content = nlohmann::json::parse(content); | |
| auto const& headers{parsed_content.at("headers")}; | |
| bool are_headers_same{true}; | |
| for (auto const& [key, value] : valid_http_header_kv_pairs) { | |
| if (value != headers.at(key).get<std::string_view>()) { | |
| are_headers_same = false; | |
| break; | |
| } | |
| } | |
| if (are_headers_same) { | |
| is_pass = true; | |
| break; | |
| } | |
| } | |
| REQUIRE(is_pass); |
| auto const parsed_content = nlohmann::json::parse(content); | ||
| auto const& headers{parsed_content.at("headers")}; | ||
| for (auto const& [key, value] : valid_http_header_kv_pairs) { | ||
| is_pass = value == headers.at(key).get<std::string_view>(); | ||
| REQUIRE(is_pass); | ||
| } |
There was a problem hiding this comment.
This part doesn't have to be in the loop, right? We can just iterate cNumMaxTrials times until get_content has returned with assert_curl_error_code successful. I would suggest the following code structure:
- Set an optional content to be
std::nullopt - Loop
cNumMaxTrialstimes and try to read content; if success, set the optional content to the actual read content and break - Assert the content is not
std::nullopt - Assert the headers in the response matches expected ones
| @@ -193,33 +193,44 @@ TEST_CASE("network_reader_illegal_offset", "[NetworkReader]") { | |||
| } | |||
|
|
|||
| TEST_CASE("network_reader_with_valid_http_header_kv_pairs", "[NetworkReader]") { | |||
| // Retry the unit test a limited number of times to handle transient server-side HTTP errors. | |||
| // This ensures the test is not marked as failed due to temporary issues beyond our control. | |||
| constexpr size_t cNumMaxTrials{10}; | |||
There was a problem hiding this comment.
How about moving right before the loop starts
There was a problem hiding this comment.
Any plan on this comment?
There was a problem hiding this comment.
Just to clarify: we should try to make variable declarations as close to where it's been used as possible
| content.emplace(get_content(reader)); | ||
| if (false == assert_curl_error_code(CURLE_OK, reader)) { | ||
| continue; | ||
| } | ||
| break; |
There was a problem hiding this comment.
| content.emplace(get_content(reader)); | |
| if (false == assert_curl_error_code(CURLE_OK, reader)) { | |
| continue; | |
| } | |
| break; | |
| if (assert_curl_error_code(CURLE_OK, reader)) { | |
| content.emplace(get_content(reader)); | |
| break; | |
| } |
content.emplace(get_content(reader))should only execute if the return code is correct, otherwise the coming assertions oncontent.has_value()is meaningless.- I think this if condition will make the code cleaner
There was a problem hiding this comment.
But I believe that it must call get_content first, otherwise the code in the reader won’t be set right?
There was a problem hiding this comment.
Right, but you can do this:
| content.emplace(get_content(reader)); | |
| if (false == assert_curl_error_code(CURLE_OK, reader)) { | |
| continue; | |
| } | |
| break; | |
| auto content = get_content(reader); | |
| if (assert_curl_error_code(CURLE_OK, reader)) { | |
| optional_content.emplace(std::move(content)); | |
| break; | |
| } |
| @@ -193,33 +193,44 @@ TEST_CASE("network_reader_illegal_offset", "[NetworkReader]") { | |||
| } | |||
|
|
|||
| TEST_CASE("network_reader_with_valid_http_header_kv_pairs", "[NetworkReader]") { | |||
| // Retry the unit test a limited number of times to handle transient server-side HTTP errors. | |||
| // This ensures the test is not marked as failed due to temporary issues beyond our control. | |||
| constexpr size_t cNumMaxTrials{10}; | |||
There was a problem hiding this comment.
Any plan on this comment?
| auto const content{get_content(reader)}; | ||
| REQUIRE(assert_curl_error_code(CURLE_OK, reader)); | ||
| auto const parsed_content = nlohmann::json::parse(content); | ||
| std::optional<std::vector<char>> content{std::nullopt}; |
There was a problem hiding this comment.
| std::optional<std::vector<char>> content{std::nullopt}; | |
| std::optional<std::vector<char>> content; |
We can rely on the default constructor if the template provides any
There was a problem hiding this comment.
Please reply this one since it's been ignored for three rounds of conversation: https://github.com/y-scope/clp/pull/613/files#r1861447753
| auto const content{get_content(reader)}; | ||
| REQUIRE(assert_curl_error_code(CURLE_OK, reader)); | ||
| auto const parsed_content = nlohmann::json::parse(content); | ||
| std::optional<std::vector<char>> content; |
There was a problem hiding this comment.
| std::optional<std::vector<char>> content; | |
| std::optional<std::vector<char>> optional_content; |
Missed in the previous review, mb
| content.emplace(get_content(reader)); | ||
| if (false == assert_curl_error_code(CURLE_OK, reader)) { | ||
| continue; | ||
| } | ||
| break; |
There was a problem hiding this comment.
Right, but you can do this:
| content.emplace(get_content(reader)); | |
| if (false == assert_curl_error_code(CURLE_OK, reader)) { | |
| continue; | |
| } | |
| break; | |
| auto content = get_content(reader); | |
| if (assert_curl_error_code(CURLE_OK, reader)) { | |
| optional_content.emplace(std::move(content)); | |
| break; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
components/core/tests/test-NetworkReader.cpp(1 hunks)
🧰 Additional context used
📓 Learnings (1)
components/core/tests/test-NetworkReader.cpp (1)
Learnt from: LinZhihao-723
PR: y-scope/clp#593
File: components/core/tests/test-NetworkReader.cpp:216-219
Timestamp: 2024-11-15T03:15:45.919Z
Learning: In the `network_reader_with_valid_http_header_kv_pairs` test case in `components/core/tests/test-NetworkReader.cpp`, additional error handling for JSON parsing failures is not necessary, as the current error message is considered sufficient.
🔇 Additional comments (3)
components/core/tests/test-NetworkReader.cpp (3)
199-199: LGTM! Appropriate type for counter variable.
The change from int to size_t is correct as it better represents a non-negative counter variable.
206-226: LGTM! Well-implemented retry logic.
The retry logic is well-implemented with:
- Clear use of optional to track content
- Proper error handling for transient server errors
- Good separation of content retrieval and error checking
Line range hint 227-232: LGTM! Robust header validation.
The header validation is properly implemented with:
- Content existence check
- Proper JSON parsing
- Thorough validation of all header key-value pairs
- Appropriate use of REQUIRE for assertions
| std::optional<std::vector<char>> optional_content; | ||
| // Retry the unit test a limited number of times to handle transient server-side HTTP errors. | ||
| // This ensures the test is not marked as failed due to temporary issues beyond our control. | ||
| constexper size_t cNumMaxTrials{10}; |
There was a problem hiding this comment.
Fix typo in keyword.
There's a typo in the keyword: constexper should be constexpr.
- constexper size_t cNumMaxTrials{10};
+ constexpr size_t cNumMaxTrials{10};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| constexper size_t cNumMaxTrials{10}; | |
| constexpr size_t cNumMaxTrials{10}; |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
components/core/tests/test-NetworkReader.cpp (1)
206-226: LGTM: Well-structured retry logicThe retry mechanism is well-implemented with clear comments explaining its purpose. Consider adding debug logs for failed attempts to help with troubleshooting.
if (assert_curl_error_code(CURLE_OK, reader)) { optional_content.emplace(content); break; +} else { + WARN(fmt::format("Attempt {} failed", i + 1)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
components/core/tests/test-NetworkReader.cpp(1 hunks)
🧰 Additional context used
📓 Learnings (1)
components/core/tests/test-NetworkReader.cpp (1)
Learnt from: LinZhihao-723
PR: y-scope/clp#593
File: components/core/tests/test-NetworkReader.cpp:216-219
Timestamp: 2024-11-15T03:15:45.919Z
Learning: In the `network_reader_with_valid_http_header_kv_pairs` test case in `components/core/tests/test-NetworkReader.cpp`, additional error handling for JSON parsing failures is not necessary, as the current error message is considered sufficient.
🔇 Additional comments (2)
components/core/tests/test-NetworkReader.cpp (2)
199-199: LGTM: Appropriate type for container size
The change from int to size_t is correct as it's used for container size and loop counter.
Line range hint 227-232: LGTM: Thorough header validation
The header validation is implemented correctly, with proper assertions to ensure each header matches the expected value.
anlowee
changed the title
anlowee
changed the title
network_reader_with_valid_http_header_kv_pairs on read failures to mitigate server-side errors.
Co-authored-by: Xiaochong Wei <[email protected]>
Co-authored-by: Xiaochong Wei <[email protected]>
Description
Allow up to 10 trials when unitesting
network_reader_with_valid_http_header_kv_pairsandnetwork_reader_with_illegal_http_header_kv_pairsValidation performed
Run these two unittest cases and get passed.
Summary by CodeRabbit
NetworkReaderclass by adding retry logic for HTTP requests, reducing the likelihood of test failures due to transient issues.