Why are the things mentioned so far not sufficient? ReadAll returns a
non-nil error, if the reader you are passing to it returns a non-nil,
non-EOF error. In this case, you are passing Response.Body
<https://godoc.org/net/http#Response.Body>, which does not make any
guarantees regarding the errors it returns, so you should just treat the
error returned as opaque (you can't reliably check the error conditions
exhaustively anyway). Things that might go wrong include (but are not
limited to) anything that might go wrong when reading from a network
connection, any error resulting from a broken encoded stream that the
client transparently decodes (like gzip encoding, TLS…)…


On Wed, Feb 14, 2018 at 10:50 AM mrx <patrik....@gmail.com> wrote:

>
> On Wed, Feb 14, 2018 at 10:30 AM, Axel Wagner <
> axel.wagner...@googlemail.com> wrote:
>
>>
>> On Wed, Feb 14, 2018 at 10:01 AM mrx <patrik....@gmail.com> wrote:
>>
>>>
>>> On Tue, Feb 13, 2018 at 5:26 PM, Axel Wagner <
>>> axel.wagner...@googlemail.com> wrote:
>>>
>>>> Not very, but it does depend on the details. For example, you might
>>>> provide your own http.Transport for the client to use, or even your own
>>>> net.Conn.
>>>>
>>>
>>> Using ioutils.ReadAll() on a HTTP request means to me to read out the
>>> response's body. I cannot see how http.Transport and net.Conn would have
>>> anything to do with this.
>>>
>>
>> Presumably, to read out a response body means, that you made a request.
>> By passing in a custom http.Transport, you can have that transport return a
>> non-EOF error at will in tests.
>>
>> It would be far more helpful, if there would be actual code here.
>>
>
> Sure thing, taken from the http docs:
>
> resp, err := http.Get("http://example.com/";)
> if err != nil {
>       // handle error
> }
> defer resp.Body.Close()
> body, err := ioutil.ReadAll(resp.Body)
>
>
> That err returned from ReadAll. I cannot see how that can possibly fail.
>
>
>> You might have the server stop sending any data, so eventually the
>>>> connection will timeout.
>>>>
>>>
>>> So what you're saying is that unless the response contain chunked data,
>>> ioutil.ReadAll() will never fail?
>>>
>>
>> I say that ioutil.ReadAll will fail if and only if the io.Reader it's
>> called with returns a non-EOF, non-nil error on some read. That, at least,
>> seems the most obvious semantics of that function to me and it's how I read
>> its documentation:
>>
>> > ReadAll reads from r until an error or EOF and returns the data it
>> read. A successful call returns err == nil, not err == EOF.
>>
>
> Sure, that make sense. When testing for err i try to imagine what could go
> wrong, to get the error handling correct. But in this case (code from http
> docs above) i cannot see it...
>
>
>>
>> The question is, though, why would you want that?
>>>>
>>>
>>> As ioutil.ReadAll does return an error in its signature, i think it's
>>> good form to test it. Don't you?
>>>
>>
>> No, I don't think I do, actually :) Like, what failure modes is this
>> testing, how often will they occur, how likely is it, that a future change
>> would break the behavior? I'm just having trouble coming up with a bug that
>> might be introduced in the future, that could be caught by testing this.
>>
>> Checking every branch seems to be an incarnation of the "we need 100%
>> line coverage" testing philosophy, but honestly, I don't believe that's
>> particularly helpful, because a) path coverage is much more important than
>> line coverage
>>
>
> I'm not after a 100% line coverage. =)
>
> Is there a way to get branch/path coverage in Go? I cannot find anything
> like that in the help from go test.
>
>
>> and b) 100% is unattainable in practice. Adhering to some kind of
>> mechanical rule about what code must have test coverage will just lead to
>> wasted time - if there are zero future bugs with or without that test, it
>> definitionally was useless. And I'd say that the likelihood that this is
>> going to catch a future bug is very low. So if, with very high probability,
>> writing that test is a waste of time then that seems a good enough reason
>> not to write it.
>>
>
> I agree, i'm just trying to get the error handling correct. That's really
> difficult if i cannot even imagine what could go wrong.
>
>
>>
>> (unless, of course, you are trying to write tests *for ReadAll itself*,
>> in which case checking the whole API contract makes of course sense. But I
>> assume you're not :) )
>>
>
> You're correct, i'm not writing tests for ioutil.ReadAll()
>
>
>>
>> Is that actually a path that is worth testing? Personally, I kind of
>>>> doubt it.
>>>>
>>>
>>> That's kind of it really, i am having a hard time making up my mind
>>> here. That's why i come for the golang nuts wisdom.
>>>
>>>
>>>> You'll probably get more bang for your buck, if you instead send back
>>>> broken/incomplete data from the server and see if the client handles that
>>>> correctly.
>>>>
>>>
>>> I already test for this kind of problems in my unittests. It's more a
>>> matter of what to do with the error return value from ioutil.ReadAll() when
>>> i cannot see how i could ever get anything but err == nil. It might just be
>>> me, that doesn't know enough about the ioutil.ReadAll() internals.
>>>
>>
>> After digging some into the code, it does inform a sensible additional
>> test you might be doing, as it also returns an error if the reader returns 
>> *too
>> much* data - so it would make sense to check what happens if the server
>> would send back, say, an infinite bytestream. But that still isn't really a
>> test for the ReadAll error return, as a check for "does this function
>> behave well if fed unreasonable data".
>>
>> And to answer the question of what to do about err != nil: You should
>> handle it :) It means the data the server sent was broken in some way, you
>> should handle that in some way that is appropriate for the application you
>> are writing - which might include logging it, ignoring it, aborting,
>> retrying or anything else, really :) Presumably, you are also handling
>> invalid data somehow - this shouldn't be any different.
>>
>
> Handling those errors still mean that i need to have at least an idea or
> two on what could happen, no?
>
> // Patrik
>

-- 
You received this message because you are subscribed to the Google Groups 
"golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to golang-nuts+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Reply via email to