Closed GoogleCodeExporter closed 8 years ago
Can you clarify why this is a Mockito issue? If it's not easy to run JUnit
tests on Android, I'm not sure how the Mockito team can fix this. Are you
saying you want a version of Mockito that works with vogar tests? Or are you
asking to convert our unit tests for Mockito into something different? Please
make it clearer what you're asking for.
Original comment by dmwallace.nz
on 13 Jan 2012 at 3:48
I'm sorry for being confusing!
Desktop Java uses a JVM that fundamentally operates on .class files. Android
uses a Dalvik virtual machine that does not use .class files. Instead Android
uses .dex files which are similar to .class files, except that one .dex file
can describe the contents of many types. They're a bit more efficient!
Since Mockito uses cglib behind-the-scenes, and cglib can only generate .class
files, there is no way to run Mockito on Android.
This change adds a new backend to Mockito called "dexmaker" as an alternative
to cglib. Dexmaker generates .dex files and works just fine on Android.
If the Mockito team would rather not have to maintain internal Android support,
please let me know. If that's the case I'll create a separate project that
exists to port mockito to Android.
Original comment by jessewil...@google.com
on 13 Jan 2012 at 4:02
The major patch is here:
http://code.google.com/r/jessewilson-mockito/source/detail?r=805e98fd4190acbbdaa
b87172c31b8dc0b78236e
Original comment by jessewil...@google.com
on 13 Jan 2012 at 4:03
OK, understood. I'm not sure whether anyone in the Mockito team would be
interested in maintaining Android support; I'm not, but I can't speak for the
entire team. Watch this space.
Original comment by dmwallace.nz
on 13 Jan 2012 at 4:18
Hi Jesse,
This is great work. However I don't think anybody in the team actually works in
the Android environment, so we are clearly not proficient in this area. You are
probably the expert here ;)
Also, for some time we have bean thinking to rearchitecture the backend to
allow things such as changing the bytecode engine. I already started research
on this matter. But free time is scarce these past months, so this research
didn't progressed at an acceptable rate (in my opinion).
However your port is interesting because it is not just about changing the
bytecode engine, but to support another platform.
(Thinking loudly) When this backend stuff will be mature enough, it could be
possible to create a project that will run the test using Vogar with Dalvik,
and on the Mockito side you could set internal settings to use Android support.
Maybe with a DefaultInternalConfiguration.
Regards,
Brice
Original comment by brice.du...@gmail.com
on 13 Jan 2012 at 9:41
Would you guys be willing to accept a different patch that made the codegen
component pluggable? I'm imagining it taking the form of a simple interface in
Mockito's internal package:
public interface MockMaker {
T newMock(Class<T> classToMock, MockitoInvocationHandler handler,
Class<?>[] ancillaryTypes, ClassLoader loader);
MockitoInvocationHandler getHandler(Object mock);
}
plus a helper class:
public class MockMakerFactory {
/**
* Returns the best mock maker for this runtime.
*/
public static MockMaker get() {
// inspect the classpath, looking for a MockMaker implementation
// this will use cglib+asm on the JVM and dexmaker on Android
}
}
That way I can ship a small 'mockito-android.jar' file that provides the
MockMaker implementation only. Android applications would just need to make
sure that .jar is on their classpath for things to work.
Original comment by jessewil...@google.com
on 13 Jan 2012 at 7:57
One advantage of this pluggable codegen approach is that Mockito's build and
test execution won't be affected, since there isn't any Android-specific code.
Original comment by tball@google.com
on 13 Jan 2012 at 8:19
@Tom Yes that would be definitely a part of the target.
@Jesse You can hack something, but it is not sure it will be integrated as is,
i.e. we need to think how to expose a good internal API, not a clunky one.
However it will definitely be a good starting point, it'll help to fathom
implications here and there. Also Android support is a bit special as it
requires to modify other objects such as the BeanPropertySetter.
Original comment by brice.du...@gmail.com
on 16 Jan 2012 at 8:51
1. @Jesse, so with the patch you presented do all mockito tests pass on dalvik?
If so that's pretty awsome.
2. The pluggability. We already have 'some' infrastructure to make it happen.
It's very simple as we didn't use it much and I think very little users (if
any) actually take advantage of it. However, it might a good start. Take a look
at the documentation of the IMockitoConfiguration. In theory, you could place
the implementation of that interface in your jar, in a correct package and
Mockito will use it. Trouble is that IMockitoConfiguration does not have any
means of configuring the algorithm that creates mocks. This is where an eager
contributor @Jesse comes in :D It would be very cool if you could come up with
some configurability there, for example:
IMockitoConfiguration {
MockMaker getMockMaker(); //or something like that.
}
I'd love to pull in such patch!
I looked at the codebase yesterday to find out if it's easy to unbake the cglib
stuff from Mockito. It didn't look easy enough for a late Sunday :/
Unfortunately, cglib stuff is in Mockito since day 1, the time when I was
focused on the fluent mocking DSL rather than on configurability. Good luck and
feel free to ask / reach out for our help.
Thanks a lot for the Dalvik team for reaching out! @David, @Brice, let's help
out as much as we can so that we can offer mockito to android devs.
Cheers :)
Original comment by szcze...@gradle.biz
on 16 Jan 2012 at 3:13
Yeah, the tests pass on Dalvik.
I took a look at IMockitoConfiguration, and it's the right idea but the wrong
interface. The main problem is that Android's implementation would also need to
define things like the AnnotationEngine, for which we don't care. This would
also prevent a mix of configuration options, like Android's MockMaker but my
application's own AnnotationEngine.
But we're really close; perhaps we just want a second interface for just the
cglib/dexmaker difference. Perhaps we'd just add a second method to
ClassPathLoader
public IMockMaker loadMockMaker() {
...
}
From my studying, Cglib is only used in a few places. It's straightforward to
encapsulate all of Mockito's use in an IMockMaker interface.
The other changes I made were to fix execution against classes not included in
Android's API (BufferedImage and BeanProperty). We can fix this on the Android
side also, by including Apache Harmony's implementations of these classes in
the Android plugin jar. This is an easy fix. The slightly less complicated fix
is to just avoid these APIs in Mockito, but I'm happy to do whatever your team
prefers.
I'll put together a quick change that adds the IMockMaker interface. It'll
probably be easier to discuss the consequences by looking at real code!
Thanks a lot guys!
Original comment by limpbizkit
on 16 Jan 2012 at 3:45
Very cool.
Yeah, we realize the limitations of IMockitoConfiguration but I though it might
be a good start (there's a public implementation DefaultMockitoConfiguration
that can be used to avoid configuring options you don't care about). However,
IMockitoConfiguration has some fundamental limitations so if you want to
introduce a new smartly loadable type (MockMaker) that's fine. We can sort out
the pluggability properly a bit later so that we don't hold up the development
of the feature.
I need to think about this pluggability a bit more. Perhaps all extension
points should just be well known interfaces that mockito will try to find in a
well known location/package. Perhaps there should be something in the top level
API to declare a plugin that is used in a given test. Will see :)
>From my studying, Cglib is only used in a few places. It's straightforward to
encapsulate all of Mockito's use in an IMockMaker interface.
If you could offer that as a contribution that would be most awesome!
Original comment by szcze...@gradle.biz
on 16 Jan 2012 at 5:07
Agreed.
This is already a feat to have achieved that compatibility, but we need to
think things beyond :P
Original comment by brice.du...@gmail.com
on 16 Jan 2012 at 6:58
Pull request! Here's a branch that adds the IMockMaker interface. All of the
commits in this branch pass all of the Mockito tests when run on a JVM.
http://code.google.com/r/jessewilson-mockito/source/list?name=imockmaker
Commit #1 is a simple refactoring. It gets basic mocking working on Android
when coupled with my private mockito-android.jar file.
http://code.google.com/r/jessewilson-mockito/source/detail?r=2865ef9cb396d41d336
de93340e6e012f2837335&name=imockmaker
Commit #2 fixes ReturnSmartNulls.
http://code.google.com/r/jessewilson-mockito/source/detail?r=884fa735e656a817b6a
2796fee88195abb5fcd11&name=imockmaker
Commit #3 changes a test's modifiers from package-private to private so that
test passes on Android.
http://code.google.com/r/jessewilson-mockito/source/detail?r=3c5bbfdb9670abeed29
84d7585b9364f2a89c8c3&name=imockmaker
Tests still broken on Android:
- bean properties
- named mocks (anything that requests the name will just get 'null')
- cglib-specific tests (ClassImposterizerTest, CGLIBHackerTest, a few in MockUtilTest)
The attached .jar file can be used to run the mockito tests on an Android
device. Putting that .jar on the class path is sufficient; ClassPathLoader will
find it.
My personal preference is to commit this stuff early and then iterate on the
API between Mockito and the Android extension. This can remain an internal API
or become a public API as you prefer. But the sooner you guys can take these
patches the sooner we can start running Mockito on our phones!
Original comment by jessewil...@google.com
on 16 Jan 2012 at 9:28
Attachments:
Cool thx, I'll take a look as soon as possible :)
Original comment by brice.du...@gmail.com
on 17 Jan 2012 at 8:36
Just want to say that having Mockito on Android would be a true game changer
for Android developers.
Thrilled to see progress on this.
Original comment by matth...@qype.com
on 23 Jan 2012 at 3:49
+1 on this. Would be happy to help
Original comment by charroch
on 27 Jan 2012 at 5:22
Ok. I'm starting to work on it. Hopefully it's going to be officially pulled
shortly :)
Original comment by szcze...@gmail.com
on 29 Jan 2012 at 10:25
I pushed the changes. There's some things pending still (the bean setter, jvm5
runnability, rename IMockMacker -> MockMacker). I'll do those changes later
today :) This is good stuff Jesse, thanks a lot!!!
Original comment by szcze...@gmail.com
on 29 Jan 2012 at 12:02
@Jesse, question: do you really need access to MockitoInvocationHandler? I'd
like to make the MockMaker simpler, e.g. having only:
<T> T createMock(Class<T> typeToMock, Class<?>[] extraInterfaces,
MockSettingsInfo settings);
void resetMock(Object mock, MockSettingsInfo settings);
That would make things simpler, we wouldn't have to expose
MockitoInvocationHandler and consequently the Invocation object.
I didn't have time to complete all stuff I wanted. Will give it a spin next
weekend. Here're the things pending:
1. bean setter stuff. This is quite easy to implement - simply not use this
offending introspector, etc.
2. add coverage for the custom mock maker
3. More work around MockitoInvocationHandler and Invocation interface (not
existing yet). That depends on the answer to the question above.
Original comment by szcze...@gmail.com
on 29 Jan 2012 at 6:38
Awesome, thanks for your work.
I think it needs MockitoInvocationHandler. With the createMock() method you've
proposed, I don't know how we'd handle invocations! We need some mechanism to
tell Mockito that a method was invoked, what its arguments are, how to call
super, and then return a value. MockitoInvocationHandler is pretty near perfect
for that.
Original comment by jessewil...@google.com
on 29 Jan 2012 at 9:21
Ah, yeah of course :)
I guess in future we will figure out some way of communicating in case we need
to tweak something. Are you going to host the AndroidMockMaker on googlecode,
too?
Here're the things that are likely to change on our end:
1. MockitoInvocationHandler leaks the Invocation object which at the moment is
an internal concrete class. We will change it into an interface and place it in
some public package.
2. The plugin loading logic might change. I still want it to be seamless, e.g.
no explicit configuration needed in case the right jar happens to be on the
classpath. I still want to be at (or close to) the convention laid out by
java's ServiceLoader.
Original comment by szcze...@gmail.com
on 29 Jan 2012 at 9:38
Yeah, I'll host AndroidMockMaker on googlecode. I'll set up a new, clean
repository that's forked from Mockito's mercurial repo.
Original comment by jessewil...@google.com
on 29 Jan 2012 at 9:46
Hi all, that is even more great news :)
About the Bean Introspector, we shall be carful as this class is supposed to be
the standard access to Bean Properties, if we replace it it should be compliant
with all the rules that define Properties in a Bean.
For example Spring is actually using this class.
Original comment by brice.du...@gmail.com
on 30 Jan 2012 at 8:29
>About the Bean Introspector
Yeah. It shouldn't be very difficult just calling the setter and comply with
most of the rules (or the rules important to us and our users).
Original comment by szcze...@gmail.com
on 30 Jan 2012 at 8:47
Agreed, I just wanted to make sure it's not forgotten ;)
@Jesse By the way on Android, you probably use or have some equivalent to the
Bean convention and also the related utility classes?
Original comment by brice.du...@gmail.com
on 30 Jan 2012 at 9:22
Android doesn't have much of a java.beans package. The JDK has 38 types in its
java.beans; Android has only 4.
But it's a relatively trivial amount of code to replace PropertyDescriptor. We
can even include Apache Harmony's copy of it in android-mockito.jar if we want
strict API compatibility.
http://svn.apache.org/viewvc/harmony/enhanced/java/trunk/classlib/modules/beans/
src/main/java/java/beans/PropertyDescriptor.java?view=markup
My personal preference is for Mockito to shed its dependency on the java.beans
package and instead use a small amount of reflective code. But you can go
either way and it's not much work for us to track that.
Original comment by jessewil...@google.com
on 30 Jan 2012 at 3:54
@Jesse Granted it's not that much and if java.beans don't exists on Android
then we should implement it ourselves.
All in all I just wanted to careful on that matter. A few years ago I remember
frameworks that didn't respect the Bean property scheme correctly which caused
mapping problems. For example what is the property name's case of the setter
"setURL(...)" ?
Original comment by brice.du...@gmail.com
on 30 Jan 2012 at 4:33
Yeah, good point. Test coverage is what we'll need there.
Original comment by jessewil...@google.com
on 30 Jan 2012 at 4:36
Yeah, good point. Test coverage is what we'll need there.
Original comment by jessewil...@google.com
on 30 Jan 2012 at 4:36
@Brice All the Java Beans introspection magic is in the java.beans.Introsepctor
class. To answer your specific question, its decapitalize() method has the
following doc-comment:
* Utility method to take a string and convert it to normal Java variable
* name capitalization. This normally means converting the first
* character from upper case to lower case, but in the (unusual) special
* case when there is more than one character and both the first and
* second characters are upper case, we leave it alone.
* <p>
* Thus "FooBah" becomes "fooBah" and "X" becomes "x", but "URL" stays
* as "URL".
Original comment by tball@google.com
on 30 Jan 2012 at 5:34
Yup I know since a long time about all these property (de)capitalization rules
:)
I just raised the question as an example to make sure we don't forgot that. I
guess this specific matter has been well discussed then :)
Anyway thx for the Javadoc pointer.
Original comment by brice.du...@gmail.com
on 30 Jan 2012 at 5:47
We're almost there. There are currently only two steps to running Mockito tests
on Android:
1. Get dexmaker.jar. For now I've decided it's simple and easy to just embed an
Android implementation of MockMaker in the main dexmaker.jar. You can get this
here:
http://code.google.com/p/dexmaker/
2. Replace Mockito's
src/org/mockito/internal/util/reflection/BeanPropertySetter.java with an
Android-compatible version, like this one:
http://code.google.com/r/jessewilson-mockito/source/browse/src/org/mockito/inter
nal/util/reflection/BeanPropertySetter.java
To run Mockito's own tests you need one more change. Fest depends on
java.awt.image.BufferedImage, a class not available on Android. We don't
actually exercise this code so you can drop in any class with this name. I've
been using an empty class; available here:
http://code.google.com/r/jessewilson-mockito/source/browse/lib/test/
Mockito folks: if you can break the dependency on BeanPropertySetter then
you'll be Android-capable without modification. I think that's pretty awesome.
Original comment by jessewil...@google.com
on 30 Jan 2012 at 11:44
Just a reminder as the code is using a ServiceLoader. This class is only
available in JDK 6. And mockito is also targeting JDK 5 users. We can't use it
as the default implementation in ClassPathLoader.
See comment here
http://code.google.com/p/mockito/source/browse/src/org/mockito/internal/configur
ation/ClassPathLoader.java?spec=svn2d74de36c814b8e240ef94e5d0192cd11277cdf5&r=2d
74de36c814b8e240ef94e5d0192cd11277cdf5#7
In other news I commited a revised BeanPropertyStter that don't use
Introspector.
Original comment by brice.du...@gmail.com
on 31 Jan 2012 at 5:00
I'm happy to contribute an alternate implementation of ClassPathLoader that
doesn't use JDK 6's ServiceLoader. I'll put together something today.
Original comment by jessewil...@google.com
on 31 Jan 2012 at 5:22
@jesse For now I think the code use a simple Class.forName. I think that's the
best thing for now.
Original comment by brice.du...@gmail.com
on 31 Jan 2012 at 5:32
@Jesse Also don't worry, I think ClassPathLoader might need some refactoring.
So we might change stuff here and there. Thank you so much for your
contribution anyway :)
Original comment by brice.du...@gmail.com
on 31 Jan 2012 at 5:47
Yeah, calling into ServiceLoader reflectively is a good idea. We don't need to
support Mockito+Android on anything lower than the Java 6 APIs.
Original comment by jessewil...@google.com
on 31 Jan 2012 at 6:57
>We don't need to support Mockito+Android on anything lower than the Java 6
APIs.
Agreed. However, if the general capability of plugging own MockMaker only works
for jdk6+ then that is a bit awkward. @Jesse if you have some good ideas about
not using ServiceRegistry we would appreciate a patch - we might use it or
inspire on it :)
I think down the road we need something much better than ServiceLoader, e.g.
here are the use cases:
- it needs to be testable (ServiceLoader caches eagerly which might be a
problem)
- it needs to behave if multiple services are found (e.g. ServiceLoader just
uses the first one on path). At the least it should warn/fail. Maybe it should
support multiple services (although it does not make much sense for the mock
maker).
Original comment by szcze...@gmail.com
on 1 Feb 2012 at 2:13
I'll put together a patch then!
Original comment by jessewil...@google.com
on 1 Feb 2012 at 3:36
Here's a change that gives us ServiceLoader compatibility without depending on
ServiceLoader and JDK 6:
http://code.google.com/r/jessewilson-mockito/source/detail?r=fa42d5e193d003e47753f4268ed9c2bb2e2ec317
This passes tests on both Android and the JDK.
Original comment by jessewil...@google.com
on 3 Feb 2012 at 4:17
Hey thx Jesse, your implementation looks like what we've been discussing with
Szczepan the other day.
However I would favor the approach of using the 'getResource' instead of the
'getResources' call because this actually scan the whole classpath. This have
the same performance drawback as ServiceLoader. On simple projects it is not an
issue, but on larger ones this could be become an issue. Well I don't feel
comfortable enough on this matter.
That's why I propose that Mocklito should only use the first implementation on
the classpath / classloader. And using the 'getResouce' call still allows
plugability classloader wise.
Thinking about that I suppose it would be clever to cache the implementation if
already found in the current ClassLoader. What do you think ?
Original comment by brice.du...@gmail.com
on 3 Feb 2012 at 7:21
Since we're only using the first resource anyway, using getResource() is a good
idea. Would you like to make that change? It should be quite straightforward.
My patch caches the MockMaker in a static field; I think that's all the caching
you'll need.
Original comment by jessewil...@google.com
on 3 Feb 2012 at 7:32
It's the end of the day and the week-end begins here in Paris.
Yep go for the change, we'll pull your code :)
> My patch caches the MockMaker in a static field; I think that's all the
caching you'll need.
Oh yeah right forgot that fact, while reading your code this morning on my
phone ;)
Though I wonder if that's ok if Mockito is executing in a child classloaders.
But maybe that's over-engineering at the present time.
Original comment by brice.du...@gmail.com
on 3 Feb 2012 at 7:44
This changes ClassPathLoader to fetch only one resource:
http://code.google.com/r/jessewilson-mockito/source/detail?r=a696b446f9ad47411dc
ed28f6247acc8de89be78
Original comment by jessewil...@google.com
on 5 Feb 2012 at 8:32
Cool, thx Jesse :)
Original comment by brice.du...@gmail.com
on 5 Feb 2012 at 8:37
Friendly ping!
Original comment by jessewil...@google.com
on 21 Feb 2012 at 9:15
Hi Jesse, sorry we didn't had the time to actually make things happen lately.
But I will have some time to spend on mockito soon, so you can expect some
feedback :)
Original comment by brice.du...@gmail.com
on 22 Feb 2012 at 5:47
[deleted comment]
Could be problematic to keep the MockMaker configuration in the META-INF folder
as the below extract from the APK builder shows:
http://source-android.frandroid.com/sdk/sdkmanager/libs/sdklib/src/com/android/s
dklib/internal/build/SignedJarBuilder.java
// do not take directories or anything inside a potential META-INF folder.
if (entry.isDirectory() || name.startsWith("META-INF/")) {
continue;
}
Original comment by charroch
on 28 Feb 2012 at 2:36
That would be problematic indeed. Jesse do you confirm ?
I don't know much about APK builder or APK, but do Android devs need to build
the package to use mockito (at least while developping).
Original comment by brice.du...@gmail.com
on 29 Feb 2012 at 11:59
Original issue reported on code.google.com by
jessewil...@google.com
on 13 Jan 2012 at 3:38