Just thought I'd share - after a second pass at this I was able to avoid PowerMock when modifying the class under test to use constructor DI. I think initially I was a bit reluctant to modify the production code here but on a second look I think it was the right thing to do. Just wanted to share the lesson I learned!
Ryan On Wed, Nov 14, 2018 at 1:34 PM Bruce Schuchardt <bschucha...@pivotal.io> wrote: > I don't think you necessarily need to redo your work Ryan. I just think > something has been left behind by the test. It looks like a mock made > its way into the JMX "Server" in the log4j code. > > On 11/14/18 10:57 AM, Ryan McMahon 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) > >> >
