[PATCH] D112751: [llvm] [Support] Add HTTP Client Support library.

[Pavel Labath via Phabricator via llvm-commits]

labath added a comment.

This looks good to me now, modulo some details in inline comments.



================
Comment at: llvm/lib/Support/HTTPClient.cpp:29-31
+HTTPResponseHandler::~HTTPResponseHandler() {}
+
+HTTPClient::~HTTPClient() {}
----------------
`= default;`


================
Comment at: llvm/unittests/Support/HTTPClient.cpp:17
+TEST(HTTPClientTests, bufferedHTTPResponseHandlerLifecycleTest) {
+  {
+    BufferedHTTPResponseHandler Handler;
----------------
This block looks weird -- I'd just make a separate test for that.


================
Comment at: llvm/unittests/Support/HTTPClient.cpp:42-45
+  EXPECT_EQ(memcmp(Handler.ResponseBuffer.Body->getBufferStart(),
+                   "body:this puts the total at 36 chars",
+                   Handler.ResponseBuffer.Body->getBufferSize()),
+            0);
----------------
noajshu wrote:
> labath wrote:
> > EXPECT_EQ(Handler.ResponseBuffer.Body->getBuffer(), "...");
> Thanks! Because the Body is actually a WritableMemoryBuffer, I need an extra namespace qualifier on getBuffer for this to work.
> 
> On that note -- do you think I should change the HTTP response body to be a MemoryBuffer (read-only)?
> 
I don't see a need for that -- I can see how the caller might want to modify the buffer for its own purposes. and he can always convert it to a read-only buffer if he does not (the opposite is harder).


================
Comment at: llvm/unittests/Support/HTTPClient.cpp:157-162
+  MockHTTPResponseHandler Handler;
+  MockHTTPClient Client;
+  HTTPRequest Request("");
+  EXPECT_THAT_ERROR(Client.perform(Request, Handler), Succeeded());
+  EXPECT_EQ(Handler.HeaderLinesTotal, 1u);
+  EXPECT_EQ(Handler.BodyBytesTotal, 7u);
----------------
noajshu wrote:
> labath wrote:
> > What is this supposed to test? This test pretty much only executes code which is defined in the test itself...
> I agree and I will remove the mockHTTPClientTest.
> 
> Do you think there is any value in the BrokenConnection test? I would be happy to remove this test as well. The only non-test code executed in that one is `Client.perform(Request)` and `Client.get("")` which is effectively a change-detector for the short convenience methods of the Client.
Yeah, I'd probably remove them, or at least significantly simplify them. If you want to test that the virtual `perform` is getting called, then all you need to do is mock that method -- you don't really need to provide a fake implementation of that method, and everything that gets called by it...


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

https://reviews.llvm.org/D112751