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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -90,15 +90,53 @@ protected SubclassBytecodeGenerator( | |
handler = ModuleHandler.make(byteBuddy, loader, random); | ||
} | ||
|
||
private static boolean needsSamePackageClassLoader(MockFeatures<?> features) { | ||
if (!Modifier.isPublic(features.mockedType.getModifiers()) | ||
|| !features.mockedType.isInterface()) { | ||
// The mocked type is package private or is not an interface and thus may contain | ||
// package private methods. | ||
return true; | ||
} | ||
for (Class<?> iface : features.interfaces) { | ||
if (!Modifier.isPublic(iface.getModifiers())) { | ||
return true; | ||
} | ||
} | ||
return false; | ||
} | ||
|
||
@Override | ||
public <T> Class<? extends T> mockClass(MockFeatures<T> features) { | ||
ClassLoader classLoader = | ||
MultipleParentClassLoader.Builder loaderBuilder = | ||
new MultipleParentClassLoader.Builder() | ||
.appendMostSpecific(getAllTypes(features.mockedType)) | ||
.appendMostSpecific(features.mockedType) | ||
.appendMostSpecific(features.interfaces) | ||
.appendMostSpecific(currentThread().getContextClassLoader()) | ||
.appendMostSpecific(MockAccess.class) | ||
.build(); | ||
.appendMostSpecific( | ||
MockAccess.class, DispatcherDefaultingToRealMethod.class) | ||
.appendMostSpecific( | ||
MockMethodInterceptor.class, | ||
MockMethodInterceptor.ForHashCode.class, | ||
MockMethodInterceptor.ForEquals.class); | ||
ClassLoader contextLoader = currentThread().getContextClassLoader(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. |
||
boolean shouldIncludeContextLoader = true; | ||
if (needsSamePackageClassLoader(features)) { | ||
// For the generated class to access package-private methods, it must be defined by the | ||
// same classloader as its type. All the other added classloaders are required to load | ||
// the type; if the context classloader is a child of the mocked type's defining | ||
// classloader, it will break a mock that would have worked. Check if the context class | ||
// loader is a child of the classloader we'd otherwise use, and possibly skip it. | ||
ClassLoader candidateLoader = loaderBuilder.build(); | ||
for (ClassLoader parent = contextLoader; parent != null; parent = parent.getParent()) { | ||
if (parent == candidateLoader) { | ||
shouldIncludeContextLoader = false; | ||
break; | ||
} | ||
} | ||
} | ||
if (shouldIncludeContextLoader) { | ||
loaderBuilder = loaderBuilder.appendMostSpecific(contextLoader); | ||
} | ||
ClassLoader classLoader = loaderBuilder.build(MockMethodInterceptor.class.getClassLoader()); | ||
|
||
// If Mockito does not need to create a new class loader and if a mock is not based on a JDK | ||
// type, we attempt | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,70 @@ | ||
/* | ||
* Copyright (c) 2017 Mockito contributors | ||
* This program is made available under the terms of the MIT License. | ||
*/ | ||
package org.mockitousage.bugs.creation; | ||
|
||
import static org.assertj.core.api.Assertions.assertThat; | ||
import static org.mockito.Mockito.mock; | ||
import static org.mockito.Mockito.when; | ||
import static org.mockito.Mockito.withSettings; | ||
|
||
import org.junit.After; | ||
import org.junit.Before; | ||
import org.junit.Test; | ||
|
||
/** Regression tests for issue #2303*/ | ||
public class PackagePrivateWithContextClassLoaderTest { | ||
|
||
private ClassLoader oldContextClassloader; | ||
|
||
public static class PublicClass { | ||
|
||
int packagePrivateMethod() { | ||
return 0; | ||
} | ||
} | ||
|
||
public interface PublicInterface {} | ||
|
||
interface PackagePrivateInterface {} | ||
|
||
static class PackagePrivateClass {} | ||
|
||
@Before | ||
public void setUp() { | ||
oldContextClassloader = Thread.currentThread().getContextClassLoader(); | ||
Thread.currentThread().setContextClassLoader(new ClassLoader(oldContextClassloader) {}); | ||
} | ||
|
||
@After | ||
public void teardown() { | ||
Thread.currentThread().setContextClassLoader(oldContextClassloader); | ||
} | ||
|
||
@Test | ||
public void should_be_able_to_mock_package_private_method() throws Exception { | ||
PublicClass publicClass = mock(PublicClass.class); | ||
when(publicClass.packagePrivateMethod()).thenReturn(3); | ||
assertThat(publicClass.packagePrivateMethod()).isEqualTo(3); | ||
} | ||
|
||
@Test | ||
public void should_be_able_to_mock_package_private_class() throws Exception { | ||
PackagePrivateClass mock = mock(PackagePrivateClass.class); | ||
} | ||
|
||
@Test | ||
public void should_be_able_to_mock_package_private_interface() throws Exception { | ||
PackagePrivateInterface mock = mock(PackagePrivateInterface.class); | ||
} | ||
|
||
@Test | ||
public void should_be_able_to_mock_package_private_extra_interface() throws Exception { | ||
PackagePrivateInterface mock = | ||
(PackagePrivateInterface) | ||
mock( | ||
PublicInterface.class, | ||
withSettings().extraInterfaces(PackagePrivateInterface.class)); | ||
} | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.