[Merge] lp:~gary-wzl77/net-cpp/fix_1631846 into lp:net-cpp

[James Henstridge]

Review: Needs Fixing

I'm not really comfortable with the test changes.  See inline comments.

Diff comments:

> 
> === modified file 'tests/http_client_test.cpp'
> --- tests/http_client_test.cpp	2016-08-11 15:06:23 +0000
> +++ tests/http_client_test.cpp	2016-11-02 04:28:36 +0000
> @@ -110,7 +110,7 @@
>      EXPECT_THROW(auto response = request->execute(default_progress_reporter), core::net::Error);
>  }
>  
> -TEST(HttpClient, DISABLED_get_request_against_app_store_succeeds)
> +TEST(HttpClient, get_request_against_app_store_succeeds)

>From the look of it, this test was disabled because it is hitting a remote site.  This means that the test could fail if the test suite is run on a host without Internet access, or if the remote server starts producing different results.  It seems unwise to re-enable it in its current form.

I would suggest either adding a new test case for this feature (running against the testbin server), or extending one of the tests that is currently enabled.

>  {
>      auto client = http::make_client();
>  
> @@ -126,6 +126,8 @@
>  
>      response.header.enumerate([](const std::string& key, const std::set<std::string>& values)
>      {
> +        //check if the headers are parsed properly.
> +        EXPECT_TRUE(key.find(":") == std::string::npos);

This is a fairly weak assertion.  While checking that the key doesn't include a colon does fail without your changes and pass with them, what we really want to check is that "the response headers are correctly parsed".

So I'd expect a test that performs a request, and then checks that an expected response header is present and that it has the expected value.  You could check multiple expected response headers, but I wouldn't bother checking them all.

>          for (const auto& value : values)
>              std::cout << key << " -> " <<  value << std::endl;
>      });


-- 
https://code.launchpad.net/~gary-wzl77/net-cpp/fix_1631846/+merge/308017
Your team Ubuntu Phablet Team is subscribed to branch lp:net-cpp.