gzhao9 opened a new pull request, #16092: URL: https://github.com/apache/druid/pull/16092
<!-- Thanks for trying to help us make Apache Druid be the best it can be! Please fill out as much of the following information as is possible (where relevant, and remove it when irrelevant) to help make the intention and scope of this PR clear in order to ease review. --> <!-- Please read the doc for contribution (https://github.com/apache/druid/blob/master/CONTRIBUTING.md) before making this PR. Also, once you open a PR, please _avoid using force pushes and rebasing_ since these make it difficult for reviewers to see what you've changed in response to their reviews. See [the 'If your pull request shows conflicts with master' section](https://github.com/apache/druid/blob/master/CONTRIBUTING.md#if-your-pull-request-shows-conflicts-with-master) for more details. --> Fixes Topics we discuss in Draft [PR](https://github.com/gzhao9/druid/pull/2) ### Description #### Refactored AsyncQueryForwardingServletTest to Reduce Code Duplication In an effort to enhance code maintainability and readability, I have refactored the `AsyncQueryForwardingServletTest` class by introducing a new private method named `getArgumentCaptor`. This method centralizes the duplicated logic previously spread across six different test cases, reducing code repetition and improving test clarity. `getArgumentCaptor` Like the following. ```java 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); } else { servlet.handleException(response, mockMapper, testException); } ArgumentCaptor<Exception> captor = ArgumentCaptor.forClass(Exception.class); Mockito.verify(mockMapper).writeValue(ArgumentMatchers.eq(outputStream), captor.capture()); return captor; } ``` This way we only have to focus on whether or not the request was created. Before calling this method, the test case looks like this. ```java @Test public void testHandleQueryParseExceptionWithFilterDisabled() throws Exception { String errorMessage = "test exception message"; ObjectMapper mockMapper = Mockito.mock(ObjectMapper.class); HttpServletRequest request = Mockito.mock(HttpServletRequest.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(), new ServerConfig() ); Mockito.when(request.getAttribute(AuthConfig.DRUID_AUTHENTICATION_RESULT)).thenReturn(new AuthenticationResult("userA", "basic", "basic", null)); IOException testException = new IOException(errorMessage); servlet.handleQueryParseException(request, response, mockMapper, testException, false); ArgumentCaptor<Exception> captor = ArgumentCaptor.forClass(Exception.class); Mockito.verify(mockMapper).writeValue(ArgumentMatchers.eq(outputStream), captor.capture()); Assert.assertTrue(captor.getValue() instanceof QueryException); Assert.assertEquals("Unknown exception", ((QueryException) captor.getValue()).getErrorCode()); Assert.assertEquals(errorMessage, captor.getValue().getMessage()); Assert.assertEquals(IOException.class.getName(), ((QueryException) captor.getValue()).getErrorClass()); } ``` With modifications, the test looks like this. ```java @Test public void testHandleQueryParseExceptionWithFilterDisabled() throws Exception { String errorMessage = "test exception message"; boolean haveRequest=true; ServerConfig serverConfig= new ServerConfig(); ArgumentCaptor<Exception> captor=getArgumentCaptor(serverConfig,errorMessage,haveRequest); Assert.assertTrue(captor.getValue() instanceof QueryException); Assert.assertEquals("Unknown exception", ((QueryException) captor.getValue()).getErrorCode()); Assert.assertEquals(errorMessage, captor.getValue().getMessage()); Assert.assertEquals(IOException.class.getName(), ((QueryException) captor.getValue()).getErrorClass()); } ``` This change appears in six test cases, which can make each test case more concise and easy to read. -- 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]
