[GitHub] [geode] kirklund commented on pull request #2891: GEODE-6052: Use constructor DI to avoid PowerMock in JMX tests

["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