Added tests for getDownloadSize
#19
Conversation
There was a problem hiding this comment.
Hi @devils2ndself — many many thanks for joining us in such an early stage : ) This make me so happy 💜
Also, thank you for the quality PR! I added minor inline comments if you wanna change them. Also, if you're changing anything, feel free to rebase and squash your PRs (otherwise, I'll do it on merge here).
Let me know if you need more time, or if we merge that one as it is and follow up with new changes in new PRs.
main_test.go
Outdated
| d.client = &http.Client{Timeout: d.TimeoutPerChunk} | ||
| got, err := d.getDownloadSize(context.Background(), "test") |
There was a problem hiding this comment.
Can we make this test more realistic (or add yet a fourth test case)?
Maybe a response with content-length set to 0 and no content-range header…?
There was a problem hiding this comment.
Yeah, good catch. However, not having Content-Range and ContentLength as 0 breaks the function, returning 18446744073709551615 and no error. Would you like me to add a sentinel to the function when no Content is presented?
There was a problem hiding this comment.
Ouch… another bug!
Yes, if you can find a workaround, that would be great. Re-reading it, it looks like this logic is deceiving:
Line 150 in a27be85
Maybe we just check resp.ContentLength <= 0 and deal with the other header inside of the if block to make it clearer and less error-prone.
Otherwise, feel free to leave the new test with the assertion commented with a // TODO: fix (or similar).
There was a problem hiding this comment.
Okay, so I encountered another bug... setting a sentinel for checking if both ContentLength and Content-Range are null breaks another test which expects an error in a different place... Is it okay if I remove the sentinel and leave this one for another issue?
There was a problem hiding this comment.
Is it okay if I remove the sentinel and leave this one for another issue?
Sure think! Just leave the test you wrote commenting the t.Errorf(…) with a // TODO: … so we don't forget about it : )
Many many thanks 💜
There was a problem hiding this comment.
Plus, it might make more sense to return 0 instead of an error if ContentLegnth is 0 and there is no Content-Range header, and then checking if download size is 0. I will leave that for you to decide
There was a problem hiding this comment.
it might make more sense to return 0 instead of an error if ContentLegnth is 0
I don't think so. The only use case we have for getting the size is to know how many content-range requests to spin. If we don't know that, the whole process is broken.
In the future (stretch goal), we can capture cases like that and set a regular download (not using a content-range header), but for that, any error on getDownloadSize would do it.
devils2ndself
force-pushed
the
issue-18
branch
2 times, most recently
from
f3ab005
to
a815397
Compare
* added tests for getDownloadSize * addressed requested changes * client assignment on DefaultDownloader() and NoContent test * added a TODO for future no-content error-handling
|
Beautifully done ✨ Thank you so much. |
devils2ndself
force-pushed
the
issue-18
branch
from
a815397
to
2c8fef3
Compare
|
just pushed a TODO for future contributers, but I think that's it for the issue |
Closes #18
I have added 3 unit tests for
getDownloadSize: one using ContentLength, one using Content-Range header, and one as a failure.