Load classes using the test's own ClassLoader

[chrisr3]

Load classes using a ClassLoader obtained from the test class or fall back to the default class loader.

This allows JUnit to find the classes while running inside an OSGi framework.

---

I hereby agree to the terms of the JUnit Contributor License Agreement.

[marcphilipp]

Also load classes via Class.forName(), which is preferred over ClassLoader.loadClass() (The latter involves taking class-loading locks, leading to contention in a multi-threaded environment.)

@chrisr3 Are there any docs stating that?

[chrisr3]

Also load classes via Class.forName(), which is preferred over ClassLoader.loadClass() (The latter involves taking class-loading locks, leading to contention in a multi-threaded environment.)

@chrisr3 Are there any docs stating that?

I would expect so, although I was actually thinking of my own experience profiling an application which did a lot of classloading. We noticed that ClassLoader.loadClass() was constantly competing for the Application classloader's class-loading lock:

protected Class<?> loadClass(String name, boolean resolve) throws ClassNotFoundException {
    synchronized(this.getClassLoadingLock(name)) {
        Class<?> c = this.findLoadedClass(name);
        if (c == null) {
            try {
                if (this.parent != null) {
                    c = this.parent.loadClass(name, false);
                } else {
                    c = this.findBootstrapClassOrNull(name);
                }
            } catch (ClassNotFoundException var10) {
            }

            if (c == null) {
                c = findClass(name);
            }
        }
        ...
    }

And all of this contention disappeared when we switched to Class.forName(String, boolean, ClassLoader) instead.

The JavaDoc for ClassLoader.loadClass() may be relevant here:

Unless overridden, this method synchronizes on the result of getClassLoadingLock method during the entire class loading process.

[sbrannen]

Thanks for the PR!

I've requested a few minor changes.

[sbrannen]

Hi @chrisr3,

Do you have time to make the requested changes?

We'd like to release 5.10 M1 soon.

So if you don't have time, I can take over the PR and make the changes after merging.

[chrisr3]

Do you have time to make the requested changes?

I'll try to find time today 👍

[marcphilipp]

@chrisr3 Could you please also add some tests that use a custom class loader for this (see OpenTest4JAndJUnit4AwareThrowableCollectorTests for an example)?

[kriegfrj]

@chrisr3 Could you please also add some tests that use a custom class loader for this (see OpenTest4JAndJUnit4AwareThrowableCollectorTests for an example)?

You might also be able to use one like what I used in #2106, if it's helpful. Been a while since I looked at it.

[chrisr3]

@chrisr3 Could you please also add some tests that use a custom class loader for this (see OpenTest4JAndJUnit4AwareThrowableCollectorTests for an example)?

Sigh, I am not going to have time to look at this today after all.

[sbrannen]

You might also be able to use one like what I used in #2106, if it's helpful. Been a while since I looked at it.

You may also find the CustomTypeClassLoader that I introduced in this commit useful.

[sbrannen]

Sigh, I am not going to have time to look at this today after all.

No worries, @chrisr3.

It seems that we will not be releasing 5.10 M1 over the weekend but rather (hopefully) before the end of next week.

So if you can get to it in the first few days of next week, that would be fine.

[sbrannen]

You might also be able to use one like what I used in #2106, if it's helpful. Been a while since I looked at it.

You may also find the CustomTypeClassLoader that I introduced in this commit useful.

Actually, please use the FilteringClassLoader that I introduced in 75c0bd0 and renamed to TestClassLoader and revised in d66dcf6.

If the TestClassLoader does not suit your needs, please let us know so that we can improve it.

[chrisr3]

If the TestClassLoader does not suit your needs, please let us know so that we can improve it.

Hmmm... I've written a lot of fiddly class-loading Java code over the years, and that experience has taught me that the JVM maintains an internal cache of classes that it has already loaded. The JVM only invokes ClassLoader.loadClass() if it cannot find a class within that cache, and if loading class X requires also loading class Y, then it will use X.class.getClassLoader() to locate class Y.

However, no classes returned by TestClassLoader.loadClass() can ever satisfy

X.class.getClassLoader() instanceof TestClassloader

which means the JVM can bypass TestClassLoader's filter in some cases. And even that is assuming that the JVM hasn't already loaded those classes into its cache in the first place.

I need to think about how to test this 🤔.

[sbrannen]

However, no classes returned by TestClassLoader.loadClass() can ever satisfy

X.class.getClassLoader() instanceof TestClassloader

When I add this:

System.err.println(customType.getClassLoader() instanceof TestClassLoader);

to org.junit.platform.commons.util.ReflectionUtilsTests.findMethodByParameterNamesWithWithCustomTypeFromDifferentClassLoader(), I get this:

true

So I don't understand what you are claiming.

In any case, TestClassloader suits our needs in 4 places already. So I feel confident it should suit your needs, too.

In summary, TestClassloader is not intended to be something production-ready. Rather, it's only intended to meet our very limited testing needs.

[sbrannen]

I need to think about how to test this 🤔.

I updated one of our tests using TestClassLoader to check the ClassLoader in 1c9c018.

From that you can see that TestClassLoader is the expected ClassLoader for the top-level type CustomType; however, the ClassLoader for NestedType is in fact null.

So maybe that's what you were hinting at.

In any case, I still think TestClassLoader is sufficient for our testing purposes. We don't want to spend a lot of time making this internal testing utility production-ready.

As for how to "test this", statements like the following from that test are sufficient in my opinion.

assertThat(parameterType).isNotEqualTo(CustomType.NestedType.class);

[chrisr3]

When I add this:

System.err.println(customType.getClassLoader() instanceof TestClassLoader);

to org.junit.platform.commons.util.ReflectionUtilsTests.findMethodByParameterNamesWithWithCustomTypeFromDifferentClassLoader(), I get this:

true

Oh my, I see what you've done... Try executing this:

var customType1 = customTypeClassLoader.loadClass(customTypeName);
var customType2 = customTypeClassLoader.loadClass(customTypeName);

[sbrannen]

Oh my, I see what you've done... Try executing this:

var customType1 = customTypeClassLoader.loadClass(customTypeName);
var customType2 = customTypeClassLoader.loadClass(customTypeName);

Indeed... it's not production-ready.

java.lang.LinkageError: loader org.junit.platform.commons.test.TestClassLoader @bcec361 attempted duplicate class definition for org.junit.platform.commons.util.classes.CustomType. (org.junit.platform.commons.util.classes.CustomType is in unnamed module of loader org.junit.platform.commons.test.TestClassLoader @bcec361, parent loader 'app')

But I'm not worried about that.

None of our tests attempt to load the same class twice. So I can live with that.

TestClassLoader is a quick and dirty solution that meets our current needs. We have no plans to publish it, and if we later find it doesn't meet our needs we can then refactor it to meet our needs.

[sbrannen]

FWIW, I pushed a fix for that LinkageError in 4b45db0... just in case we end up having a test case that attempts to load the same class via reflection multiple times.

[sbrannen]

@chrisr3, if you run into any challenges with testing the changes, feel free to finish up the PR without the tests, and someone in the team will add tests afterwards.

Depending on the use case, you might need to use the EngineTestKit combined with a custom ClassLoader (maybe the TestClassLoader) to actually test the changes in an integration test.

Cheers,

Sam

[chrisr3]

FWIW, I pushed a fix for that LinkageError in 4b45db0... just in case we end up having a test case that attempts to load the same class via reflection multiple times.

i would actually have suggested using ClassLoader.loadClass(String, boolean) as model.

[sbrannen]

i would actually have suggested using ClassLoader.loadClass(String, boolean) as model.

Thanks for the tip, @chrisr3. 👍

I revised the implementation in f16982e.

[sbrannen]

• see also Load classes using Class.forName() instead of ClassLoader.loadClass() #3290

[sbrannen]

Hi @chrisr3,

I see you've made all of the requested changes. 👍

So, all that's missing is:

• tests
• release notes entry

Since you mentioned that you didn't have time to address #3290 (#3290 (comment)), do you think you'll have time to finish this PR this week?

If not, that's absolutely not a problem. Someone in the team can take over the PR and finish it.

Please just let us know about your availability for completing this issue.

Thanks!

[chrisr3]

I think this test has an error:

diff --git a/platform-tests/src/test/java/org/junit/platform/commons/util/ReflectionUtilsTests.java b/platform-tests/src/test/java/org/junit/platform/commons/util/ReflectionUtilsTests.java
index e8bc60ff01..877139f3cd 100644
--- a/platform-tests/src/test/java/org/junit/platform/commons/util/ReflectionUtilsTests.java
+++ b/platform-tests/src/test/java/org/junit/platform/commons/util/ReflectionUtilsTests.java
@@ -1049,10 +1049,9 @@ class ReflectionUtilsTests {
 
                                var parameterTypes = method.getParameterTypes();
                                assertThat(parameterTypes).extracting(Class::getName).containsExactly(nestedTypeName);
-                               Class<?> parameterType = parameterTypes[0].getClass();
+                               Class<?> parameterType = parameterTypes[0];
                                assertThat(parameterType).isNotEqualTo(CustomType.NestedType.class);
-                               // The ClassLoader is null because of the behavior of our TestClassLoader.
-                               assertThat(parameterType.getClassLoader()).isNull();
+                               assertThat(parameterType.getClassLoader()).isEqualTo(customTypeClassLoader);
                        });
                }
        }

[sbrannen]

I think this test has an error:

Oops... that's embarrassing. 😮

Good catch!

I'll fix it right away.

[chrisr3]

It's no good - I've run out of time to look at this PR further 😞.

[sbrannen]

It's no good - I've run out of time to look at this PR further 😞.

Don't worry about it, @chrisr3.

We'll try to take it from here.

Thanks for bringing the issue to our attention, creating the PR, assisting with TestClassLoader improvements, etc.! 👍

[sbrannen]

Thanks for making the requested changes.

I'm approving this PR and will take it from here.

[sbrannen]

This has been merged into main in ebe7a02 and revised in cd1b3b0, 8bfe25a, and b3f6c99.

Thanks again for your contributions! 👍