One other to bear in mind is to avoid adding too much boiler code into the
production solely for the purpose of testing. I am typically a OO person,
but I wouldn't sacrifice code clarity and readability for it.
On Fri, Jul 15, 2016 at 12:14 PM, Kirk Lund <kl...@pivotal.io> wrote:
> 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
>
--
Cheers
Jinmei
