kfaraz commented on code in PR #16092:
URL: https://github.com/apache/druid/pull/16092#discussion_r1518726183
##########
services/src/test/java/org/apache/druid/server/AsyncQueryForwardingServletTest.java:
##########
@@ -1063,4 +942,41 @@ public int getPort()
return port;
}
}
+ private ArgumentCaptor<Exception> getArgumentCaptor(ServerConfig
serverConfig, String errorMessage, boolean haverequest) throws IOException
Review Comment:
I like the idea of returning an `ArgumentCaptor` here rather than doing all
the assertions in the private method. I would still prefer passing the lambda
to denote which method of `AsyncQueryForwardingServlet ` is being invoked. It
would essentially look similar to the JUnit `Assert.assertThrows`.
##########
services/src/test/java/org/apache/druid/server/AsyncQueryForwardingServletTest.java:
##########
@@ -1063,4 +942,41 @@ public int getPort()
return port;
}
}
+ private ArgumentCaptor<Exception> getArgumentCaptor(ServerConfig
serverConfig, String errorMessage, boolean haverequest) throws IOException
+ {
+ ObjectMapper mockMapper = Mockito.mock(ObjectMapper.class);
+ HttpServletResponse response = Mockito.mock(HttpServletResponse.class);
+ ServletOutputStream outputStream = Mockito.mock(ServletOutputStream.class);
+ Mockito.when(response.getOutputStream()).thenReturn(outputStream);
+ final AsyncQueryForwardingServlet servlet = new
AsyncQueryForwardingServlet(
+ new MapQueryToolChestWarehouse(ImmutableMap.of()),
+ mockMapper,
+ TestHelper.makeSmileMapper(),
+ null,
+ null,
+ null,
+ new NoopServiceEmitter(),
+ new NoopRequestLogger(),
+ new DefaultGenericQueryMetricsFactory(),
+ new AuthenticatorMapper(ImmutableMap.of()),
+ new Properties(),
+ serverConfig
+ );
+ Exception testException = new IllegalStateException(errorMessage);
+
+ if (haverequest) {
Review Comment:
This boolean `haverequest` is too vague. If you using the lambda style, this
flag won't be needed.
##########
services/src/test/java/org/apache/druid/server/AsyncQueryForwardingServletTest.java:
##########
@@ -1063,4 +942,41 @@ public int getPort()
return port;
}
}
+ private ArgumentCaptor<Exception> getArgumentCaptor(ServerConfig
serverConfig, String errorMessage, boolean haverequest) throws IOException
+ {
+ ObjectMapper mockMapper = Mockito.mock(ObjectMapper.class);
+ HttpServletResponse response = Mockito.mock(HttpServletResponse.class);
+ ServletOutputStream outputStream = Mockito.mock(ServletOutputStream.class);
+ Mockito.when(response.getOutputStream()).thenReturn(outputStream);
+ final AsyncQueryForwardingServlet servlet = new
AsyncQueryForwardingServlet(
+ new MapQueryToolChestWarehouse(ImmutableMap.of()),
+ mockMapper,
+ TestHelper.makeSmileMapper(),
+ null,
+ null,
+ null,
+ new NoopServiceEmitter(),
+ new NoopRequestLogger(),
+ new DefaultGenericQueryMetricsFactory(),
+ new AuthenticatorMapper(ImmutableMap.of()),
+ new Properties(),
+ serverConfig
+ );
+ Exception testException = new IllegalStateException(errorMessage);
+
+ if (haverequest) {
+ HttpServletRequest request = Mockito.mock(HttpServletRequest.class);
+
Mockito.when(request.getAttribute(AuthConfig.DRUID_AUTHENTICATION_RESULT)).thenReturn(new
AuthenticationResult("userA", "basic", "basic", null));
+ IOException testException2 = new IOException(errorMessage);
+ servlet.handleQueryParseException(request, response, mockMapper,
testException2, false);
Review Comment:
I would prefer using the originally suggested lambda style.
While reading the main test, it should be evident which method of
`AsyncQueryForwardingServlet` is being called.
Also, the exception being handled should be _passed_ to this method rather
than being constructed here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
