Fixes #1997 : make mockito-junit-jupiter workin in OSGiMockitoExtensi…

[rotty3000]

…on

without
a) giving up the internal nature of some packages
b) resorting to substandard solutions like OSGi fragements

Signed-off-by: Raymond Augé raymond.auge@liferay.com

Hey,

Thanks for the contribution, this is awesome.
As you may have read, project members have somehow an opinionated view on what and how should be
Mockito, e.g. we don't want mockito to be a feature bloat.
There may be a thorough review, with feedback -> code change loop.

Which branch :

• On mockito 3.x, make your pull request target release/3.x
• On mockito 2.x, make your pull request target release/2.x (2.x is in maintenance mode)

This block can be removed
_Something wrong in the template fix it here .github/PULL_REQUEST_TEMPLATE.md

check list

• Read the contributing guide
• PR should be motivated, i.e. what does it fix, why, and if relevant how
• If possible / relevant include an example in the description, that could help all readers
  including project members to get a better picture of the change
• Avoid other runtime dependencies
• Meaningful commit history ; intention is important please rebase your commit history so that each
  commit is meaningful and help the people that will explore a change in 2 years
• The pull request follows coding style
• Mention Fixes #<issue number> in the description if relevant
• At least one commit should mention Fixes #<issue number> if relevant

[codecov-commenter]

Codecov Report

Merging #2047 into release/3.x will increase coverage by 0.02%.
The diff coverage is 0.00%.

@@                Coverage Diff                @@
##             release/3.x    #2047      +/-   ##
=================================================
+ Coverage          84.89%   84.92%   +0.02%     
  Complexity          2704     2704              
=================================================
  Files                325      325              
  Lines               8204     8204              
  Branches             979      979              
=================================================
+ Hits                6965     6967       +2     
  Misses               968      968              
+ Partials             271      269       -2     

Impacted Files	Coverage Δ	Complexity Δ
...ernal/configuration/CaptorAnnotationProcessor.java	100.00% <ø> (ø)	3.00 <0.00> (ø)
...ockito/internal/configuration/ClassPathLoader.java	54.54% <ø> (ø)	2.00 <0.00> (ø)
...internal/configuration/DefaultInjectionEngine.java	100.00% <ø> (ø)	2.00 <0.00> (ø)
...to/internal/configuration/GlobalConfiguration.java	100.00% <ø> (ø)	13.00 <0.00> (ø)
...nal/configuration/IndependentAnnotationEngine.java	85.36% <ø> (ø)	14.00 <0.00> (ø)
...ernal/configuration/InjectingAnnotationEngine.java	89.13% <ø> (ø)	14.00 <0.00> (ø)
...nternal/configuration/MockAnnotationProcessor.java	96.96% <ø> (ø)	13.00 <0.00> (ø)
...to/internal/configuration/SpyAnnotationEngine.java	98.41% <ø> (ø)	24.00 <0.00> (ø)
...ockito/internal/configuration/plugins/Plugins.java	88.88% <ø> (ø)	8.00 <0.00> (ø)
...internal/session/DefaultMockitoSessionBuilder.java	96.15% <ø> (ø)	14.00 <0.00> (ø)
... and 14 more

---

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 b6ae6cf...65c3bb9. Read the comment docs.

[TimvdLippe]

I understand the usage of the apiguardian, but I am not thrilled of adding another compile dependency to Mockito. I am going to ponder on it a bit to understand why it is used and whether there are alternative solutions available.

[TimvdLippe]

It's not clear to me why this part was removed and what changed it. Could you please add a comment to the functionality that replaces it to explain why we need it? (E.g. reproducible jars) And could you update your commit message to explain the change that was made.

[rotty3000]

bnd has this feature builtin! I enabled it by adding -noextraheaders: true rendering this code obsolete.

[rotty3000]

if you diff the results before and after (which I did to double check) you will see what I mean :)

[TimvdLippe]

Could you add an example in the comment what would be invalid?

[rotty3000]

An error will cause the build to fail with something like this:

> Task :junit-jupiter:verifyOSGi FAILED
Resolution failed. Capabilities satisfying the following requirements could not be found:
    [<<INITIAL>>]
      ⇒ osgi.identity: (osgi.identity=org.mockito.junit-jupiter)
          ⇒ [org.mockito.junit-jupiter version=3.5.11]
              ⇒ osgi.wiring.package: (osgi.wiring.package=com.foo)

I triggered this failure by manually importing a fake package com.foo (to replicate some package which cannot be satisfied by the available runtime dependencies) which is the same effect for any change that would inadvertently break the OSGi support.
If such a failure ever happens, you can ping me and I would be more than happy to help track down the reason (though I think it might become obvious to you pretty quickly :) )

[rotty3000]

@TimvdLippe we could avoid using apiguardian if you prefer.

There are three main options:

• OSGi fragment
• manual definition of "friend" packages
• use apiguardian

Let's consider the differences that would be involved:
OSGi fragments
-- simplest solution
-- worst OSGi experience (because there is an ordering problem, most likely to produce issues that people don't understand)
-- all you need to do is mark the mockito-junit-jupiter bundle as being a fragment of mockito-core (one liner)
manual definition of "friend" packages
-- hardest solution (the idea is a parallel of qualified exports in JPMS)
-- really good OSGi experience with predictable behaviours
-- need to manage packages you want to make "qualified exports" to mockito-junit-bundle in a manual way which means adding package attributes to exports/imports of both bundles which will be error prone, won't have any tooling support and painful for even the most experienced OSGi dev.
apiguardian
-- middle of the road difficulty
-- same solution as manual definition of "friend" packages but managed with a bnd plugin that does the heavy lifting on both sides (i.e. adds the required metadata)
-- really good OSGi experience with predictable behaviours
-- requires some java class level metadata (@API) to reflect the intent of certain classes/packages

HTH a little.

[rotty3000]

whichever option you choose I can refactor the PR into... it's not a problem. I immediately opted to go with the most ideal solution from the OSGi perspective. but I understand if you opt to chose something less invasive! :)

[TimvdLippe]

Thanks for the explanations! I will think a bit about it and then get back to you 😄

[TimvdLippe]

I have filed apiguardian-team/apiguardian#19 to explore adding the annotation on a package statement. Then we would only need to add it to the package-info.java of org.mockito.internal. Let's see how that discussion develops.

[timothyjward]

@rotty3000 Do you think it would be worth reconsidering the fragment approach?

OSGi fragments
-- simplest solution
-- worst OSGi experience (because there is an ordering problem, most likely to produce issues that people don't understand)
-- all you need to do is mark the mockito-junit-jupiter bundle as being a fragment of mockito-core (one liner)

I'm not sure that I agree that being a fragment would give a poor OSGi experience. The Mockito JUnit 5 "bundle" contains types that are explicitly referenced and used in test classes in an "API" package, and then those make use of internal packages from Mockito. It's not like JUnit 5, where I need a bunch of types that I don't have a direct dependency chain to (e.g. the JUnit engine and launcher).

Essentially if I make use of @MockitoSettings or @ExtendWith(MockitoExtension.class) then my test class file has a hard link to org.mockito.junit.jupiter, then as that package comes from a fragment it would have a hard link to mockito-core (which I probably also have a hard link to by using @Mock or Mockito. I'm not sure where the possible ordering problem comes in for this case.

While I'm sure the enhanced API Guardian support will be useful in the future I am keen to see this issue fixed in Mockito sooner if possible.

[TimvdLippe]

@rotty3000 Could you give an update on this PR?

[rotty3000]

I'm doing an updated check on this. Sorry for the delay.

[rotty3000]

I'm made small changes that I think simplify the solution and don't require API guardian to be updated or even used directly, but still leverage it's support in bnd (without any changes being required).

i.e. this solution is complete and mergeable as is.

[TimvdLippe]

Thanks for all the work that was involved into this!

[wborn]

Thanks a lot! It's great to see a fix for the issue merged. 👍