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

Avoid warning when dynamic attach is enabled #3551

Merged
merged 6 commits into from
Dec 19, 2024
Merged

Conversation

raphw
Copy link
Member

@raphw raphw commented Dec 16, 2024

Checks the command line arguments for the EnableDynamicAgentLoading argument and does not warn about dynamic attach, if the option is set.

# Conflicts:
#	mockito-core/src/main/java/org/mockito/internal/PremainAttachAccess.java
@codecov-commenter
Copy link

codecov-commenter commented Dec 16, 2024

Codecov Report

Attention: Patch coverage is 65.00000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 85.55%. Comparing base (2e7992b) to head (374c605).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
...java/org/mockito/internal/PremainAttachAccess.java 65.00% 6 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #3551      +/-   ##
============================================
- Coverage     85.60%   85.55%   -0.05%     
- Complexity     2954     2955       +1     
============================================
  Files           341      341              
  Lines          8976     8995      +19     
  Branches       1118     1119       +1     
============================================
+ Hits           7684     7696      +12     
- Misses         1003     1009       +6     
- Partials        289      290       +1     

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

…ilable due to use of modules, or when running on Android.
…ilable due to use of modules, or when running on Android.
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.

Nice!

@TimvdLippe TimvdLippe merged commit cd4704a into main Dec 19, 2024
18 checks passed
@TimvdLippe TimvdLippe deleted the dynamic-attach-warning branch December 19, 2024 12:56
@schabe77
Copy link

schabe77 commented Jan 3, 2025

This doesn't work for me. That's because

List<String> arguments =
(List<String>)
runtimeMXBean
.getClass()
.getMethod("getInputArguments")
.invoke(runtimeMXBean);

results in a

java.lang.IllegalAccessException: class org.mockito.internal.PremainAttachAccess cannot access class sun.management.RuntimeImpl (in module java.management) because module java.management does not export sun.management to unnamed module @78b66d36

and therefore in dynamicAgentLoading = false;

But it works, when getInputArguments is requested without Reflection:

        RuntimeMXBean runtimeMXBean = (RuntimeMXBean)
                Class.forName("java.lang.management.ManagementFactory")
                        .getMethod("getRuntimeMXBean")
                        .invoke(null);
       dynamicAgentLoading =
                runtimeMXBean.getInputArguments().stream()
                        .anyMatch(
                                argument ->
                                        argument.contains(
                                                "-XX:+EnableDynamicAgentLoading"));

@raphw
Copy link
Member Author

raphw commented Jan 3, 2025

Indeed, this is an annoying implication of the module system if a method is resolved against an overriding class, not the interface. I will need to change that to get the interface class and to invoke the method on it.

@raphw
Copy link
Member Author

raphw commented Jan 5, 2025

Suggested fix: #3566

TimvdLippe pushed a commit that referenced this pull request Jan 6, 2025
Use interface type for invoking method to avoid that module boundaries are breached when using reflection on internal implementation type.

Relates to #3551
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.

6 participants