This is an automated email from the ASF dual-hosted git repository. mcmellawatt pushed a commit to branch develop in repository https://gitbox.apache.org/repos/asf/geode.git
The following commit(s) were added to refs/heads/develop by this push: new 8aba8b9 GEODE-6052: Use constructor DI to avoid PowerMock in JMX tests (#2891) 8aba8b9 is described below commit 8aba8b9f8f0301370cf55fcd8824b2a80e21f3d1 Author: Ryan McMahon <rmcma...@pivotal.io> AuthorDate: Mon Nov 26 16:13:31 2018 -0800 GEODE-6052: Use constructor DI to avoid PowerMock in JMX tests (#2891) --- .../internal/security/MBeanSecurityJUnitTest.java | 3 +- .../geode/management/internal/MBeanJMXAdapter.java | 20 +++++---- .../internal/SystemManagementService.java | 19 ++++----- .../management/internal/MBeanJMXAdapterTest.java | 47 ++++++---------------- 4 files changed, 37 insertions(+), 52 deletions(-) diff --git a/geode-core/src/integrationTest/java/org/apache/geode/management/internal/security/MBeanSecurityJUnitTest.java b/geode-core/src/integrationTest/java/org/apache/geode/management/internal/security/MBeanSecurityJUnitTest.java index e573a0e..c1130fb 100644 --- a/geode-core/src/integrationTest/java/org/apache/geode/management/internal/security/MBeanSecurityJUnitTest.java +++ b/geode-core/src/integrationTest/java/org/apache/geode/management/internal/security/MBeanSecurityJUnitTest.java @@ -35,6 +35,7 @@ import org.junit.Rule; import org.junit.Test; import org.junit.experimental.categories.Category; +import org.apache.geode.distributed.DistributedMember; import org.apache.geode.management.ManagementException; import org.apache.geode.management.ManagementService; import org.apache.geode.management.MemberMXBean; @@ -106,7 +107,7 @@ public class MBeanSecurityJUnitTest { () -> server.createMBean("FakeClassName", new ObjectName("GemFire", "name", "foo"))) .isInstanceOf(ReflectionException.class); - MBeanJMXAdapter adapter = new MBeanJMXAdapter(); + MBeanJMXAdapter adapter = new MBeanJMXAdapter(mock(DistributedMember.class)); assertThatThrownBy(() -> adapter.registerMBean(mock(DynamicMBean.class), new ObjectName("MockDomain", "name", "mock"), false)) .isInstanceOf(ManagementException.class); diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/MBeanJMXAdapter.java b/geode-core/src/main/java/org/apache/geode/management/internal/MBeanJMXAdapter.java index 3a13c11..a76a85c 100644 --- a/geode-core/src/main/java/org/apache/geode/management/internal/MBeanJMXAdapter.java +++ b/geode-core/src/main/java/org/apache/geode/management/internal/MBeanJMXAdapter.java @@ -63,22 +63,21 @@ import org.apache.geode.management.RegionMXBean; * */ public class MBeanJMXAdapter implements ManagementConstants { - /** The <code>MBeanServer</code> for this application */ public static MBeanServer mbeanServer = ManagementFactory.getPlatformMBeanServer(); - private Map<ObjectName, Object> localGemFireMBean; + private final Map<ObjectName, Object> localGemFireMBean; - private DistributedMember distMember; + private final DistributedMember distMember; - private Logger logger = LogService.getLogger(); + private static final Logger logger = LogService.getLogger(); /** * public constructor */ - public MBeanJMXAdapter() { + public MBeanJMXAdapter(DistributedMember distMember) { this.localGemFireMBean = new ConcurrentHashMap<>(); - this.distMember = InternalDistributedSystem.getConnectedInstance().getDistributedMember(); + this.distMember = distMember; } /** @@ -165,7 +164,7 @@ public class MBeanJMXAdapter implements ManagementConstants { } catch (InstanceAlreadyExistsException instanceAlreadyExistsException) { // An InstanceAlreadyExistsException in this context means that the MBean // has already been registered, so just log a warning message. - logger.warn("MBean with ObjectName " + objectName + " has already been registered."); + logRegistrationWarning(objectName, true); } catch (NullPointerException | NotCompliantMBeanException | MBeanRegistrationException e) { throw new ManagementException(e); @@ -194,7 +193,7 @@ public class MBeanJMXAdapter implements ManagementConstants { // has already been unregistered, so just log a debug message as it is // essentially a no-op. // has already been unregistered, so just log a warning message. - logger.warn("MBean with ObjectName " + objectName + " has already been unregistered."); + logRegistrationWarning(objectName, false); } catch (NullPointerException | MBeanRegistrationException e) { throw new ManagementException(e); } @@ -561,4 +560,9 @@ public class MBeanJMXAdapter implements ManagementConstants { return true; } + + void logRegistrationWarning(ObjectName objectName, boolean registering) { + logger.warn("MBean with ObjectName " + objectName + " has already been " + + (registering ? "registered." : "unregistered.")); + } } diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/SystemManagementService.java b/geode-core/src/main/java/org/apache/geode/management/internal/SystemManagementService.java index 2f069d4..0ffa38d 100755 --- a/geode-core/src/main/java/org/apache/geode/management/internal/SystemManagementService.java +++ b/geode-core/src/main/java/org/apache/geode/management/internal/SystemManagementService.java @@ -66,7 +66,7 @@ public class SystemManagementService extends BaseManagementService { /** * The concrete implementation of DistributedSystem that provides internal-only functionality. */ - private InternalDistributedSystem system; + private final InternalDistributedSystem system; /** * core component for distribution @@ -77,7 +77,7 @@ public class SystemManagementService extends BaseManagementService { * This is a notification hub to listen all the notifications emitted from all the MBeans in a * peer cache./cache server */ - private NotificationHub notificationHub; + private final NotificationHub notificationHub; /** * whether the service is closed or not if cache is closed automatically this service will be @@ -93,15 +93,15 @@ public class SystemManagementService extends BaseManagementService { /** * Adapter to interact with platform MBean server */ - private MBeanJMXAdapter jmxAdapter; + private final MBeanJMXAdapter jmxAdapter; - private InternalCacheForClientAccess cache; + private final InternalCacheForClientAccess cache; private FederatingManager federatingManager; private final ManagementAgent agent; - private ManagementResourceRepo repo; + private final ManagementResourceRepo repo; /** * This membership listener will listen on membership events after the node has transformed into a @@ -113,9 +113,10 @@ public class SystemManagementService extends BaseManagementService { * Proxy aggregator to create aggregate MBeans e.g. DistributedSystem and DistributedRegion * GemFire comes with a default aggregator. */ - private List<ProxyListener> proxyListeners; + private final List<ProxyListener> proxyListeners; - private UniversalListenerContainer universalListenerContainer = new UniversalListenerContainer(); + private final UniversalListenerContainer universalListenerContainer = + new UniversalListenerContainer(); public static BaseManagementService newSystemManagementService( InternalCacheForClientAccess cache) { @@ -133,9 +134,9 @@ public class SystemManagementService extends BaseManagementService { throw new DistributedSystemDisconnectedException( "This connection to a distributed system has been disconnected."); } - this.jmxAdapter = new MBeanJMXAdapter(); - this.repo = new ManagementResourceRepo(); + this.jmxAdapter = new MBeanJMXAdapter(this.system.getDistributedMember()); + this.repo = new ManagementResourceRepo(); this.notificationHub = new NotificationHub(repo); if (system.getConfig().getJmxManager()) { diff --git a/geode-core/src/test/java/org/apache/geode/management/internal/MBeanJMXAdapterTest.java b/geode-core/src/test/java/org/apache/geode/management/internal/MBeanJMXAdapterTest.java index cc91c03..4f3a478 100644 --- a/geode-core/src/test/java/org/apache/geode/management/internal/MBeanJMXAdapterTest.java +++ b/geode-core/src/test/java/org/apache/geode/management/internal/MBeanJMXAdapterTest.java @@ -16,55 +16,34 @@ package org.apache.geode.management.internal; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.spy; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; -import static org.powermock.api.mockito.PowerMockito.mock; -import static org.powermock.api.mockito.PowerMockito.mockStatic; -import static org.powermock.api.mockito.PowerMockito.when; +import static org.mockito.Mockito.when; import javax.management.InstanceAlreadyExistsException; import javax.management.InstanceNotFoundException; import javax.management.MBeanServer; import javax.management.ObjectName; -import org.apache.logging.log4j.Logger; import org.junit.Before; import org.junit.Test; -import org.junit.runner.RunWith; -import org.powermock.core.classloader.annotations.PowerMockIgnore; -import org.powermock.core.classloader.annotations.PrepareForTest; -import org.powermock.core.classloader.annotations.SuppressStaticInitializationFor; -import org.powermock.modules.junit4.PowerMockRunner; - -import org.apache.geode.distributed.internal.InternalDistributedSystem; -import org.apache.geode.internal.logging.LogService; - -@RunWith(PowerMockRunner.class) -@PrepareForTest({InternalDistributedSystem.class, LogService.class}) -@PowerMockIgnore({"javax.management.*", "javax.script.*"}) -@SuppressStaticInitializationFor({"org.apache.geode.internal.logging.LogService", - "org.apache.geode.distributed.internal.InternalDistributedSystem"}) + +import org.apache.geode.distributed.DistributedMember; + public class MBeanJMXAdapterTest { private ObjectName objectName; private MBeanServer mockMBeanServer; - private Logger mockLogger; + private DistributedMember distMember; @Before public void setUp() throws Exception { - mockStatic(InternalDistributedSystem.class); - when(InternalDistributedSystem.getConnectedInstance()) - .thenReturn(mock(InternalDistributedSystem.class)); - - mockStatic(LogService.class); - mockLogger = mock(Logger.class); - when(mockLogger.isDebugEnabled()).thenReturn(true); - when(LogService.getLogger()).thenReturn(mockLogger); - mockMBeanServer = mock(MBeanServer.class); objectName = new ObjectName("d:type=Foo,name=Bar"); + distMember = mock(DistributedMember.class); } @Test @@ -77,14 +56,14 @@ public class MBeanJMXAdapterTest { // has already been unregistered doThrow(new InstanceNotFoundException()).when(mockMBeanServer).unregisterMBean(objectName); - MBeanJMXAdapter mBeanJMXAdapter = new MBeanJMXAdapter(); + MBeanJMXAdapter mBeanJMXAdapter = spy(new MBeanJMXAdapter(distMember)); MBeanJMXAdapter.mbeanServer = mockMBeanServer; mBeanJMXAdapter.unregisterMBean(objectName); // InstanceNotFoundException should just log a debug message as it is essentially a no-op // during unregistration - verify(mockLogger, times(1)).warn(anyString()); + verify(mBeanJMXAdapter, times(1)).logRegistrationWarning(any(ObjectName.class), eq(false)); } @Test @@ -98,13 +77,13 @@ public class MBeanJMXAdapterTest { doThrow(new InstanceAlreadyExistsException()).when(mockMBeanServer) .registerMBean(any(Object.class), eq(objectName)); - MBeanJMXAdapter mBeanJMXAdapter = new MBeanJMXAdapter(); + MBeanJMXAdapter mBeanJMXAdapter = spy(new MBeanJMXAdapter(distMember)); MBeanJMXAdapter.mbeanServer = mockMBeanServer; - mBeanJMXAdapter.registerMBeanProxy(mock(Object.class), objectName); + mBeanJMXAdapter.registerMBeanProxy(objectName, objectName); // InstanceNotFoundException should just log a debug message as it is essentially a no-op // during registration - verify(mockLogger, times(1)).warn(anyString()); + verify(mBeanJMXAdapter, times(1)).logRegistrationWarning(any(ObjectName.class), eq(true)); } }