[tests] Fix flaky TestNSUrlSessionHandlerSendClientCertificate. Fixes #25240#25258
[tests] Fix flaky TestNSUrlSessionHandlerSendClientCertificate. Fixes #25240#25258rolfbjarne merged 1 commit intomainfrom
Conversation
The test was flaky because: 1. EchoClientCertificateUrl was a plain field without an upfront network connectivity check (unlike all other URLs in NetworkResources), so the test would attempt the request even when the server was unreachable. 2. When the request completed with a non-timeout network error (e.g., connection reset, HTTP 502), the test would hard-fail instead of being marked as inconclusive. Fix both issues by: - Changing EchoClientCertificateUrl to use AssertNetworkConnection() for an upfront reachability check (consistent with other URLs in the class). - Adding IgnoreInCIIfBadNetwork(ex) before the assertion, matching the pattern used by other network-dependent tests in the file. Fixes #25240 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Reduces CI flakiness in TestNSUrlSessionHandlerSendClientCertificate by ensuring the test is skipped/inconclusive under known-bad network conditions instead of hard-failing.
Changes:
- Make
NetworkResources.EchoClientCertificateUrlconsistent with other network resources by routing it throughAssertNetworkConnection (...). - Treat non-timeout network failures in
TestNSUrlSessionHandlerSendClientCertificateas “bad network” in CI viaTestRuntime.IgnoreInCIIfBadNetwork (ex)before asserting success.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/monotouch-test/System.Net.Http/NetworkResources.cs | Ensures the client-certificate echo endpoint participates in the standard upfront network reachability/skip logic. |
| tests/monotouch-test/System.Net.Http/MessageHandlers.cs | Marks CI failures due to transient/bad network as ignored/inconclusive rather than hard failures for this test. |
✅ [PR Build #dede3df] Build passed (Build packages) ✅Pipeline on Agent |
✅ [PR Build #dede3df] Build passed (Detect API changes) ✅Pipeline on Agent |
✅ API diff for current PR / commitNET (empty diffs)✅ API diff vs stableNET (empty diffs)ℹ️ Generator diffGenerator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes) Pipeline on Agent |
✅ [CI Build #dede3df] Build passed (Build macOS tests) ✅Pipeline on Agent |
🚀 [CI Build #dede3df] Test results 🚀Test results✅ All tests passed on VSTS: test results. 🎉 All 156 tests passed 🎉 Tests counts✅ cecil: All 1 tests passed. Html Report (VSDrops) Download macOS tests✅ Tests on macOS Monterey (12): All 5 tests passed. Html Report (VSDrops) Download Linux Build VerificationPipeline on Agent |
The test
TestNSUrlSessionHandlerSendClientCertificatewas flaky due to two issues:Missing upfront network check:
EchoClientCertificateUrlwas a plain field withoutAssertNetworkConnection(), unlike all other URLs inNetworkResources. When the external Azure server was unreachable, the test would attempt the request anyway instead of being skipped.Missing network error handling: When the request completed with a non-timeout network error (connection reset, HTTP 502, etc.), the test hard-failed on
Assert.IsNull(ex)instead of being marked inconclusive. Other network-dependent tests in the file useIgnoreInCIIfBadNetwork(ex)for this purpose.Fix:
EchoClientCertificateUrlto useAssertNetworkConnection()(consistent with other URLs)IgnoreInCIIfBadNetwork(ex)before the assertion (consistent with other tests)Fixes #25240