Increase code coverage for DirectDruidClientTest
#18861
Conversation
Replaces mocks with more concrete test helpers that will invoke and exercise the internals of DirectDruidClient response handler, etc. Also, add a new unit test for ResourceExceededException that wasn't previously exercised.
jtuglu1
left a comment
jtuglu1
left a comment
There was a problem hiding this comment.
LGTM – once the minor comments are handled.
| return initHttpClientWithFuture(new TestHttpClient(objectMapper, future), false); | ||
| } | ||
|
|
||
| private HttpClient initHttpClientWithFuture(TestHttpClient httpClient, boolean throwQueryError) |
There was a problem hiding this comment.
can we name this something different? It has the same name as the above method but different signature. Maybe something like initHttpClientFromExistingClient.
| } | ||
|
|
||
| @Test | ||
| public void testQueryTimeoutBeforeFuture() throws IOException, InterruptedException |
There was a problem hiding this comment.
Removing the mocks I believe removes InterruptedException – let's clean that up too while we're here.
Come to think of it, I'm not quite sure why a linter wouldn't catch this. We should add this as a rule.
There was a problem hiding this comment.
Removed InterruptedException from this test method.
Come to think of it, I'm not quite sure why a linter wouldn't catch this. We should add this as a rule.
hmm, I vaguely recall that we did have IntelliJ checks to catch this kind of issue, but they were removed at some point. I'm not sure if pmc/checkstyle can be configured to catch this.
| .andReturn(SettableFuture.create()) | ||
| .atLeastOnce(); | ||
| // A simple HttpClient that records requests and returns queued futures per call. | ||
| class SequencedHttpClient implements HttpClient |
There was a problem hiding this comment.
Can we move this to member class? This might be helpful for other tests.
There was a problem hiding this comment.
Sure, I’ve moved it to a separate test class under the same root as TestHttpClient.
2 participants
Test-only change:
HttpResponseHandlerto be exercised.This PR has: