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

Allow use of Mockito jar as an agent. #3137

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Allow use of Mockito jar as an agent. #3137

wants to merge 1 commit into from

Conversation

raphw
Copy link
Member

@raphw raphw commented Oct 9, 2023

Adds premain method such that Mockito can be used as an agent and enable instrumentation, considering upcoming restrictions on dynamic attach.

@codecov-commenter
Copy link

codecov-commenter commented Oct 9, 2023

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (5d946b4) 85.48% compared to head (ec5ccd1) 85.44%.
Report is 7 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #3137      +/-   ##
============================================
- Coverage     85.48%   85.44%   -0.05%     
- Complexity     2892     2910      +18     
============================================
  Files           329      334       +5     
  Lines          8826     8869      +43     
  Branches       1095     1099       +4     
============================================
+ Hits           7545     7578      +33     
- Misses          994     1000       +6     
- Partials        287      291       +4     
Files Coverage Δ
...on/bytebuddy/InlineDelegateByteBuddyMockMaker.java 77.41% <66.66%> (-0.16%) ⬇️
...util/reflection/InstrumentationMemberAccessor.java 88.94% <66.66%> (-0.38%) ⬇️
...c/main/java/org/mockito/internal/MockitoAgent.java 46.15% <46.15%> (ø)

... and 9 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…ble instrumentation, considering upcoming restrictions on dynamic attach.
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.

Changes look good to me. Should we also update our CI to run on 21 or add a regression test in a different way to ensure we remain compatible?

@raphw
Copy link
Member Author

raphw commented Oct 9, 2023

Likely a good idea. But probably in a separate PR?

@TimvdLippe
Copy link
Contributor

Sure. That works for me. As long as we make sure it does fix the problem on Java 21.


public class MockitoAgent {

private static Instrumentation instrumentation;
Copy link
Contributor

Choose a reason for hiding this comment

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

I would say this should be volatile or guarded by a lock.
You can't guarantee visibility of the static field, because you don't know which thread will call premain and which one calls getInstrumentation().

@hgschmie
Copy link

This is open for two months now. Any ETA when this will hit an actual mockito release?

hgschmie added a commit to hgschmie/jdbi that referenced this pull request Dec 22, 2023
Revisit the mockito setup (this can be improved once
mockito/mockito#3137 has landed and was
released with mockito). For now, we add the bytebuddy agent directly.

Also update spotbugs and junit, which is needed to pass build under
Java 22-ea.
@TimvdLippe
Copy link
Contributor

We are awaiting the resolution of the Maven team on what the official migration path for projects such as Mockito will be: #3037 (comment)

hgschmie added a commit to hgschmie/jdbi that referenced this pull request Dec 28, 2023
Revisit the mockito setup (this can be improved once
mockito/mockito#3137 has landed and was
released with mockito). For now, we add the bytebuddy agent directly.

Also update spotbugs and junit, which is needed to pass build under
Java 22-ea.
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

5 participants