[Tests] If we have server errors. Mark test as inconclusive. - #7401
Conversation
If we are getting errors (500,401..) do not mark a link all test as a failure, but as inconclusive. Fixes: https://github.com/xamarin/maccore/issues/2056
|
Build failure Test results16 tests failed, 70 tests passed.Failed tests
|
|
All failures are https://github.com/xamarin/maccore/issues/581 |
| } else { | ||
| string content = await response.Content.ReadAsStringAsync (); | ||
| waited = true; | ||
| bool success = !String.IsNullOrEmpty (content); |
There was a problem hiding this comment.
tiny nit: it's a little confusing to have a whole bool success variable that we expect to be true always.
In the context of http request code, I would expect a variable named success to indicate something like response.IsSuccessStatusCode, but it seems like by the time we hit the else branch we're already in the success case :)
Otherwise, looks good to me.
There was a problem hiding this comment.
good point. I think we are just testing that async/await works. If that is the case, even the errno would be good.
@rolfbjarne do you know why do we have this tests exactly? I looked at the bugzilla issue and might just be for the async support.
There was a problem hiding this comment.
The test was added here: https://github.com/xamarin/maccore/commit/ca7dd9294d173893fbb2b635175141797261ead8, and yes, it looks like it was just to make sure async/await works.
But did you try just changing the url to http://example.com? That's a url we're already using in other places, and which will fail if the url goes down. It's possible http://msdn.microsoft.com will always return us a 500 code, and it doesn't feel right to ignore any such failures (eventually we'll end up ignoring legitimate failures).
There was a problem hiding this comment.
make sense to reduce the number of endpoint our tests depends on... maybe we should have them as constants (in a different PR)
| } else { | ||
| string content = await response.Content.ReadAsStringAsync (); | ||
| waited = true; | ||
| bool success = !String.IsNullOrEmpty (content); |
There was a problem hiding this comment.
make sense to reduce the number of endpoint our tests depends on... maybe we should have them as constants (in a different PR)
|
@spouliot @rolfbjarne I'll update the url, seems like msd is giving a lot of problems. Ignoring anyway seems good, since inconclusive in this case is what we want. BUT! Maybe just the HttpResponse with the errno is good enough to test the async pattern, and in this particular case we do not care about the content of the response. |
|
Build success |
|
@monojenkins backport xcode11.2 |
|
@monojenkins backport xcode11.3 |
|
@monojenkins backport d16-5 |
|
@monojenkins backport d16-4 |
5 participants
If we are getting errors (500,401..) do not mark a link all test as a
failure, but as inconclusive.
Fixes: https://github.com/xamarin/maccore/issues/2056