[TEST] Fixes BasicCurlHttpTests.RepeatedCallerThreadCancelsAreClean is flaky - #4524
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4524 +/- ##
==========================================
- Coverage 83.47% 83.46% -0.00%
==========================================
Files 521 521
Lines 20380 20380
==========================================
- Hits 17011 17009 -2
- Misses 3369 3371 +2 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟢 Approval recommended
The change is small and targeted to test flakiness, with only minor maintainability nits (unused failure counter) noted.
Pull request overview
This PR addresses flakiness in BasicCurlHttpTests.RepeatedCallerThreadCancelsAreClean by ensuring the test’s terminal-notification accounting includes additional failure outcomes that can occur during cancellation/connection-race scenarios in the curl HTTP client tests.
Changes:
- Treat
SessionState::ConnectFailedandSessionState::SendFailedas terminal outcomes forTerminalCountingHandler’s terminal counter. - Add tracking for failure outcomes (currently via a new
failed_count_counter).
File summaries
| File | Description |
|---|---|
ext/test/http/curl_http_test.cc |
Updates TerminalCountingHandler to count connect/send failures as terminal events to stabilize terminal totals in the flaky test scenario. |
Review details
Suppressed comments (1)
ext/test/http/curl_http_test.cc:145
- After removing the unused
failed_count_updates, this member can also be removed to keepTerminalCountingHandlerminimal.
std::atomic<int> terminal_count_{0};
std::atomic<int> cancelled_from_callback_{0};
std::atomic<int> failed_count_{0};
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
marcalff
left a comment
There was a problem hiding this comment.
Thanks for the root cause analysis and fix.
LGTM, see some cleanup around unused member variable.
Fixes #4510
Changes
For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes