Fix http tests #1451
Fix http tests #1451
Conversation
mlaily
changed the title
tests/FSharp.Data.Tests/Http.fs
Outdated
| let localServer = startHttpLocalServer() | ||
| using localServer <| fun _ -> |
There was a problem hiding this comment.
Any reason not to do this?
use localServer = startHttplocalServer()
//...There was a problem hiding this comment.
I started with an implementation where the IDisposable returned by startHttpLocalServer() didn't contain anything useful beside Dispose() and I was afraid Dispose() would be called too early if the value wasn't otherwise used. (I'm not sure when it goes out of scope in this situation)
This is less true now that the BaseAddress is used to create the http request.
Do you want me to change it? (and if you know about when Dispose() is called with seemingly unused use bindings, I'd be interested to know :))
There was a problem hiding this comment.
It will only get called when the whole function is out of scope, see here: https://sharplab.io/#v2:DYLgZgzgPg9gDgUwHYAIDKBPCAXBBbAWACh5l0tc8A6AFQAsAnBAQwBMBLJAc1uYgGsIxYtgyIUASRoIcACWzY4aBAwBuKlAF5iKXSk50V7bJIAi7CHBgRmAI2AIdeuzgbMAxibz5bGgEJ8CACCrKxMEBAgKK6cXE66LthunijeeL4MKADqMAz8KjR8/FGFAsJEDia2zAxgKAAUAJRa8SgA3ihICADuktJyCkoq6pndxnSteqk+Gth0FlTmltYITS1EU5t6cAyc2GCoAEQYCAjYh5N6aRkocws5eQVFWiil/FQASgCuSPVgPygAPooAC0AD4Go1GpddNdZvMIFQAhBgqFwhAXsdTucUABfVrESooOprbQbPRfFEoOh4PAvaq1SEE8m6HZ7A4oQ7dOjMXAjC4slBspD7I7MJCsW4IgVTYWizniyV3HAwGCHIA
There was a problem hiding this comment.
Oh ok! So nothing too fancy going on. Thanks for the demo! 🙏
I should use sharplab more often...
(I'll update the commit)
tests/FSharp.Data.Tests/Http.fs
Outdated
| let exc = Assert.Throws<WebException> (fun () -> | ||
| Http.Request(localServer.BaseAddress + "/200?sleep=1000", customizeHttpRequest = (fun req -> req.Timeout <- 1; req)) |> ignore) | ||
|
|
||
| Assert.That(exc.Status, Is.EqualTo(WebExceptionStatus.Timeout)) |
There was a problem hiding this comment.
Any reason why Assert.AreEqual can't still be used?
There was a problem hiding this comment.
Not really. I find this syntax more easy to read, that's all (There is less ambiguity which value is the "expected" value and which one is the "actual" value).
I can change it to Assert.AreEqual if you feel like consistency with the rest of the tests is more important.
There was a problem hiding this comment.
Yes, please change it so that it's consistent with the rest.
There was a problem hiding this comment.
Ok. (actually, the rest of the file uses ... |> should equal ... so I'm going to do the same)
I believe there is a race condition between the http stack timeout and the async timeout. If the http stack timeout is triggered first, the inner exception is null. In both cases, the WebException status should indicate a timeout.
|
Thank you too! |
The CI for #1447 already failed two times, so let's see if this PR improves the situation...
I initially thought the problem was simply that using an external (internet) endpoint for the tests was unreliable, but the tests were still failing a bit randomly after replacing the remote endpoints with a local http test server.
After investigation, I believe there is a race condition between the http stack timeout and the
Async.StartChildtimeout. (ingetResponseinHttp.fs)If the http stack timeout is triggered first, the inner exception is null. Since the tests were asserting that the inner exception should be a
TimeoutException, they were failing in this case.In all cases, the
WebExceptionstatus should indicate a timeout, so I changed the assertion accordingly...