[PATCH] D111252: [llvm] [Support] [Debuginfo] Add http and debuginfod client libraries and llvm-debuginfod-find tool

[Noah Shutty via Phabricator via llvm-commits]

noajshu added inline comments.


================
Comment at: llvm/unittests/Support/HTTPClient.cpp:15
+
+TEST(HTTPClientTests, invalidUrlTest) {
+  std::string invalidUrl = "llvm is fun";
----------------
labath wrote:
> noajshu wrote:
> > labath wrote:
> > > phosek wrote:
> > > > Ditto for this one.
> > > This would be better done as `EXPECT_THAT_EXPECTED(httpGet(...), Failed<StringError>())` or even `FailedWithMessage("whatever")`, though I agree that this is not very useful.
> > > 
> > > I think the interesting question is whether we're ok with not having any coverage for the lower level apis, and I'm not really sure what the answer to that is.
> > Thanks, updated!
> > I agree with the comments that these unit tests are not adding much value. One option until we have a server in llvm would be to GET http://llvm.org, although that does require an internet connection. By the lower level APIs, are you referring to the static callback functions used to implement `httpGet`, or to the CURL library itself?
> I mean the `httpGet` function itself -- we generally don't test private implementation details (directly).
> In an ideal world I'd split this patch into two (or even three, with the first part being the introduction of httpGet), and each would come with it's own tests. Testing the error message is nice to have, but it just scratches the surface. In httpGet, the content-length handling seems interesting to test, for example. 
> 
> But yes, you'd need some way to create/mock the server connection for that to work...
Hi Pavel, first off thanks for all of your comments. I refactored the HTTP client  to enable meaningful unit tests. It's now split into two new class hierarchies rooted at `HTTPRequest` and `HTTPResponseHandler`. This way CURL can be swapped out with a a different HTTP backend, including a simulated backend during unit testing.
Similarly, the buffered response handler can be unit tested in isolation without data going through a socket. This allows some nontrivial tests of how it handles content-lengths.
I am very interested in your thoughts on this refactor, and if you have ideas for other unit tests of the HTTP client or feedback on the ones here. I wonder if it makes sense to refactor the Debuginfod client library as well to enable more meaningful unit tests there. And please feel free to suggest a different approach if you feel the refactor moves us in the wrong direction.

For now I will work on addressing your comments on the other areas of the diff.
Since it is even larger now, it could certainly make sense to split off into a few diffs. One way to do this could be:

[diff 0 ] HTTP Client + unit tests
[diff 1]  Debuginfod Client library + unit tests
[diff 2] Debuginfod-Find tool + end-to-end (lit) tests


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111252/new/

https://reviews.llvm.org/D111252