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

Use the parent classloader if the context classloader is a child of it. #2306

Merged
merged 4 commits into from May 27, 2021

Conversation

charlesmunger
Copy link
Contributor

This should only affect cases that previously would have thrown an
exception.
Fixes #2303

Charles Munger and others added 2 commits May 25, 2021 11:42
@TimvdLippe
Copy link
Contributor

There are some formatting issues that should be automatically fixed if you run ./gradlew spotlessApply locally

@charlesmunger
Copy link
Contributor Author

Formatting issues fixed.

@codecov-commenter
Copy link

codecov-commenter commented May 25, 2021

Codecov Report

Merging #2306 (97903b9) into main (c2715ea) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #2306      +/-   ##
============================================
- Coverage     84.68%   84.63%   -0.06%     
- Complexity     2761     2766       +5     
============================================
  Files           328      328              
  Lines          8410     8428      +18     
  Branches       1004     1011       +7     
============================================
+ Hits           7122     7133      +11     
- Misses         1008     1015       +7     
  Partials        280      280              
Impacted Files Coverage Δ
.../creation/bytebuddy/SubclassBytecodeGenerator.java 79.28% <100.00%> (-4.33%) ⬇️
...to/internal/util/concurrent/WeakConcurrentMap.java 39.39% <0.00%> (+2.02%) ⬆️

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 c2715ea...97903b9. Read the comment docs.

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.

LGTM from my side. @raphw could you double-check as well?

MockMethodInterceptor.class,
MockMethodInterceptor.ForHashCode.class,
MockMethodInterceptor.ForEquals.class);
ClassLoader contextLoader = currentThread().getContextClassLoader();
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to avoid the static import here as it's the only use and we tend to not use static imports.

Copy link
Contributor

Choose a reason for hiding this comment

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

We already did this on line 99 in the current implementation, so we don't change this here. Can we clean these up in a follow-up?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's figure this out in a follow-up. I think we need to reconfigure the formatter to do this.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed.

// private methods.
return true;
}
for (Class<?> iface : features.interfaces) {
Copy link
Member

Choose a reason for hiding this comment

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

Interfaces might declare interfaces of their own.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's OK, as all interface methods are public, and a public interface that extends a package-private interface doesn't require the implementation to be in the same package. All that matters for the generated type is that the directly implemented interfaces are accessible.

Copy link
Member

Choose a reason for hiding this comment

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

Of course, my bad. I was thinking that you iterated over the interfaces of the class but it's of course the directly implemented ones that matter.

@TimvdLippe TimvdLippe merged commit 66998ea into mockito:main May 27, 2021
@TimvdLippe
Copy link
Contributor

Thanks @charlesmunger for working on this and the regression test!

@charlesmunger
Copy link
Contributor Author

I have since encountered two cases where this PR introduces breakage.

  1. Mocking a private abstract class while using https://github.com/google/gwtmockito. This worked before because gwtmockito was stripping final and other modifiers off of loaded classes, but now the changed loader behavior causes it to fail.
  2. A custom classloader that instruments some but not all of the packages it loads, delegating others to its parent.

It turns out that the root of these problems is
loaderBuilder.build(MockMethodInterceptor.class.getClassLoader()). I didn't actually intend to include that, it came from the 2.x release branch when I initially forked the repo, and was removed in 9bc9be6 originally.

Sorry for the error! I'll send a PR to fix.

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