Jinmei,
You describe a good solution for testing GeodeSecurityUtil directly in
Integration Tests. This does not however address unit testing of classes
that depend on GeodeSecurityUtil.
Such classes need to be completely isolated in the unit test. Then the
behavior needs to be tested including the interaction with collaborators
-- such as expected parameters passed to and expected results returned from.
For example, we want to test that each Client Command is passing the
correct permission to the authorize methods. The best way to do this is
with Mockito and JMock. We can write very fast-running tests for actions
that occur when authorized AND when not authorized without resorting to
longer-running Integration Tests
We do want a few Integration Tests but we want many more Unit Tests for
every interaction.
-Kirk
On Wednesday, July 20, 2016, Jinmei Liao <jil...@pivotal.io> 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
>
