Junit5: Classpath scanning fails for malformed class names

[kevinvandervlist]

When using org.junit.platform:junit-platform-gradle-plugin:1.0.0-M1 (but also org.junit:junit-gradle:5.0.0-ALPHA) you can run into a crash scenario when the project somehow has classnames that are invalid.

Running gradle check:

Jul 14, 2016 3:14:31 PM org.junit.platform.launcher.core.ServiceLoaderTestEngineRegistry loadTestEngines
INFO: Discovered TestEngines with IDs: [junit-jupiter]
Exception in thread "main" java.lang.InternalError: Malformed class name
        at java.lang.Class.getSimpleName(Class.java:1330)
        at java.lang.Class.isAnonymousClass(Class.java:1411)
        at java.lang.Class.isLocalClass(Class.java:1422)
        at org.junit.jupiter.engine.discovery.predicates.IsPotentialTestContainer.test(IsPotentialTestContainer.java:35)
        at org.junit.jupiter.engine.discovery.predicates.IsTestClassWithTests.test(IsTestClassWithTests.java:43)
        at org.junit.jupiter.engine.discovery.predicates.IsScannableTestClass.test(IsScannableTestClass.java:36)
        at org.junit.jupiter.engine.discovery.predicates.IsScannableTestClass.test(IsScannableTestClass.java:26)
        at java.util.Optional.filter(Optional.java:178)
        at org.junit.platform.commons.util.ClasspathScanner.collectClassesRecursively(ClasspathScanner.java:129)
        at org.junit.platform.commons.util.ClasspathScanner.collectClassesRecursively(ClasspathScanner.java:132)
        at org.junit.platform.commons.util.ClasspathScanner.collectClassesRecursively(ClasspathScanner.java:132)
        at org.junit.platform.commons.util.ClasspathScanner.collectClassesRecursively(ClasspathScanner.java:132)
        at org.junit.platform.commons.util.ClasspathScanner.collectClassesRecursively(ClasspathScanner.java:132)
        at org.junit.platform.commons.util.ClasspathScanner.collectClassesRecursively(ClasspathScanner.java:132)
        at org.junit.platform.commons.util.ClasspathScanner.collectClassesRecursively(ClasspathScanner.java:132)
        at org.junit.platform.commons.util.ClasspathScanner.findClassesInSourceDirRecursively(ClasspathScanner.java:116)
        at org.junit.platform.commons.util.ClasspathScanner.scanForClassesInClasspathRoot(ClasspathScanner.java:87)
        at org.junit.platform.commons.util.ReflectionUtils.findAllClassesInClasspathRoot(ReflectionUtils.java:376)
        at org.junit.jupiter.engine.discovery.DiscoverySelectorResolver.lambda$resolveSelectors$0(DiscoverySelectorResolver.java:48)
        at java.util.ArrayList.forEach(ArrayList.java:1249)
        at org.junit.jupiter.engine.discovery.DiscoverySelectorResolver.resolveSelectors(DiscoverySelectorResolver.java:47)
        at org.junit.jupiter.engine.JupiterTestEngine.resolveDiscoveryRequest(JupiterTestEngine.java:50)
        at org.junit.jupiter.engine.JupiterTestEngine.discover(JupiterTestEngine.java:43)
        at org.junit.platform.launcher.core.DefaultLauncher.discoverRoot(DefaultLauncher.java:108)
        at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:84)
        at org.junit.platform.console.tasks.ExecuteTestsTask.executeTests(ExecuteTestsTask.java:60)
        at org.junit.platform.console.tasks.ExecuteTestsTask.lambda$execute$5(ExecuteTestsTask.java:52)
        at org.junit.platform.console.tasks.CustomContextClassLoaderExecutor.invoke(CustomContextClassLoaderExecutor.java:33)
        at org.junit.platform.console.tasks.ExecuteTestsTask.execute(ExecuteTestsTask.java:52)
        at org.junit.platform.console.tasks.ConsoleTaskExecutor.executeTask(ConsoleTaskExecutor.java:38)
        at org.junit.platform.console.ConsoleLauncher.execute(ConsoleLauncher.java:54)
        at org.junit.platform.console.ConsoleLauncher.main(ConsoleLauncher.java:39)

On closer inspection it crashes on the following class:

 nl.soqua.dl.domain.projection.album.AlbumProjection$albumByIdentity$$inlined$let$lambda$lambda$1

... which is a class generated by Kotlin.

I know you can't influence the code that is generated by Kotlin, but I don't think the TestEngine should fail with an InternalError, without at least signaling the user which class actually caused that error.

[marcphilipp]

Thanks for the bug report -- we should definitely fix it for M2.

[kevinvandervlist]

Thanks, if you need any more info, just give me a heads up

[marcphilipp]

On second thought, I'm not sure how to fix this. I'm not comfortable with catching InternalError. Do you know a way to check this? My best shot would be to check for the presence of $$ to exclude classes with binary names that do not follow any JLS specification (cf. https://docs.oracle.com/javase/specs/jls/se8/html/jls-13.html#jls-13.1).

@junit-team/junit-lambda Any ideas?

[kevinvandervlist]

No, I don't have any ideas on how to check this.

It might be fine if the exception is just wrapped with another InternalError with a different message though. If the filename of the offending class is included in the message it is trivial to diagnose the problem, that will already help tremendously.

[sbrannen]

I think we should just add a try-catch block around the following lines in ClasspathScanner.collectClassesRecursively(File, String, List, Predicate) which catches Throwable.

Optional<Class<?>> classForClassFile = loadClassForClassFile(file, packageName);
classForClassFile.filter(classFilter).ifPresent(classesCollector::add);

Then we have two options:

1. Rethrow the Throwable wrapped in something like an IllegalStateException.
2. Log the Throwable at WARN level and swallow it.

In both cases, we should include a meaningful error message stating the canonical name of the class that caused the error, and even then the retrieval of the canonical name should be performed defensively within another try-catch block.

[smoyer64]

A Java class name cannot start with a number but it can start with (and contain within) any letter, number or connecting character. Since you can put $ characters in a class name, you also can't assume that you can split a class, inner-class and inner-inner-class by the $ character. So I believe the class in question is a legal class identifier (though not a conventional one by any means).

[marcphilipp]

Why does Class.getSimpleName() throw an InternalError then?

[marcphilipp]

I think we should just add a try-catch block around the following lines in ClasspathScanner.collectClassesRecursively(File, String, List, Predicate) which catches Throwable.

If we catch Throwable we have to make sure not to catch OutOfMemoryErrors etc. again. We'd have to move BlacklistedExceptions to junit-platform-commons for that.

1. Rethrow the Throwable wrapped in something like an IllegalStateException.

If we throw an exception it should be a JUnitException, shouldn't it?

[sbrannen]

If we catch Throwable we have to make sure not to catch OutOfMemoryErrors etc. again. We'd have to move BlacklistedExceptions to junit-platform-commons for that.

That's true, but that's also why I prefer option # 2.

Since this is about classpath scanning where multiple things can go wrong, I think the best solution is simply to swallow such exceptions and log it. If a test class doesn't get picked up via classpath scanning, the user can then check to the log to see what went wrong. And... in most cases when a failure occurs during classpath scanning, I think the average user simply won't care.

If we throw an exception it should be a JUnitException, shouldn't it?

Yes. When I said "like an IllegalStateException", that could well be interpreted as a JUnitException. 😉

[smoyer64]

@marcphilipp

You ask hard questions! A couple hours of writing junk code and trying to reproduce this issue allows me to stand by my assertion regarding allowed character sets. The answer is that the innermost class named lambda is not allowed per the Java specification to hide it's parent class' name. It turns out that it doesn't matter to the JVM. But if you try to do reflection on that class, it's trying to reflect the Kotlin generated classes as though they're Java.

I originally thought there might be a similar problem with generated proxy classes like those used in CDI or JPA but they actually don't hide classes when nested. There have been similar issues in Scala byte-code.

So for JUnit 5, I don't think there's a choice except to catch the error. I would argue that Kotlin should probably fix this (they claim to be 100% compatible with Java). Even if Kotlin has it's own reflection library, if you include another Java library you never know when that library might use reflection.

[kevinvandervlist]

If it helps, here is a relatively small testcase that replicates the crash. I tried to extract only the parts relevant to this issue.

https://github.com/kevinvandervlist/kotlin-junit5-bug

[mmerdes]

in progress

[mmerdes]

@kevinvandervlist

Thanks for providing the testcase project. I added it as a local gradle file dependency and was able to reproduce the bug with a simple unit test for the IsPotentialTestContainer predicate.

[mmerdes]

A similiar crash happens when com.intellij.junit5.JUnit5IdeaTestRunner is used to run tests within Idea:

java.lang.InternalError: Malformed class name

at java.lang.Class.getSimpleName(Class.java:1330)
at java.lang.Class.isAnonymousClass(Class.java:1411)
at java.lang.Class.isLocalClass(Class.java:1422)
at org.junit.jupiter.engine.discovery.predicates.IsPotentialTestContainer.test(IsPotentialTestContainer.java:35)
at org.junit.jupiter.engine.discovery.predicates.IsPotentialTestContainer.test(IsPotentialTestContainer.java:27)
at org.junit.jupiter.engine.discovery.predicates.IsPotentialTestContainerTests.crashesOnInvalidClassName(IsPotentialTestContainerTests.java:32)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:497)
at org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:252)
at org.junit.jupiter.engine.execution.ExecutableInvoker.invoke(ExecutableInvoker.java:114)
at org.junit.jupiter.engine.descriptor.MethodTestDescriptor.lambda$invokeTestMethod$51(MethodTestDescriptor.java:210)
at org.junit.jupiter.engine.execution.ThrowableCollector.execute(ThrowableCollector.java:40)
at org.junit.jupiter.engine.descriptor.MethodTestDescriptor.invokeTestMethod(MethodTestDescriptor.java:206)
at org.junit.jupiter.engine.descriptor.MethodTestDescriptor.execute(MethodTestDescriptor.java:155)
at org.junit.jupiter.engine.descriptor.MethodTestDescriptor.execute(MethodTestDescriptor.java:63)
at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor.lambda$execute$15(HierarchicalTestExecutor.java:81)
at org.junit.platform.engine.support.hierarchical.SingleTestExecutor.executeSafely(SingleTestExecutor.java:66)
at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor.execute(HierarchicalTestExecutor.java:77)
at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor.lambda$execute$15(HierarchicalTestExecutor.java:88)
at org.junit.platform.engine.support.hierarchical.SingleTestExecutor.executeSafely(SingleTestExecutor.java:66)
at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor.execute(HierarchicalTestExecutor.java:77)
at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor.lambda$execute$15(HierarchicalTestExecutor.java:88)
at org.junit.platform.engine.support.hierarchical.SingleTestExecutor.executeSafely(SingleTestExecutor.java:66)
at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor.execute(HierarchicalTestExecutor.java:77)
at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor.execute(HierarchicalTestExecutor.java:51)
at org.junit.platform.engine.support.hierarchical.HierarchicalTestEngine.execute(HierarchicalTestEngine.java:43)
at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:124)
at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:84)
at com.intellij.junit5.JUnit5IdeaTestRunner.startRunnerWithArgs(JUnit5IdeaTestRunner.java:42)
at com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:253)
at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:84)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:497)
at com.intellij.rt.execution.application.AppMain.main(AppMain.java:147)

This exception is triggered by MethodTestDescriptor so fixing only ClasspathScanner won't suffice.

[mmerdes]

When comparing both stacktraces one finds that both crashes are caused by invoking
org.junit.jupiter.engine.discovery.predicates.IsPotentialTestContainer#test.
So if the solution described above is applied here both cases should be fixed and BlacklistedExceptions can remain in its current module.

[marcphilipp]

This exception comes from your tests IsPotentialTestContainerTests. crashesOnInvalidClassName, doesn't it?

[mmerdes]

yes, when executing a local test case for the malformed Kotlin class, i.e. running the predicate on the Kotlin class instance. The point is that the situation can occur both in classpath scanning and in execution.

[mmerdes]

In any case I would consider it safest to catch the problem as soon as possible, i.e. as soon as there is a class implemented by JUnit on the stack trace.

[mmerdes]

The root cause seems to be that calling getSimpleName() on a class instance with a malformed name, e.g. the Kotlin generated example leads to the InternalError in question. This can happen twice when the predicate is evaluated.

[mmerdes]

Team decision: place the safeguard measure at the classpath scanner level for now.

[kevinvandervlist]

Awesome, thanks.

For future reference, solved by https://github.com/junit-team/junit5/commit/389de48c2a18c5a93a7203ef424aa47a8a835a74

[sbrannen]

@kevinvandervlist, I made some additional refinements regarding this issue, none of which should have any adverse side effects on the fix. However, it would be great if you could take the latest snapshot for a spin and confirm that the modified fix still works for you.

[kevinvandervlist]

I just verified it with commit 43b33fa09236110eae742e152f78b726052b18aa, and it works like a charm:

WARNING: The java.lang.Class loaded from file [/tmp/kotlin-junit5-bug/bug/build/classes/main/nl/soqua/dl/domain/projection/album/AlbumProjection$albumByIdentity$$inlined$let$lambda$lambda$1.class] has a malformed class name [nl.soqua.dl.domain.projection.album.AlbumProjection$albumByIdentity$$inlined$let$lambda$lambda$1].

Thanks!

[sbrannen]

Great!

Thanks for letting us know, @kevinvandervlist.