I've created a Jira to track the elimination of PowerMock from these tests in particular, which will probably involve doing the major refactoring mentioned in item #4 in my previous email.
https://issues.apache.org/jira/browse/GEODE-6052 On Wed, Nov 14, 2018 at 10:57 AM Ryan McMahon <mcmellaw...@apache.org> wrote: > 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) >> >
