We recently introduced GeodeSecurityUtil for GEODE-17, and I'm looking into
ways to refactor this class or the classes using it to better facilitate
unit testing of those classes. I'm hoping that others on this list will
chime in with ideas and opinions. GeodeSecurityUtil itself is a collection
of public static methods that use Apache Shiro and callback APIs (such as
SecurityManager) in Geode.
First, a few general pre-assertions:
A.1) we want to embrace clean coding and solid OOP coding practices,
A.2) we want to write fast running unit tests that fully isolate the class
under test (this typically uses mocking),
A.3) we want to be able to unit test any code in a class even if it's in a
method that invokes public static methods in another class,
A.4) simple mocking with Mockito is much preferred over using Powermock,
A.5) we want to also write integration tests but the emphasis is on fast
unit tests.
There are quite a lot of procedurally static classes already in the Geode
codebase, so I'm hoping some of these ideas will be applicable to helping
out others working on other areas of the code as well.
Any class that has calls to a collaborator with public static methods makes
the unit testing of that class much more difficult:
B.1) you cannot spy the static util class to verify that the class under
test invoked it with proper parameters
B.2) you cannot mock the static util class to return expected values to the
class under test
If the following is a util type class:
public class GeodeUtils {
public static void persist(String key) {
...
}
public static String getValue() {
...
}
}
...then there are two potential ways to facilitate unit testing of classes
that depend on GeodeUtils:
C.1) change all methods in GeodeUtils to be non-static and pass in an
instance of GeodeUtils to the class constructor of any class that will use
it
C.2) wrap these statics with an interface and default implementation that
delegates to the static calls:
public interface GeodeService {
void persist(String key);
String getValue();
}
public class DefaultGeodeService implements GeodeService {
public void persist(String key) {
GeodeUtils.persist(key);
}
public String getValue() {
return GeodeUtils.getValue();
}
}
public class DurableOperation {
private final Properties config;
private final GeodeService geodeService;
public DurableOperation(Properties config) {
this(config , new DefaultGeodeService());
}
DurableOperation(Properties config, GeodeService geodeService) {
this.config = config;
this.geodeService = geodeService;
}
public void perform(String key) throws IOException {
...
this.geodeService.persist(key);
...
}
}
public class DurableOperationTest {
@Test
public void shouldPersistKey() throws Exception {
// given
Properties config = generateFancyOperationConfig();
GeodeService spyGeodeService = spy(GeodeService.class);
when(spyGeodeService.getValue()).thenReturn("bar");
DurableOperation durableOperation = new DurableOperation(config,
spyGeodeService);
// when
durableOperation.perform("foo");
// then
verify(spyGeodeService, times(1)).persist("foo");
}
}
C.3) do C.2 until C.1 can occur (what prevents us from just doing C.1?)
I believe that in general we want to use the solution in C.1 where possible.
To foster some discussion:
D.1) do we agree on the pre-assertions I listed in A.1-5?
D.2) do we have procedurally static classes that cannot or should not be
changed from static to non-static as described by C.1?
D.3) are there any other approaches that will better facilitate unit
testing of classes that depend on static collaborators?
I'd like to ultimately encourage the Geode dev community to avoid creating
new classes that consist of static methods and state. I believe we can do
this and that it's for the best.
-Kirk
