Description
Dear Spring Security Developers,
I have been reviewing your test code and noticed an issue with repeated creation of mock objects in your tests. Here are four examples where this occurs:
Example 1
This example is found in ActiveDirectoryLdapAuthenticationProviderTests.
The repeated code fragment is as follows:
DirContext ctx = mock(DirContext.class);
given(ctx.getNameInNamespace()).willReturn("");The issue with this code is that willReturn("") is unnecessary because Mockito defaults the return of String methods to "". This piece of code is used 18 times across 8 test cases.
Thus, I propose the following solution:
- Remove
given(ctx.getNameInNamespace()).willReturn(""); - Move
DirContext ctx = mock(DirContext.class);to@BeforeEach
This change reduces the code by 16 lines. I have created a Draft PR, and you can view the changes here.
Example 2
This code is repeated in 8 test cases across 3 test classes:
The repeated code is as follows:
SecurityContextHolderStrategy strategy = mock(SecurityContextHolderStrategy.class);
// other statements
given(strategy.getContext()).willReturn(new SecurityContextImpl(authentication));The creation and given content of SecurityContextHolderStrategy mocks are very similar each time, except for the return value.
My solution is to create a MockSecurityContextHolderStrategy function, which is called each time a mock is needed:
public class MockSecurityContextHolderStrategy {
static SecurityContextHolderStrategy getmock(SecurityContextImpl securityContextImpl){
SecurityContextHolderStrategy strategy = mock(SecurityContextHolderStrategy.class);
given(strategy.getContext()).willReturn(securityContextImpl);
return strategy;
}
}The code after calling it would look like this:
SecurityContextHolderStrategy strategy = MockSecurityContextHolderStrategy.getmock(new SecurityContextImpl(authentication));I have created a Draft PR, and you can view the changes here.
Example 3
Similar to Example 2, I found repetition in the test class ConcurrentSessionFilterTests when creating mocks for SessionRegistry as follows:
SessionRegistry registry = mock(SessionRegistry.class);
SessionInformation information = new SessionInformation("user", "sessionId",
new Date(System.currentTimeMillis() - 1000));
information.expireNow();
given(registry.getSessionInformation(anyString())).willReturn(information);This repetition occurs in 6 test cases.
My solution is to create a mockSessionRegistry:
private SessionRegistry mockSessionRegistry(){
SessionRegistry registry = mock(SessionRegistry.class);
SessionInformation information = new SessionInformation("user", "sessionId",
new Date(System.currentTimeMillis() - 1000));
information.expireNow();
given(registry.getSessionInformation(anyString())).willReturn(information);
return registry;
}To create a SessionRegistry, simply call as follows:
ConcurrentSessionFilter filter = new ConcurrentSessionFilter(mockSessionRegistry());I have created
a Draft PR, and you can view the changes here.
Example 4
This repetition occurs in ExceptionTranslationFilterTests with the following code:
FilterChain fc = mock(FilterChain.class);
willThrow(new BadCredentialsException("")).given(fc)
.doFilter(any(HttpServletRequest.class), any(HttpServletResponse.class));This occurs in 8 different test cases, each returning a different Exception.
My solution is to create a method mockFilterChainWithException that takes different Exceptions as parameters:
private FilterChain mockFilterChainWithException(Object exception) throws ServletException, IOException {
FilterChain fc = mock(FilterChain.class);
willThrow((Throwable) exception).given(fc).doFilter(any(HttpServletRequest.class), any(HttpServletResponse.class));
return fc;
}To create a FilterChain mock, call it like this:
FilterChain fc = mockFilterChainWithException(new BadCredentialsException(""));I have created a Draft PR, and you can view the changes here.
These are the four instances of repeated mock test code I found, along with my solutions for refactoring the test code. I believe these changes will
- reduce the LOC for each test case
- allowing tests to focus more on testing rather than on recreating objects.
- Most importantly, consolidating repetitive parts improves code maintainability.
Dear Spring Security developers, Do you like the solution proposed in the issue? I'm very much looking forward to your response, and I'm ready to start submitting PRs if you're interested. If you have any suggestions for improvements, I would greatly appreciate them.