Added subproject for supporting class loading on Android. #774
Conversation
|
Thanks for opening this. One problem is really test coverage because running the test requires writing instrumented tests that can only be run on an emulator or on a device. I doubt that our current infrastructure supports this. I ran a successful test yesterday night on my own device. In contrast to Mockito-Dexmaker I also managed to add automatic support for modern API versions where the previous automatic discovery did no longer work. |
Current coverage is 86.55% (diff: 68.00%)
|
|
I will take a look at the Travis configuration to build on Travis :)
…On Thu, 24 Nov 2016, 09:57 Codecov, ***@***.***> wrote:
Current coverage <https://codecov.io/gh/mockito/mockito/pull/774?src=pr>
is 86.54% (diff: 68.00%)
No coverage report found for *release/2.x* at b904694
<b904694>
.
Powered by Codecov <https://codecov.io?src=pr>. Last update
b904694...99f0fb8
<https://codecov.io/gh/mockito/mockito/compare/b904694ca3dd8ffe7879885d8197fb59f88c9bb3...99f0fb89bc3927ab399625a6137f659979d19aec?src=pr>
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#774 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AFrDb-r6tQDLUfSpxRBSBFKa3Fr0NI4Vks5rBVFwgaJpZM4K7aCd>
.
|
|
Great, thank you. As I said before, ideally, I would like the subproject to follow the version numbers of |
|
What if we create another project on the mockito organisation, that can have a different lifecycle and travis configuration ? |
|
Yes that was my thinking too, but I do want to test out if we can avoid the
project. Have to test it out on Travis :)
…On Thu, 24 Nov 2016, 11:12 Brice Dutheil, ***@***.***> wrote:
What if we create another project on the mockito org, that can have a
different lifecycle and travis configuration ?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#774 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AFrDb0rMHn5K7eUubwo2FTdq4wy9El-xks5rBWL2gaJpZM4K7aCd>
.
|
|
Curious, what's driving this effort? E.g. why to replace dexmaker?
Great idea!!!
|
|
Dexmaker does no longer work out of the box with newer android versions and its not compatible with mockito 2. Also, the project seems dead. |
|
I will remove the Travis config and try to fix publication of these artifacts. Not sure how I would be able to test these though... |
|
@szczepiq could you take a look at the above commit and confirm this correctly publishes 2 artifacts to bintray with separate artifact-id's? Then we can merge this PR 😄 |
|
Sure, I'll look into it by the end of this week! |
| @@ -9,7 +9,7 @@ bintray { | |||
| user = 'szczepiq' | |||
| key = System.env.MOCKITO_BINTRAY_API_KEY | |||
|
|
|||
| publications = ['mockitoCore'] | |||
| publications = ['mockitoCore', 'mockitoAndroid'] | |||
There was a problem hiding this comment.
I don't think this works. Did you test it? 'mockitoAndroid' is a publication in a different Gradle subproject.
| @@ -1,3 +1,5 @@ | |||
| include 'android' | |||
| include 'inline' | |||
There was a problem hiding this comment.
'inline' artifact, too? :)
What is the strategy for 'inline' artifact? I'd rather mockito worked out of the box for final classes (or have toggles in the API). I don't like the idea of separate mockito-inline artifact.
|
FYI: dexmaker is alive - group of devs acquired the ownership and are planning to ship new versions with bugfixes. @TimvdLippe, the publication will not work. I guess you didn't have a clean way to test this. I will try to fix this so that the release procedure is testable locally. @bric3, @raphw, this should be a separate project. Consistent version scheme makes sense from the standpoint of simplicity. However, it introduces awkward versioning: what if there is a change in 'android' project without a change in mockito project. In this scenario do we publish new version of mockito-core without any changes to its code? Also, keeping it in the same project prevents us from having different release cadence that everybody would like to try out (e.g. pushing less frequently ;). Also, it prevents us from having different contributors for the project. Same project does have it's charm :) It would automatically publish new version of mockito-android everytime there is a new version of mockito-core. However, we could probably get this working using Travis build requests. I suggest we push forward the separate project idea. I will update the release plugin so that it dead simple to reuse. |
There was a problem hiding this comment.
Content seems fine. @szczepiq when you have fixed the release procedure you can merge this PR 😄
|
I disagree with the "different projects" assertion. The For now, I think that simply publishing the artifacts with a common version would be the most pragmatic solution. A lot of libraries are published like this to allow reusing of a single version number and in order to keep the publication process atomic. |
While dexmaker is being restarted at linkedin I don't see it as a problem to have a concurrent implementation, Byte buddy supports dex since a long time already. Choice is always good. And both mockmaker project may help each other if needed ;)
@raphw For tracking purpose I'd like to have a separate PR for the inline artifact. We can discuss over there whether or not we want it. I'm not in favor of this idea, however I'm not against it. |
| "or", | ||
| "getInstrumentation().getTargetContext().getCacheDir().getPath()" | ||
| )); | ||
| } |
There was a problem hiding this comment.
Since the exception contain implementation details of AndroidTempFileLocator I think it should be moved in this class to have the whole responsability of the target location including failure.
| public Object invoke(Object proxy, Method method, Object[] args) throws Throwable { | ||
| throw new IllegalStateException("Could not initialize plugin: " + pluginType, t); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Isn't it covered by loadImpl ?
Shall we instead catch something larger than Exception there ?
|
Fun fact : since I'm the one to have opened the PR I cannot request changes... |
|
Hum, I don't know why the build failed |
| artifact javadocJar | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The publishing declaration here fail, not sure why https://scans.gradle.com/s/7myhdoeojp3wi/failure
as it does look correct.
|
I plan to finish off #809 tomorrow and then we will have release capability of other jars. |
|
@raphw do you have time to update this PR with the new publication method? Else I will try to make it work soonTM. |
|
Hei Tim, I am quite busy with Byte Buddy Java 9 support these days so I would prefer if you find the time. Appreciate it! |
|
@raphw All right will try to take a look! |
|
I fixed compilation, but it is now failing on generating Javadoc for the Android subproject: In a similar fashion to the |
| "Failed to load " + pluginType, e); | ||
| } | ||
| } catch (final Throwable t) { | ||
| return (T) Proxy.newProxyInstance(pluginType.getClassLoader(), |
There was a problem hiding this comment.
Why do we return proxy instead of failing?
There was a problem hiding this comment.
Because throwing exceptions from initializers yield strange exceptions where the cause for the failure is no longer apparent but NoClassDefFoundErrors are shown. This results in much better errors where the cause is visible.
There was a problem hiding this comment.
I fell over this myself when dealing with a robolectrics incompatibility. Took me hours to understand this link.
This is a work from @raphw to bring mockito to android devices without dexmaker, using ByteBuddy Android instead.