IMHO static helper methods like those in Bytes are fine. We would never
want to substitute a mock implementation of one of those methods and they
don't have any collaborators.
Just glancing at GeodeSecurityUtils, what I don't like is that it's relying
heavily on mutable static singletons (securityManager, postProcessor) and
thread locals. I think we should avoid using singletons and thread locals
in geode code, because it's too easy to leave junk lying around and it also
makes code hard to test. In this case maybe it would make more sense to
pass some sort of session object around?
-Dan
On Mon, Jul 25, 2016 at 4:26 PM, Kirk Lund <kl...@pivotal.io> wrote:
> I was trying to describe any Java class in Geode which provides feature
> functionality that should be unit tested and integration tested, but is
> either currently implemented as a static class or which collaborates with a
> non-trivial component currently implemented as a static class.
>
> I wasn't describing the simple static classes like Byte, StringUtils,
> IOUtils. These are trivial and not generally considered to be an important
> collaborator requiring a mock.
>
> Using Mockito allows us to test the interaction between a class under test
> and its collaborators. To facilitate this, we need the collaborator to be
> defined as an interface or abstract class.
>
> This specifically came about on my team because we have been unit testing
> GMSAuthenticator and lots of Client Commands that interact with Integrated
> Security. We want to include unit tests to verify that the class under test
> correctly interacts with its collaborators including Integrated Security
> (GEODE-17). For this reason, we want to transform GeodeSecurityUtil into a
> non-static class that implements an interface that can be mocked.
>
> Hopefully everyone agrees that we need to make a concerted effort to write
> lots of new fast-running unit tests that use mocks. This includes old and
> new code.
>
> In general, actual features implemented in static classes is common in
> Geode but it inhibits unit testing and thus should change as we refactor
> the code towards testability.
>
> This thread started as me describing a simple pattern to facilitate unit
> testing against static classes that for whatever reason, we want to hold
> off refactoring them but we don't want to hold off writing unit tests for
> them as an important collaborator class.
>
> Does this make sense? Is everyone on the same page regarding the value of
> unit testing and refactoring to testability & maintainability?
>
> -Kirk
>
> On Wednesday, July 20, 2016, Anthony Baker <aba...@pivotal.io> wrote:
>
> > Great discussion! I’ve got two questions:
> >
> > 1) What do you think of Bytes.java
> > (:geode-core:com.gemstone.gemfire.internal.util)? Is it a reasonable use
> > of the Helper / Utility class pattern? If not, how would you change it?
> > Creating a new instance for each use seems inefficient and using a
> > Singleton is just a different way to do statics.
> >
> > 2) Is GeodeSecurityUtil of the same category as Bytes.java or is it
> > something more?
> >
> > Anthony
> >
> >
> > > On Jul 20, 2016, at 10:41 AM, Jinmei Liao <jil...@pivotal.io
> > <javascript:;>> wrote:
> > >
> > > Wouldn't the change be too intrusive? Now any class that uses security
> > > would need to have this additional constructor, and if a no-arg
> > constructor
> > > is available, it's error prone that the class will be used without
> > > security, and if we delete the no-arg constructor, that would cause a
> lot
> > > of chain reaction.
> > >
> > > The usage of shiro itself is static (all the state information is in
> the
> > > thread), which, in my opinion is one of the selling points of Shiro
> since
> > > it greatly simplifies the usage of it. That's why GeodeSecurityUtil
> > follows
> > > this path. If no security is configured, calls into any of the method
> in
> > > GeodeSecurityUtil would be a no-op. And each VM would need only one
> > > instance of GeodeSecurityUtil so static makes sense here IMHO.
> > >
> > > For testing purposes, if you are not testing security in your test, the
> > > calls into GeodeSecurityUtil/Shiro would act like non-existent. If you
> > need
> > > special behavior of GeodeSecurityUtil, just init it with a property
> > object
> > > that would give you the behavior you want in your test like this, but
> > then
> > > we've had tests that already covers the security aspect of all the
> > calling
> > > code, so your test would not need to.
> > >
> > > @Test
> > > public void testInitialSecurityFlags() {
> > > // initial state of GeodeSecurityUtil
> > > assertFalse(GeodeSecurityUtil.isClientSecurityRequired());
> > > assertFalse(GeodeSecurityUtil.isIntegratedSecurity());
> > > assertFalse(GeodeSecurityUtil.isPeerSecurityRequired());
> > > }
> > >
> > > @Test
> > > public void testInitWithSecurityManager() {
> > > properties.setProperty(SECURITY_MANAGER,
> > > "org.apache.geode.security.templates.SampleSecurityManager");
> > > GeodeSecurityUtil.initSecurity(properties);
> > > assertTrue(GeodeSecurityUtil.isClientSecurityRequired());
> > > assertTrue(GeodeSecurityUtil.isIntegratedSecurity());
> > > assertTrue(GeodeSecurityUtil.isPeerSecurityRequired());
> > > }
> > >
> > > @Test
> > > public void testInitWithClientAuthenticator()
> > > {
> > > properties.setProperty(SECURITY_CLIENT_AUTHENTICATOR, "org.abc.test");
> > > GeodeSecurityUtil.initSecurity(properties);
> > > assertTrue(GeodeSecurityUtil.isClientSecurityRequired());
> > > assertFalse(GeodeSecurityUtil.isIntegratedSecurity());
> > > assertFalse(GeodeSecurityUtil.isPeerSecurityRequired());
> > > }
> > >
> > > @Test
> > > public void testInitWithPeerAuthenticator()
> > > {
> > > properties.setProperty(SECURITY_PEER_AUTHENTICATOR, "org.abc.test");
> > > GeodeSecurityUtil.initSecurity(properties);
> > > assertFalse(GeodeSecurityUtil.isClientSecurityRequired());
> > > assertFalse(GeodeSecurityUtil.isIntegratedSecurity());
> > > assertTrue(GeodeSecurityUtil.isPeerSecurityRequired());
> > > }
> > >
> > > @Test
> > > public void testInitWithShiroAuthenticator()
> > > {
> > > properties.setProperty(SECURITY_SHIRO_INIT, "shiro.ini");
> > > GeodeSecurityUtil.initSecurity(properties);
> > > assertTrue(GeodeSecurityUtil.isClientSecurityRequired());
> > > assertTrue(GeodeSecurityUtil.isIntegratedSecurity());
> > > assertTrue(GeodeSecurityUtil.isPeerSecurityRequired());
> > > }
> > >
> > >
> > > On Wed, Jul 20, 2016 at 10:16 AM, Kirk Lund <kl...@pivotal.io
> > <javascript:;>> wrote:
> > >
> > >> To avoid adding boilerplate code, we could just avoid writing any new
> > >> static methods or classes of static methods. Remember static is bad in
> > OO
> > >> and is one of the main things that prevents true, good unit testing
> > because
> > >> you can't replace the class or the method with a fake of any sort
> (mock,
> > >> spy, or dummy).
> > >>
> > >> I would vote to eliminate all statics and adopt a policy of no new
> > statics
> > >> and follow:
> > >>
> > >> 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
> > >>
> > >> -Kirk
> > >>
> > >>
> > >> On Wed, Jul 20, 2016 at 9:57 AM, Jinmei Liao <jil...@pivotal.io
> > <javascript:;>> wrote:
> > >>
> > >>> 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
> > <javascript:;>> 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
> > >>>
> > >>
> > >
> > >
> > >
> > > --
> > > Cheers
> > >
> > > Jinmei
> >
> >
>
