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

[Pavel Labath via Phabricator via llvm-commits]

labath added a comment.

Well... I hate to say it, but I found the version very easy to get lost in. It seems to me you're exposing a lot more implementation details than it is necessary (including the entire curl.h header, which is something we try to avoid <https://llvm.org/docs/SupportLibrary.html#don-t-expose-system-headers>. And the fact that tests don't even go through the public interface make it hard to keep track of what is being tested. If we are going to do this, maybe we could use a pattern like this:

  class CurlInterface {
    virtual Error init()  = 0;
    virtual Error cleanup() = 0;
    virtual Error perform() = 0;
    virtual void setWriteFunction(...) = 0;
    // etc. 
    // It doesn't have to match the curl interface verbatim. Feel free 
    // to perform basic simplifications and c++-ifications of the 
    // interface. You'll need to do some of those to avoid dependence 
    // on curl.h anyway.
  };
  
  Expected<HTTPResponseBuffer> httpGet(const Twine &Url, CurlInterface &curl);
  Expected<HTTPResponseBuffer> httpGet(const Twine &Url); // calls the first function with the default/real curl implementation

This shouldn't expose too much implementation details, and this interface could even conceivably by useful by someone who wants to use a custom http-ish library/transport/thingy.

Then, in the test, you could use gmock to program a fake curl implementation:

  class CurlMock: public CurlInterface {
    MOCK_METHOD0(init, Error());
    ...
  };
  
  TEST(httpGet, whatever) {
    CurlMock curl;
    EXPECT_CALL(curl, setWriteFunction).WillOnce(SaveArg<0>(&writeFunc));
    EXPECT_CALL(curl, perform).WillOnce([&] {
      EXPECT_THAT_ERROR(writeFunc(...), llvm::Succeeded());
      EXPECT_THAT_ERROR(writeFunc(...), llvm::Failed());
      ...
      return llvm::Error::success();
    });
    EXPECT_THAT_ERROR(httpGet("http://foo", curl), llvm::Succeeded())
  }

How does that sound?



================
Comment at: llvm/lib/Support/HTTPClient.cpp:68
+
+size_t llvm::CurlHTTPRequest::curlHandleHeaderLine(char *Contents, size_t Size,
+                                                   size_t NMemb,
----------------
the llvm:: qualifications are not necessary here


================
Comment at: llvm/lib/Support/HTTPClient.cpp:138
+
+  curlSetOpt(CURLOPT_URL, Config.Url.str().str().c_str());
+  curlSetOpt(CURLOPT_FOLLOWLOCATION, Config.FollowRedirects);
----------------
drop .str().str(), it's cleaner


================
Comment at: llvm/unittests/Support/HTTPClient.cpp:15
+
+TEST(HTTPClientTests, invalidUrlTest) {
+  std::string invalidUrl = "llvm is fun";
----------------
noajshu wrote:
> 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
Yeah, if we're going to have tests for each of these components, I'd definitely recommend splitting them out into separate patches.


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

https://reviews.llvm.org/D111252