-
Notifications
You must be signed in to change notification settings - Fork 564
[tests] Assert.Inconclusive() tests with http errors
#10650
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2,8 +2,10 @@ | |||||||||||||||||
| using System.Collections.Concurrent; | ||||||||||||||||||
| using System.IO; | ||||||||||||||||||
| using System.Linq; | ||||||||||||||||||
| using System.Net; | ||||||||||||||||||
| using System.Net.Http; | ||||||||||||||||||
| using System.Security.Cryptography; | ||||||||||||||||||
| using NUnit.Framework; | ||||||||||||||||||
|
|
||||||||||||||||||
| namespace Xamarin.ProjectTools | ||||||||||||||||||
| { | ||||||||||||||||||
|
|
@@ -35,16 +37,45 @@ public string GetAsFile (string url, string filename = "") | |||||||||||||||||
| if (File.Exists (filename)) | ||||||||||||||||||
| return filename; | ||||||||||||||||||
| // FIXME: should be clever enough to resolve name conflicts. | ||||||||||||||||||
| using (var response = httpClient.GetAsync (url).GetAwaiter ().GetResult ()) { | ||||||||||||||||||
| response.EnsureSuccessStatusCode (); | ||||||||||||||||||
| using (var fileStream = File.Create (filename)) | ||||||||||||||||||
| using (var httpStream = response.Content.ReadAsStreamAsync ().GetAwaiter ().GetResult ()) { | ||||||||||||||||||
| httpStream.CopyTo (fileStream); | ||||||||||||||||||
| try { | ||||||||||||||||||
| using (var response = httpClient.GetAsync (url).GetAwaiter ().GetResult ()) { | ||||||||||||||||||
| response.EnsureSuccessStatusCode (); | ||||||||||||||||||
| using (var fileStream = File.Create (filename)) | ||||||||||||||||||
| using (var httpStream = response.Content.ReadAsStreamAsync ().GetAwaiter ().GetResult ()) { | ||||||||||||||||||
| httpStream.CopyTo (fileStream); | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
| } catch (HttpRequestException ex) when (IsTransientError (ex)) { | ||||||||||||||||||
| TestContext.WriteLine ($"Transient network error downloading '{url}':"); | ||||||||||||||||||
| TestContext.WriteLine ($" Message: {ex.Message}"); | ||||||||||||||||||
| if (ex.StatusCode.HasValue) { | ||||||||||||||||||
| TestContext.WriteLine ($" HTTP Status Code: {(int)ex.StatusCode.Value} ({ex.StatusCode.Value})"); | ||||||||||||||||||
| } | ||||||||||||||||||
| TestContext.WriteLine ($" URL: {url}"); | ||||||||||||||||||
| TestContext.WriteLine ($" Stack Trace: {ex.StackTrace}"); | ||||||||||||||||||
| Assert.Inconclusive ($"Test skipped due to transient network error: {ex.Message}"); | ||||||||||||||||||
| } | ||||||||||||||||||
| return filename; | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| static bool IsTransientError (HttpRequestException ex) | ||||||||||||||||||
| { | ||||||||||||||||||
| // Check for timeout errors | ||||||||||||||||||
| if (ex.Message.Contains ("504") || ex.Message.Contains ("Gateway Time-out")) | ||||||||||||||||||
| return true; | ||||||||||||||||||
|
|
||||||||||||||||||
|
||||||||||||||||||
Copilot
AI
Dec 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The message-based check for "504" and "Gateway Time-out" is fragile and redundant. Line 72 already handles HttpStatusCode.GatewayTimeout (which is 504), making this string check unnecessary. String matching on exception messages is brittle since message formats can vary across different .NET versions or localization settings. Remove these lines and rely solely on the StatusCode property check.
| // Check for timeout errors | |
| if (ex.Message.Contains ("504") || ex.Message.Contains ("Gateway Time-out")) | |
| return true; | |
| // Check for other common transient errors | |
| // Check for common transient HTTP status codes |
Copilot
AI
Dec 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The multi-line return statement should follow the project's formatting style. Based on the coding guidelines, this should be formatted with each condition on its own line with proper indentation, or use a more compact style with parentheses. The current formatting with the continuation is inconsistent with typical Mono style.
| return statusCode == HttpStatusCode.RequestTimeout || | |
| statusCode == HttpStatusCode.GatewayTimeout || | |
| statusCode == HttpStatusCode.ServiceUnavailable || | |
| statusCode == HttpStatusCode.BadGateway; | |
| return statusCode == HttpStatusCode.RequestTimeout | |
| || statusCode == HttpStatusCode.GatewayTimeout | |
| || statusCode == HttpStatusCode.ServiceUnavailable | |
| || statusCode == HttpStatusCode.BadGateway; |
Copilot
AI
Dec 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the trailing whitespace on this empty line to maintain code consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, this actually needs to
File.Delete(filename)...Probably also need a
try-catch-throwthat does the same thing for all uncaught exceptions.