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
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -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
charlesmunger marked this conversation as resolved.
Show resolved Hide resolved
// 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.

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.interfaces)
.appendMostSpecific(currentThread().getContextClassLoader())
.appendMostSpecific(MockAccess.class)
.build();
.append(features.mockedType)
charlesmunger marked this conversation as resolved.
Show resolved Hide resolved
.append(features.interfaces)
.append(MockAccess.class, DispatcherDefaultingToRealMethod.class)
.append(
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.

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.append(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
Expand Down
@@ -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));
}
}