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)
