google / guice

Guice (pronounced 'juice') is a lightweight dependency injection framework for Java 11 and above, brought to you by Google.
https://github.com/google/guice
Apache License 2.0
12.51k stars 1.67k forks source link

Refactor to Eliminate Repetitive Mock Object Creation in `ServletPipelineRequestDispatcherTest` #1825

Open gzhao9 opened 5 months ago

gzhao9 commented 5 months ago

Hi there!

While working with the ServletPipelineRequestDispatcherTest, I've noticed that there are four mock variables repeatedly created across various tests. To simplify the code, I propose a small refactor to eliminate these redundancies, which could reduce the code by 60 lines.

Here's a summary of the repetitive mock creations:

For instance, creating a mock for HttpServletRequest currently looks like this:

final HttpServletRequest mockRequest = mock(HttpServletRequest.class);
when(mockRequest.getScheme()).thenReturn("https");
when(mockRequest.getServerName()).thenReturn("the.server");
when(mockRequest.getServerPort()).thenReturn(443);

To make this process more efficient, we can introduce a createMockRequest method:

public final HttpServletRequest createMockRequest(String scheme, int serverPort) {
    HttpServletRequest mockRequest = mock(HttpServletRequest.class);
    when(mockRequest.getScheme()).thenReturn(scheme);
    when(mockRequest.getServerName()).thenReturn("the.server");
    when(mockRequest.getServerPort()).thenReturn(serverPort);
    return mockRequest;
}

With this method, creating a mock HttpServletRequest becomes:

final HttpServletRequest mockRequest = createMockRequest("https", 443);

Similarly, for the mock creation of Binding<HttpServlet>, Binding<ServletDefinition> and Injector We can introduce methods for creating mocks for these classes as well:

mock Binding<HttpServlet> creation

public final Binding<HttpServlet> createMockBinding() {
    final Binding<HttpServlet> binding = mock(Binding.class);
    when(binding.acceptScopingVisitor(any())).thenReturn(true);
    return binding;
}

mock Binding<ServletDefinition> creation

public final Binding<ServletDefinition> createMockBinding(ServletDefinition servletDefinition) {
    Provider<ServletDefinition> bindingProvider = Providers.of(servletDefinition);
    Binding<ServletDefinition> mockBinding = mock(Binding.class);
    when(mockBinding.getProvider()).thenReturn(bindingProvider);
    return mockBinding;
}

mock Injector creation

public final Injector createMockInjector(Binding<HttpServlet> binding, HttpServlet mockServlet, Binding<ServletDefinition> mockBinding) {
    final Key<ServletDefinition> servletDefsKey = Key.get(TypeLiteral.get(ServletDefinition.class));
    final Injector injector = mock(Injector.class);
    when(injector.getBinding(Key.get(HttpServlet.class))).thenReturn(binding);
    when(injector.getInstance(HTTP_SERLVET_KEY)).thenReturn(mockServlet);
    when(injector.findBindingsByType(eq(servletDefsKey.getTypeLiteral())))
        .thenReturn(ImmutableList.of(mockBinding));
    return injector;
}

The code to create the mock before using these methods looks like this:

final Injector injector = mock(Injector.class);//mock injector
final Binding<HttpServlet> binding = mock(Binding.class);//mock injector Binding<HttpServlet> 
//Other java code in testcase
when(binding.acceptScopingVisitor((BindingScopingVisitor) any())).thenReturn(true);
when(injector.getBinding(Key.get(HttpServlet.class))).thenReturn(binding);
when(injector.getInstance(HTTP_SERLVET_KEY)).thenReturn(mockServlet);

final Key<ServletDefinition> servetDefsKey = Key.get(TypeLiteral.get(ServletDefinition.class));

Binding<ServletDefinition> mockBinding = mock(Binding.class);//mock Binding<ServletDefinition>
when(injector.findBindingsByType(eq(servetDefsKey.getTypeLiteral())))
    .thenReturn(ImmutableList.<Binding<ServletDefinition>>of(mockBinding));
Provider<ServletDefinition> bindingProvider = Providers.of(servletDefinition);
when(mockBinding.getProvider()).thenReturn(bindingProvider);

Using these methods, the refactored code becomes much cleaner:

final Binding<HttpServlet> binding = createMockBinding();
// Other code in the test case
Binding<ServletDefinition> mockBinding = createMockBinding(servletDefinition);
final Injector injector = createMockInjector(binding, mockServlet, mockBinding);

And for further improvement, we can overload createMockInjector for a more streamlined approach:

public final Injector createMockInjector(ServletDefinition servletDefinition, HttpServlet mockServlet) {
    return createMockInjector(createMockBinding(), mockServlet, createMockBinding(servletDefinition));
}

This final refactored code is:

// Other code in the test case
final Injector injector = createMockInjector(servletDefinition, mockServlet);

I’ve created a draft PR in my forked project where you can see the detailed changes here.

The refactor reduced the test cases by 60 lines of code, and I believe these changes will improve code readability.