Re: PowerMock unit test errors
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 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 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 > >> > >> >
Re: PowerMock unit test errors
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 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
Re: PowerMock unit test errors
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 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 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 >> >>
Re: PowerMock unit test errors
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 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) >
Re: PowerMock unit test errors
I'm running into the same PowerMock problem that Kirk hit. I can find only one test that uses PowerMock and invokes registerMBean - MBeanJMXAdapterTest. This is a new test that was checked in mid-October. commit 76420dcd93e17f009aa73ca8188d135158358323 Author: Ryan McMahon Date: Wed Oct 17 10:21:03 2018 -0700 GEODE-5857: Handle JMX race conditions during registration and cleanup Maybe it needs to do more cleanup On 11/6/18 10:40 AM, Kirk Lund 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.(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)
PowerMock unit test errors
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.(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)
