Changes to HTTPMetricHandler to fix Issue #732

[dhoard]

Changes to HTTPServer and HTTPMetricHandler to resolve getting Content-Length=0 when Transfer-Encoding=chunked. Refactored TestHTTPTest for cleaner testing.

Signed-off-by: Doug Hoard doug.hoard@gmail.com

[fstab]

Thaks a lot for the PR! I like the idea of refactoring the request() methods to make the test clearer.

However, it would be good if after the refactoring the test had less lines of code, because the new HttpRequest / HttpResponse classes allowed expressing the test more concisely. Currently, the test grew from 576 lines to 724 lines even though all the request() methods are removed. With a bit of refactoring we could reduce the boiler plate:

Assert.assertTrue(
        (httpResponse.getHeader("content-length") == null)
        ^ (httpResponse.getHeader("transfer-encoding") == null));

This is copied 16 times. It would be good to have one test to make sure that the content-length and transfer-encoding headers are correct, and not to copy-and-paste this to all tests.

HttpRequest httpRequest = new HttpRequest.Builder()
        .withURL("http://localhost:" + httpServer.getPort() + "/metrics?x")
        .build();

HttpResponse httpResponse = httpRequest.execute();
String body = httpRequest.execute().getBody();

The relevant part here is the ?x URL parameter. This was easier to see in the previous single line String body = request(s, "?x").body;. Maybe we can add a helper method request(HTTPServer, String) that returns the initialized Builder? Then we could cover the simple case with request(httpServer, "?x").build().execute().getBody(). To reduce this even further, we could just move the Builder methods directly into the HttpRequest, then you could write request(httpServer, "?x").execute().getBody().

For more complex tests we could still write something like

request(httpServer, "?x")
    .withHeader(...)
    .withAuthorization(...)
    .execute().getBody();

[dhoard]

@fstab I have refactored the code to mostly align with your suggestions.

[fstab]

It's looking great, thanks a lot! I added two little nitpick remarks, apart from that it's ready to be merged.

[fstab]

Forgot to push?

[dhoard]

Forgot to push?

Yep. pushed. Apologies.

[fstab]

Thanks a lot!