You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by "mcmellawatt (GitHub)" <gi...@apache.org> on 2018/11/21 20:02:28 UTC
[GitHub] [geode] mcmellawatt opened pull request #2891: GEODE-6052:
Use constructor DI to avoid PowerMock in JMX tests
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [X] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [X] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [X] Is your initial contribution a single, squashed commit?
- [X] Does `gradlew build` run cleanly?
- [X] Have you written or updated unit tests to verify your changes?
- N/A If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
### Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
[ Full content available at: https://github.com/apache/geode/pull/2891 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on pull request #2891:
GEODE-6052: Use constructor DI to avoid PowerMock in JMX tests
Posted by "kirklund (GitHub)" <gi...@apache.org>.
While you're editing MBeanJMXAdapter, I recommend making these member fields final:
```
private Map<ObjectName, Object> localGemFireMBean;
private DistributedMember distMember;
```
Please make this logger static final:
```
private Logger logger = LogService.getLogger();
```
Although I guess the above is the whole point for this PowerMock usage(?). See more comments at the bottom of this for better ways to test this class that follow mocking best practices.
Please make this reference to the Platform MBeanServer final as well:
```
public static MBeanServer mbeanServer = ManagementFactory.getPlatformMBeanServer();
```
Unfortunately, making the above two fields `static final` require some changes to MBeanJMXAdapterTest. Mocking best practices discourage mocking anything that isn't our code, so think about what you could do to change those tests. Same goes for Logger. In general, you should take one of these approaches:
1. Don't test Logger usage is one option that's favored by many authors.
2. Move usage of Logger and MBeanServer to package-protected methods and then use a Mockito spy for partial mocking in the test. Mocking best practices also discourage partial mocking but this is encouraged above mocking a JDK class which is what the tests currently do. Best practices say to only resort to partial mocking when testing legacy code that you don't want to refactor further.
3. Hide Logger and/or MBeanServer usage within new classes that will be passed into the ctor of MBeanJMXAdapter, becoming new collaborators. Then mock those collaborators. This is requires the most work but is considered the best practice for mocking and testing in this case.
4. If you really need to test logging directly, then please use a custom log4j2.xml and write a dedicated logging integration test for MBeanJMXAdapter in org.apache.geode.internal.logging.log4j -- use any of the integrationTests in that package for examples. You can use ListAppender to capture LogEvents, you can use `debug="true"` on the LogWriterAppender to capture LogEvents, or you can use SystemOutRule to capture stdout output generated by the Logger. In general, `#3` is a better approach for this case.
[ Full content available at: https://github.com/apache/geode/pull/2891 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt commented on pull request #2891:
GEODE-6052: Use constructor DI to avoid PowerMock in JMX tests
Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.
I was able to make all the suggested fields `final` (including the `static final` logger), except for the `mbeanServer`. I need to be able to replace this server with a mock which simulates a race condition, which is at the heart of the test. I ended up going with a variation of option #2 above to be able to use the `static final` logger. We found that option #4 had some side effects when the test is run as a unit test, because there are interactions between Log4J and System.out which prevent running a test in the same JVM twice.
[ Full content available at: https://github.com/apache/geode/pull/2891 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on pull request #2891:
GEODE-6052: Use constructor DI to avoid PowerMock in JMX tests
Posted by "kirklund (GitHub)" <gi...@apache.org>.
SystemManagementService already has a reference to InternalDistributedSystem, so please use that instead of reaching out to the singleton getter.
Several of the member fields in SystemManagementService including `private InternalDistributedSystem system` can be made final and should be changed as well:
```
private InternalDistributedSystem system;
private NotificationHub notificationHub;
private MBeanJMXAdapter jmxAdapter;
private InternalCache cache;
private ManagementResourceRepo repo;
private List<ProxyListener> proxyListeners;
private UniversalListenerContainer universalListenerContainer = new UniversalListenerContainer();
```
The fact that the following two fields cannot be final indicates a possible concurrency bug in this class especially given the volatile fields `closed` and `isStarted` (which suggests that multiple threads use the instance of this class). I recommend reviewing these to determine if more bugs are hiding:
```
private LocalManager localManager;
private FederatingManager federatingManager;
```
The inner-class UniversalListenerContainer also has a field that can be final:
```
private List<MembershipListener> membershipListeners = new CopyOnWriteArrayList<>();
```
[ Full content available at: https://github.com/apache/geode/pull/2891 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on pull request #2891:
GEODE-6052: Use constructor DI to avoid PowerMock in JMX tests
Posted by "kirklund (GitHub)" <gi...@apache.org>.
Yes. If `SystemManagementService` has a reference to the previous `InternalDistributedSystem` then something is very wrong.
[ Full content available at: https://github.com/apache/geode/pull/2891 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on pull request #2891:
GEODE-6052: Use constructor DI to avoid PowerMock in JMX tests
Posted by "kirklund (GitHub)" <gi...@apache.org>.
Just skip the other two fields. It's enough just to look at them to see how much work there is. I think you've done more than enough for this ticket!
[ Full content available at: https://github.com/apache/geode/pull/2891 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jinmeiliao commented on pull request #2891:
GEODE-6052: Use constructor DI to avoid PowerMock in JMX tests
Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.
usually a logger is static
[ Full content available at: https://github.com/apache/geode/pull/2891 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jinmeiliao commented on pull request #2891:
GEODE-6052: Use constructor DI to avoid PowerMock in JMX tests
Posted by "jinmeiliao (GitHub)" <gi...@apache.org>.
you probably can get away with only injecting the DistributedMember for this constructor to satisfy all your mocking need in the junit test
[ Full content available at: https://github.com/apache/geode/pull/2891 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt commented on pull request #2891:
GEODE-6052: Use constructor DI to avoid PowerMock in JMX tests
Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.
I was able to make all the suggested fields `final` (including the `static final` logger), except for the `mbeanServer`. I need to be able to replace this server with a mock which simulates a race condition, which is at the heart of the test. I ended up going with a variation of option 2 above to be able to use the `static final` logger. We found that option 4 had some side effects when the test is run as a unit test, because there are interactions between Log4J and System.out which prevent running a test in the same JVM twice.
[ Full content available at: https://github.com/apache/geode/pull/2891 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt closed pull request #2891: GEODE-6052:
Use constructor DI to avoid PowerMock in JMX tests
Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.
[ pull request closed by mcmellawatt ]
[ Full content available at: https://github.com/apache/geode/pull/2891 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt commented on pull request #2891:
GEODE-6052: Use constructor DI to avoid PowerMock in JMX tests
Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.
I spent quite a bit of time understanding why the logger should be static, and it appears to be a combination of convention and also protecting against potentially expensive instantiations if there are many instances of the object which references the logger.
https://logging.apache.org/log4j/2.x/manual/usage.html#StaticVsNonStatic
The downside of using a static logger is of course that you cannot inject the logger via DI. Without DI, we can still test using facades, adapters, and other such tools but its not as straight forward as simple DI.
I went ahead and made it static and inlined the assignment, then added a package-protected wrapper method for the logger that I verify in my test.
[ Full content available at: https://github.com/apache/geode/pull/2891 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt commented on pull request #2891:
GEODE-6052: Use constructor DI to avoid PowerMock in JMX tests
Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.
Is it safe to assume that the distributed system reference in `SystemManagementService` is identical to the one retrieved by `InternalDistributedSystem.getConnectedInstance()`? I'd think that this is safe due to the distributed system being a singleton, but just checking.
I was able to make the suggested fields final, except for the two you mentioned (which may be hiding concurrency bugs as you pointed out). I'll take a look at the potential for concurrency issues, but I feel that is probably outside of the scope of this ticket.
[ Full content available at: https://github.com/apache/geode/pull/2891 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] kirklund commented on pull request #2891:
GEODE-6052: Use constructor DI to avoid PowerMock in JMX tests
Posted by "kirklund (GitHub)" <gi...@apache.org>.
While you're editing MBeanJMXAdapter, I recommend making these member fields final:
```
private Map<ObjectName, Object> localGemFireMBean;
private DistributedMember distMember;
```
Please make this logger static final:
```
private Logger logger = LogService.getLogger();
```
Although I guess the above is the whole point for this PowerMock usage(?). See more comments at the bottom of this for better ways to test this class that follow mocking best practices.
Please make this reference to the Platform MBeanServer final as well:
```
public static MBeanServer mbeanServer = ManagementFactory.getPlatformMBeanServer();
```
Unfortunately, making the above two fields `static final` require some changes to MBeanJMXAdapterTest. Mocking best practices discourage mocking anything that isn't our code, so think about what you could do to change those tests. Same goes for Logger. In general, you should take one of these approaches:
1. Don't test Logger usage is one option that's favored by many authors.
2. Move usage of Logger and MBeanServer to package-protected methods and then use a Mockito spy for partial mocking in the test. Mocking best practices also discourage partial mocking but this is encouraged above mocking a JDK class which is what the tests currently do. Best practices say to only resort to partial mocking when testing legacy code that you don't want to refactor further.
3. Hide Logger and/or MBeanServer usage within new classes that will be passed into the ctor of MBeanJMXAdapter, becoming new collaborators. Then mock those collaborators. This requires the most work but is usually considered the best practice for mocking and testing in this case.
4. [edited] My recommendation is to use SystemOutRule and then just assert that the logged output contains the expected log message.
[ Full content available at: https://github.com/apache/geode/pull/2891 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org