Definitely a topic ripe for debate :) In my view there’s a whole spectrum, with one side being the case Oleg describes, where the existing encapsulation is not compromised solely for the sake of testing. On the far side is pure design-by-contract. For example, the case could be made that the JMS processor should not be so tightly coupled to a particular client, and certainly not to a class but rather to an interface. Another upside for moving the client call to a protected method is not just for testing but so that child classes can override, which is not an encapsulation thing but inheritance. That might not be useful in this particular case, but if we’re talking OO in general then it applies.
Since Bryan has cited precedence for the inner class stuff in NiFi, I tend towards that as a consistent approach. Then again, to quote my close friend Oscar Wilde ;) "Consistency is the last refuge of the unimaginative" lol Cheers! Matt On 12/18/15, 5:54 PM, "Oleg Zhurakousky" <[email protected]> wrote: >Personally I am with Joe on this one > >Exposing visibility on the method just for testing is very dangerous as it >breaks encapsulation. There are different expectations and consideration on >things that are made private, protected and public. Yes, all of that is >meaningless when one uses reflection, but that’s a whole other discussion. >Relaxing visibility implies advertisement that something is up for grabs. And >in fact in Joe’s case while his intentions were noble, the fallout could be >anything but (see my comments here https://github.com/apache/nifi/pull/145). > >Just my opinion > >Cheers >Oleg > >On Dec 18, 2015, at 5:42 PM, Joe Skora ><[email protected]<mailto:[email protected]>> wrote: > >Wrapping createMessageProducer() in an instance method is a good >suggestion, but it seems overkill just to enable testing. Prompted by >Oleg's suggestion, I got around instance variable visibility with >Reflection, which is nice because it doesn't require "private" be changed >to "protected" in the class under test and doesn't require an inner test >class. But, that doesn't appear to be possible for static methods, so >wrapping with class methods may be the only choice. Hopefully, I've missed >something. > > >On Fri, Dec 18, 2015 at 3:58 PM, Bryan Bende ><[email protected]<mailto:[email protected]>> wrote: > >If you get it into a protected instance method, you can also make an inner >class in your test, something like TestablePutJMS extends PutJMS, and >overrides that method to return a mock or whatever you want. That is a >common pattern in a lot of the processor tests. > >On Fri, Dec 18, 2015 at 3:44 PM, Matt Burgess ><[email protected]<mailto:[email protected]>> wrote: > >You could move the one static call into an instance method of PutJMS, and >use Mockito.spy() to get a partial mock of the processor, then use when() >to override the instance method in the test. Not sure if that's how it's >done in other places but it's worked for me in the past. > >Regards, >Matt > >Sent from my iPhone > >On Dec 18, 2015, at 3:20 PM, Joe Skora ><[email protected]<mailto:[email protected]>> wrote: > >For unit testing, one problem I've run into is overriding the returns >from >static class methods. > >For instance, PutJMS contains this code: > >try { > wrappedProducer = JmsFactory.createMessageProducer(context, true); > logger.info("Connected to JMS server {}", > new Object[]{context.getProperty(URL).getValue()}); >} catch (final JMSException e) { > logger.error("Failed to connect to JMS Server due to {}", new >Object[]{e}); > session.transfer(flowFiles, REL_FAILURE); > context.yield(); > return; >} > >where JmsFactory.createmessageProducer call being defined as > >public static WrappedMessageProducer createMessageProducer(... > >which presents a problem since it can't be easily overridden for a unit >test. Exercising the > >How you handle this problem? > >Regards, >Joe > > >
