Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce animal sniffer #2006

Merged
merged 2 commits into from Aug 18, 2020
Merged

Introduce animal sniffer #2006

merged 2 commits into from Aug 18, 2020

Conversation

raphw
Copy link
Member

@raphw raphw commented Aug 17, 2020

Introduces animal sniffer with exclusion of inline-mock-maker classes which would never be present on Android. Avoids calling invoke/invokeExact methods of handles directly but rather puts the invocations into generated code to avoid breaking Android builds.

…ude classes which must not be run on Android.
…id breaking Android builds which cannot process these methods.
Copy link
Contributor

@TimvdLippe TimvdLippe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL about animalsniffer. Really nice to prevent these issues!

@@ -98,6 +99,14 @@ dependencies {
testRuntime configurations.compileOnly

testUtil sourceSets.test.output

signature 'org.codehaus.mojo.signature:java18:1.0@signature'
signature 'net.sf.androidscents.signature:android-api-level-24:7.0_r2@signature'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So for my understanding, this says that we are supporting Android API levels 24 and over? If so, we should document that somewhere as well. In the Twitter thread, they proposed level 21 and over. If we set this to 21, do we need to make a lot of changes? Iirc, Mockito mostly builds in Google so I think we would be able to do so?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should, indeed. Android level 21 does not support types such as Optional or Function. We can go that far but it's already part of the public API. I guess 3.5.0 is not super-wide spread yet but I'd like to avoid releasing 4.0 just for that.

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, iirc Mockito 3.4.0 builds just fine internally and also includes Optional. I can take a look tomorrow as to why that works.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks though, that Android can easily desugar these few types: https://developer.android.com/studio/write/java8-support - it only does not work with method handle types which are not exposed. I just asked for a validation of this branch of the user who reported this. By requiring API level 24, we do not leak API that cannot be desugared at least. But yes, we should document this!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, with code shrinking, it should already work with 3.5.0: https://developer.android.com/studio/build/shrink-code#shrink-code

@codecov-commenter
Copy link

codecov-commenter commented Aug 17, 2020

Codecov Report

Merging #2006 into release/3.x will increase coverage by 0.04%.
The diff coverage is 77.77%.

Impacted file tree graph

@@                Coverage Diff                @@
##             release/3.x    #2006      +/-   ##
=================================================
+ Coverage          84.86%   84.90%   +0.04%     
  Complexity          2692     2692              
=================================================
  Files                324      324              
  Lines               8146     8161      +15     
  Branches             972      972              
=================================================
+ Hits                6913     6929      +16     
+ Misses               965      964       -1     
  Partials             268      268              
Impacted Files Coverage Δ Complexity Δ
...in/java/org/mockito/internal/MockedStaticImpl.java 78.66% <0.00%> (ø) 11.00 <0.00> (ø)
...al/creation/bytebuddy/InlineBytecodeGenerator.java 92.50% <ø> (ø) 37.00 <0.00> (ø)
...util/reflection/InstrumentationMemberAccessor.java 81.44% <77.50%> (+2.11%) 21.00 <1.00> (ø)
...l/creation/bytebuddy/InlineByteBuddyMockMaker.java 68.18% <84.61%> (ø) 45.00 <0.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 70633c4...ec659f4. Read the comment docs.

@TimvdLippe
Copy link
Contributor

@raphw Can we publish a new release to Maven central for #2007?

@raphw
Copy link
Member Author

raphw commented Aug 18, 2020

Doesn't Android consume JCenter?

@darakeon
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants