I will write up a story to address the use of PowerMock in these JMX tests in particular. I remember attempting to avoid PowerMock when writing this test, because I agree that it should be avoided. I just want to explain my thinking so that we can discuss what would have been a better approach.
In this particular set of tests, I used it to mock two static methods: 1. InternalDistributedSystem.getConnectedInstance() 2. LogService.getLogger() I considered making these instance methods as part of this ticket. When I started down that path, I found there are 41 calls to InternalDistributedSystem.getConnectedInstance(), and many of the callers have no reference to an instance of InternalDistributedSystem. While I think this is a solvable problem, it would have vastly increased the scope of the work involved. Would this ticket have been the place to do that work? I am under the impression that we have some Jira tickets specifically related to removing statics. Are those more aptly fit for that work? I agree that the LogService could have been injected in the MBeanJMXAdapter constructor, so that would have been a reasonable API change to make this testable without mocking the static LogService. However, I'm still interested in what people think about the InternalDistributedSystem case described above. There are ways I could have changed the public API of InternalDistributedSystem and related classes to make them testable without the use of PowerMock. There are a lot of ways to do this, but they all boil down to adding test hooks in public API. Is that better than using PowerMock? I see the following options: 1. Don't test at this level which requires mocking statics 2. Use PowerMock 3. Add test hooks (perhaps make InternalDistributedSystem wrap a singleton which you can swap for a test object, but there are other ways to do this) 4. Do major refactoring to eliminate the statics involved I don't think that not testing at this level is a great solution. We've discussed the disadvantages of PowerMock already, so the cons of #2 are well known. I am not a fan of adding public API test hooks, and in this case I'd rather use PowerMock honestly, but that is highly subjective. And #4 is the "ideal world" case, but isn't always practically possible given the scope of the work involved. I'm curious what other people think about this topic? Thanks, Ryan On Tue, Nov 6, 2018 at 10:40 AM Kirk Lund <kl...@apache.org> wrote: > I think we should try really hard to avoid using PowerMock. If you have > some code that you need to use PowerMock to make it testable, please > consider refactoring the code to a) avoid statics, b) pass all dependencies > in via the constructor. > > The following was spit out by one of my unit test runs. None of the logging > or log4j tests in my PR involve PowerMock. Also, this unit test run had no > FAILED tests, so this would appear to be generated by some unit test that > uses PowerMock and PASSED. > > > Task :geode-core:test > 2018-11-06 18:03:49,443 Distributed system shutdown hook ERROR Could not > reconfigure JMX java.lang.LinkageError: loader constraint violation: loader > (instance of org/powermock/core/classloader/MockClassLoader) previously > initiated loading for a different type with name > "javax/management/MBeanServer" > at java.lang.ClassLoader.defineClass1(Native Method) > at java.lang.ClassLoader.defineClass(ClassLoader.java:763) > at > > org.powermock.core.classloader.MockClassLoader.loadUnmockedClass(MockClassLoader.java:262) > at > > org.powermock.core.classloader.MockClassLoader.loadModifiedClass(MockClassLoader.java:206) > at > > org.powermock.core.classloader.DeferSupportingClassLoader.loadClass1(DeferSupportingClassLoader.java:89) > at > > org.powermock.core.classloader.DeferSupportingClassLoader.loadClass(DeferSupportingClassLoader.java:79) > at java.lang.ClassLoader.loadClass(ClassLoader.java:357) > at > > org.apache.logging.log4j.core.jmx.Server.unregisterAllMatching(Server.java:335) > at > > org.apache.logging.log4j.core.jmx.Server.unregisterLoggerContext(Server.java:259) > at > > org.apache.logging.log4j.core.jmx.Server.reregisterMBeansAfterReconfigure(Server.java:164) > at > > org.apache.logging.log4j.core.jmx.Server.reregisterMBeansAfterReconfigure(Server.java:140) > at > > org.apache.logging.log4j.core.LoggerContext.setConfiguration(LoggerContext.java:558) > at > > org.apache.logging.log4j.core.LoggerContext.reconfigure(LoggerContext.java:619) > at > > org.apache.logging.log4j.core.LoggerContext.reconfigure(LoggerContext.java:636) > at > org.apache.logging.log4j.core.LoggerContext.start(LoggerContext.java:231) > at > > org.apache.logging.log4j.core.impl.Log4jContextFactory.getContext(Log4jContextFactory.java:243) > at > > org.apache.logging.log4j.core.impl.Log4jContextFactory.getContext(Log4jContextFactory.java:45) > at org.apache.logging.log4j.LogManager.getContext(LogManager.java:174) > at org.apache.logging.log4j.LogManager.getLogger(LogManager.java:661) > at > org.apache.geode.internal.logging.LogService.getLogger(LogService.java:64) > at > > org.apache.geode.internal.tcp.ConnectionTable.<clinit>(ConnectionTable.java:61) > at > > org.apache.geode.distributed.DistributedSystem.setThreadsSocketPolicy(DistributedSystem.java:263) > at > > org.apache.geode.distributed.internal.InternalDistributedSystem.lambda$static$0(InternalDistributedSystem.java:2320) > at java.lang.Thread.run(Thread.java:748) >