jMock

I am sorry for adding the source that way. But I guess the JIRA create Feature mask is incomplete. I was looking for attaching files with the feature and well, didn't found it.

Maybe they should improve it or well I am blind or something. Also I would like to edit my wrtings (remove them after I had found out how to add attachments). I found no edit button either (Is there a possiblity to alter a feature report?).

The getClassLoader() method of the CGLibCoreMock should be protected. It is critical to override it when using interfaces (or classes) of referenced plugins within eclipse.

By the way I found a good proof for the need of the CGLibCoreMock. I had to create a dummy instance of ASTNode, which is an important class of Eclipse JDT plugin. But sadly there isn't any interface to stub.

Abstract superclass constructors that call abstract methods (implemented in the class being mocked) are being intercepted by the mock, which of course has no expectations during construction.

This seems be caused by a (perhaps unavoidable) inconsistency in classes enhanced by CGLIB Enhancer. From my tests, method calls from the Enhanced class's constructor are not intercepted, however abstract calls from a super-constructor are intercepted. Note that pre-patch testConstructorCallsMethod passes, testSuperConstructorCallsMethods fails.

Patch and test cases attached.

Proposed patch.

Test cases.

I would like to add a strong vote of support for fixing this stuff. I made the jump to JMock when 1.0 came out. The CGLib support is great, and will be even better with support for non-default/non-trivial constructors. I have hit my head against the wall a number of times with the current implementation, and look forward to a solution to this being integrated.

(it will be great to have a dyna mock for the java io stuff).

I have tested the abstract method calls by a abstract super class constructor using your test-case and it works without a single problem.

You patched solution has two flaws. Frist of all the proxy is not created lazily. Therefore you can not stub or mock any method during the constructor call. Second there is no support for non-default constructors.

By testing did you used the attached solution?

Hi Martin,

My patch was only intended as a part response to your issue (which covers a couple of areas), rather than re-logging. I consider that the most serious bug is the issue with abstract methods called in the superclass constructor - my patch addresses that only. This was in response to a mailing list query today from Uwe.

I have not attempted to address non-default constructors.

If you are mocking the behaviour of a class, then hopefully the constuction of this object is irrelevant. You only want to mock the interaction of it via it's public interface. Ideally, a constructor for a mock object would be bypassed and have zero side-effects. I don't see the point in mocking constructor-called methods. The activities that take place during the construction of an object should have no impact on our ability to mock the object, so I don't see this as a flaw.

As you are probably aware (having responded to my prior posting), I have been doing some work with Javassist, and alternative bytecode tool to CGLIB, as I found it easy to completely bypass contructors of mock-objects using Javassist. I was unable to achieve this with CGLIB.

Regards

Brett

A constructor that calls an abstract method is fundamentally poor design. The constructor is calling into a part of the object that isn't constructed! In this case, jMock is doing EXACTLY what it is designed to do: highlight bad design decisions.

Hi Nat,

Thanks for your observations and comments.

Putting abstract methods aside for the moment, your suggestion seems to lead to the axiom that constructors ought not call any non-final methods, as overriding a concrete method (that is called by a superclass constructor) in the subclass, will result in the same thing - code from the subclass executing during construction of the superclass.

Used properly and carefully, I have considered constructor-calls-non-final-methods to be OK, but I'm very happy to be corrected. I like to insist that a constructor always leaves the object in a consistent, ready-to-use state, and this sometimes means that subclasses may need to (carefully) influence common construction code. I'm thinking that "banning" this could limit refactoring opportunities and could lead to repeated code in subclass constructors. Perhaps there is a better way though ...

Regards

Brett Randall

@Brett

>> Used properly and carefully, I have considered constructor-calls-non-final-methods to be OK, but I'm very happy to be corrected. I like to insist that a constructor always leaves the object in a consistent, ready-to-use state, and this sometimes means that subclasses may need to (carefully) influence common construction code. I'm thinking that "banning" this could limit refactoring opportunities and could lead to repeated code in subclass constructors. <<

There are rare situations, where you may use abstract methods or even overridden methods called in the construction phase (whether called by the constructor, an init/start/stop/destroy method or by an Aspect doesn't matter). Before the DependencyInjection (InversionOfControl) pattern got widely spread, people used protected abstract methods to create depending objects, making it able to deliver dependency by subclassing the object. Even without the StaticFactory pattern, this was also used for any object creation to reduce coupling and also to ease testing.

So it's not uncommon to see such code in the wild. And used carefully, there is nothing wrong with it in my oppinion. The trouble starts when you expose such things to the world. Then you will need many FallThroughStub(s) and you have to mock those classes partly, which let you feel great pain. So if this is only visible at package/module level, don't hate it. It adds value.

>> Perhaps there is a better way though ... <<

Sure it is! Mostly you can use the Strategy pattern or (derived from it) the Processor or the Visitor pattern. Those pattern are very valuable. Indeed these refactorings make people to say 'Wow'. Sometimes the Listener pattern is also handy. Just listen for changes and react by assisting the object you are listening to (feedback). And even the Command pattern... .

But there is nothing diffrent in calling am overridden method or providing a special strategy implementation passed to the object. You can cause abuse to most of those pattern easily, too. Also you have to remember the TDD process. You should never think something like: "Wow this task screams for the use of the Strategy pattern". The reason for such statements is mostly personal habbit.

So you will just write your tests and see where it will drive you to. That's one of my critics to the statement: "jMock is not a testing tool, it is a design tool'. The design is driven by tests/implementation over here. I can not refactor nor redesign something, I have not a single test for. How should I know I am right? And starting with something complex makes me debug and suffer from lost of confidence. Programming by starting with something complex is mostly a waste of time (also it is not fun).

@nat

>> A constructor that calls an abstract method is fundamentally poor design. <<

I agree with this statement. It's poor design, but sometimes it's the easiest thing you can come up with. So it's poor design but sometimes the best solution.

>> The constructor is calling into a part of the object that isn't constructed! In this case, jMock is doing EXACTLY what it is designed to do: highlight bad design decisions. <<

It does highlight it by forbidding things to do. That's always bad. How can you come up with something better, when there is nothing that works? I don't like to be forced to use state-based testing, just because my tool tells me to do.

And I am also asking myself, why does the CGLib enabled jMock extension exist after all? Mocking concret classes is always a bad design situation. So using the CGLib enabled extension removes the capablilites of jMock to highlight bad design in favour for easy testing? Why?

What I would like to see is a extension of cglib including a explaination, why using this extension is that bad. I use my own extension (fallThrough, non-default constructors) every day (some others do so, too). I just can test what I want. Bad design is highlighted by the every-day usage experience of the code, which includes writing tests. There is nothing wrong with interaction-based testing of bad design. It helps to spot some design gaps rather then forcing you to write complicated tests to get some tests running, you can trust on.

I have stoped to think about design in the first two phases of TDD (1. writing a test and 2. implementation). It's not a issue there. Think for the simpliest thing, that might work and go for it. Bad design is handled only in the 3rd phase (refactoring). I would advise to never do the design first approach, when using TDD. Also you will have metrics in phase three, not in phase one.

Please think about it. The CGLib enabled extension is already existing. The box is open so I would like to get as much out of the box as possible.

I am puzzled about it.

PS: jMock is great, I love it and use it every day... . (just to make things clear)

Methods in a constructor that are overridden in subclasses, whether on purpose or accidentally, are not the simplest thing that could possibly work. They're the simplest thing that will start creating obscure bugs when the system grows larger than can be maintained by two or four people. Sure, if you are working on a small piece of software you can get away with lots of dodgy design decisions. But when you're working on large systems being developed by large teams, proper encapsulation is very, very important. Having had to track down bugs caused by this very issue several times in a system that is over a million lines of Java code developed by over 100 people, I am very sensitive to the issue!

The CGLIB extension has its uses – to avoid an interface that would only exist for testing, for example – but you have to listen to your tests. If they're telling you that your classes are doing bad things, clean up your classes or introduce an interface.

>> Methods in a constructor that are overridden in subclasses, whether on purpose or accidentally, are not the simplest thing that could possibly work. They're the simplest thing that will start creating obscure bugs when the system grows larger than can be maintained by two or four people. <<

I think this is not a proper argument against it. Just think about any kind of initialisation. Constructor arn't the only ways to construct an object. By using an init method or establish contracts of dependency injection or something similar, you will run in the same situations. The only work around this, is to limit coupling by using static factories when ever possible. Stick to 'the interface belongs to the client' stuff and you will die old and happy. But inside this package you can do whatever you think is the best. Your unit-test will tell you if the created object returned by the factory does what it should. So you can reduce your problem to public non-abstract but extensible classes. And this is just the lesson being teached by 'programming against types rather then classes' principle.

>> Sure, if you are working on a small piece of software you can get away with lots of dodgy design decisions. But when you're working on large systems being developed by large teams, proper encapsulation is very, very important. <<

I didn't said something else, did I? Proper encapsulation is supported by common contracts, reducing coupling in any ways and hidding implementation details as much as possible. If you need to expose something important, you have to state it out. We have a common rule over here. Everytime you are about to override a method, whatch the javadoc comment of that method. Eclipse make this very easy. If there is a 'NOTE:' or 'CONTRACT:' annotation, read it and follow the advises.

>> Having had to track down bugs caused by this very issue several times in a system that is over a million lines of Java code developed by over 100 people, I am very sensitive to the issue! <<

Maybe we missunderstood each other. But my biggest project wasn't that big, thats true. It was about 200k lines and around 10 people and some nasty legacy code. (Lines of code means here the number of ';' minus the count of the for loops). But we also split up the responsibility over here. There were a couple of people being named for each modul. They need to be around everytime, something was changed on that modules or at least, they have to approve the change, before it is committed. I think this is common usage.

But I still don't see any diffrence between a constructor or an init method or any other kind of initialization of an object. All can lead into nasty bugs, when done without discipline. All can lead in inconsitent object states. But this is the water and bread of a programmer. Just watch the java docs, use it rarely and use it within a limited space like package level.

>> The CGLIB extension has its uses – to avoid an interface that would only exist for testing, for example – but you have to listen to your tests. If they're telling you that your classes are doing bad things, clean up your classes or introduce an interface <<

But again I have a problem, when it comes to legacy code or I am not confident in my understanding or you have to tackle bad libs like some parts of Eclipse (3.0 is much better then 2.0) and java-libs or I am just doing my first baby steps. So before you can refactor, you will need working tests, which you fully understand. Just to extract an interface because you simply can do it, won't save your day.

So I would like to have an extension, being able to help me out of these messy situations. You can wast much time in such situations, you know. Maybe another extension than the cglib one can be issused for this purpose. Just think about the last newsgroup postings. Half of them were dealing with things like, how to mock classes with non-default constructors or what to do when it comes to legacy code... . There should be a solution for this, too.

My oppinion is still: "The code will tell you, when it smells. You don't need jmock to sniff it." jMock's power lies in the way, you specify your expectations (IMO), not in forcing you to consider advanced designs even before you write a single line. By writing expectations, you will quickly see, if the code smells. Thats when it is most helpful.

"I think this is not a proper argument against it. Just think about any kind of initialisation. Constructor arn't the only ways to construct an object. By using an init method or establish contracts of dependency injection or something similar, you will run in the same situations."

It should never be possible to create an object in an invalid state. The constructor's job is to initialise the state of the object so that the object meets its class invariants. If the constructor does not do that, the class is badly designed. If you need to call a second method to put the object into a valid state, the class is badly designed. If its possible for a subclass to override a method that makes the base-class constructor invalid, the class is badly designed.

>> The constructor's job is to initialise the state of the object so that the object meets its class invariants. If the constructor does not do that, the class is badly designed. <<

So you think any Spring or Eclipse class is badly designed? Any thing working within a container is badly designed? Any component is badly designed? I don't think so. Taking spring for example you mostly have something similar:

class A {
Object dependency;
public void setDependency(Object dependency) { this.dependency=dependency; }
public void afterPropertiesSet() { //check if all requirements (dependencies etc.) are met }
public void doSomeStuff() { //assume to be in a valid state //Can produce much noise if the object is not(!) in a valid state }
}

So this should be bad design? I don't see why I should go inflexible constructors... . Also the metrics look bad. You will end up with non-default constructors with tons of parameters without such a pattern. Seperating initialization and constructor
by applying some generic contracts is a very strong mechanism to create reusable objects. The only thing that is indeed truly needed, is an easy and well known contract. Even like: "before using it, call afterPropertiesSet". It's that simple and powerfull. This is not bad design. I disagree... .

Yes. It would be better to initialse the dependency with a reference to a Null Object, not to a null reference.

>> Yes. It would be better to initialse the dependency with a reference to a Null Object, not to a null reference. <<

The Null Object pattern does not apply here, I think. By using a Null Object as the default value of a property, the created object would be fully functional, even if it is not properly initialized. But it will provide it's functionality and will silently fail. I guess this will be a greater source of disorder than anything else.

Having an object that fails to process some operations/commands/messages, since it was not properly initalized, is one thing. But having an object claiming to act in the right way, but does nothing (operation only affect the Null Objects) will silently fail. The problem will be only noticable, if something goes wrong - It doesn't work like expected). For example, your security layer was not initialized properly and instead of failing, it does not restrict the access of anyone. This is absolutly harmfull in my point of view.

Another way out, would be proper validation. Validate the state of the object (fully initialized?) every time, just before any operation/message/request is performed by the object. That would be an option. But again, forgetting a validation step is also a very likely mistake, so you would end up choosing between many bad alternatives. And so you go for the simple one. Establish a contract, write acceptence tests and move on.

Well except one thing, you may apply the Factory pattern to avoid initialization problems... . Using the Factory pattern is always first duty anyway.

But inside of a modul you may go for it. Also if you only use a certain framework/container your code will incoperate with. This doesn't produce more harm then NullObjects and the validation steps. All this can lead to abuses and bugs. And I think the Null Object pattern is the loser of the pattern. You have to write them, you have to test them and you have to consider them. Tree big problems.

But you may use an implementation that fails everytime it is invoked. That would work, but this is not the Null Object pattern. (Is there any pattern about this?)

Updated test cases to clarify that the CGLIB extension needs patching, to allow mocking of any class that overrides a method called by its superclass constructor, abstract or concrete.

I'd also like to add my vote in support of fixing this problem. I don't know if the resolution would be in CGLib or jMock.

In my first attempted use of the jMock CGLib support, I'm trying to mock javax.naming.InitialContext. My Test class extends org.jmock.cglib.MockObjectTestCase. The specific test fails as soon as it tries to get the Mock instance:

String mockName =
getClass().getName() + '.' + getName() + " mock InitialContext";
// NEXT LINE FAILS
DynamicMock controlIn = mock(InitialContext.class, mockName);

The error looks like this:

net.sf.cglib.core.CodeGenerationException: org.jmock.core.DynamicMockError-->[TestClassName].[testMethodName] mock InitialContext: no match found
Invoked: javax.naming.InitialContext.init(<null>)
Allowed:
No expectations set
at net.sf.cglib.core.ReflectUtils.newInstance(ReflectUtils.java:270)
at net.sf.cglib.core.ReflectUtils.newInstance(ReflectUtils.java:255)
at net.sf.cglib.core.ReflectUtils.newInstance(ReflectUtils.java:251)
at net.sf.cglib.proxy.Enhancer.createUsingReflection(Enhancer.java:388)
at net.sf.cglib.proxy.Enhancer.firstInstance(Enhancer.java:351)
at net.sf.cglib.core.AbstractClassGenerator.create(AbstractClassGenerator.java:193)
at net.sf.cglib.proxy.Enhancer.createHelper(Enhancer.java:330)
at net.sf.cglib.proxy.Enhancer.create(Enhancer.java:246)
at net.sf.cglib.proxy.Enhancer.create(Enhancer.java:403)
at org.jmock.cglib.CGLIBCoreMock.<init>(Unknown Source)
at org.jmock.cglib.CGLIBCoreMock.<init>(Unknown Source)
at org.jmock.cglib.MockObjectTestCase.newCoreMock(Unknown Source)
at org.jmock.MockObjectTestCase.mock(Unknown Source)
[etc ...]

InitialContext has a default constructor which calls several internal methods, none of which are abstract. The first method it calls is the init() method, which is protected, not public. It appears that jMock/CGLib is failing on the init() method, ie: as soon as the default constructor attempts to call another method within the InitialContext class.

I've checked the other problem reports and this one seems closest to the problem I'm experienceing here.

Thanx - Xris

Hi Chris,

I can confirm that the attached patch allows mocking of InitialContext without the problem you experienced.

Brett

It seems that this code is actually calling the constructor of the class I'm trying to mock, which really defeats the whole point of using mocks.

junit.framework.AssertionFailedError: Exception in constructor: testGetSubInstanceIdElements (net.sf.cglib.core.CodeGenerationException: java.net.UnknownHostException-->hostname: hostname
at net.sf.cglib.core.ReflectUtils.newInstance(ReflectUtils.java:235)
at net.sf.cglib.core.ReflectUtils.newInstance(ReflectUtils.java:220)
at net.sf.cglib.proxy.Enhancer.createUsingReflection(Enhancer.java:564)
at net.sf.cglib.proxy.Enhancer.firstInstance(Enhancer.java:493)
at net.sf.cglib.core.AbstractClassGenerator.create(AbstractClassGenerator.java:220)
at net.sf.cglib.proxy.Enhancer.createHelper(Enhancer.java:368)
at net.sf.cglib.proxy.Enhancer.create(Enhancer.java:299)
at com.mkodo.test.CGLibCoreMock.createProxy(CGLibCoreMock.java:54)
at com.mkodo.routing.smpp.test_SMPPTransport.<init>(test_SMPPTransport.java:26)

Whether it is good/bad design, I have run into the issue of having to mock classes that do not have a default constructor. I have put together a fairly simple work-around that seems to be solid.

A simple extension of Mock.

package org.jmock.cglib;

public class UberMock extends org.jmock.Mock {

public UberMock(Class mockedType, Class[] argTypes, Object[] args){ super(new CGLIBCoreUberMock(mockedType, argTypes, args)); }
}

and then a modified version of CGLIBCoreMock.

package org.jmock.cglib;

import java.lang.reflect.Method;
import net.sf.cglib.proxy.Enhancer;
import net.sf.cglib.proxy.MethodInterceptor;
import net.sf.cglib.proxy.MethodProxy;
import org.jmock.core.AbstractDynamicMock;
import org.jmock.core.Invocation;
import org.jmock.core.LIFOInvocationDispatcher;

public class CGLIBCoreUberMock extends AbstractDynamicMock implements MethodInterceptor {

private Enhancer enhancer;
private Class[] argTypes;
private Object[] args;
private Object proxy;

public CGLIBCoreUberMock(Class mockedType, Class[] argTypes, Object[] args){ super(mockedType,mockNameFromClass(mockedType),new LIFOInvocationDispatcher()); this.argTypes = argTypes; this.args = args; this.enhancer = new Enhancer(); enhancer.setSuperclass(mockedType); enhancer.setCallback(this); }

public Object proxy() {
if(proxy == null){ this.proxy = enhancer.create(argTypes, args); }
return(proxy);
}

public Object intercept( Object thisProxy, Method method, Object[] args, MethodProxy superProxy ) throws Throwable { Invocation invocation = new Invocation(proxy, method, args); return mockInvocation(invocation); }
}

Okay, so UberMock is a bad name... it should be LazyMock or something, and I am sure there is room for improvement. I just needed something that worked... and this does.

Hope this helps someone. JMock has been the miracle cure for my unit testing problems.

Jeff: there's no way NOT to call a superclass constructor in Java. That's guaranteed by the language.

The only work-around is to use undocumented features of the JVM implementation that the tests are running on. It's possible to do (e.g. use one of the sun.* packages in Sun's JVM), but is so project-specific that I don't think it should be included in the main jMock distribution.

I have code on my laptop, will check in when I get a chance.

Separated out issue of complex constructors from that of non-default constructors.

Implemented in CVS, will be released as part of v1.1.0.