Classpath scanning fails for malformed class names #401

kevinvandervlist · 2016-07-14T14:15:18Z

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.

The text was updated successfully, but these errors were encountered:

marcphilipp · 2016-07-14T14:18:17Z

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

kevinvandervlist · 2016-07-14T17:25:23Z

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

marcphilipp · 2016-07-16T19:26:55Z

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 · 2016-07-17T11:12:05Z

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 · 2016-07-17T11:52:30Z

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:

Rethrow the Throwable wrapped in something like an IllegalStateException.
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 · 2016-07-17T11:57:41Z

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 · 2016-07-17T12:14:43Z

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

marcphilipp · 2016-07-17T12:18:32Z

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.

Rethrow the Throwable wrapped in something like an IllegalStateException.

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

sbrannen · 2016-07-17T15:08:28Z

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 · 2016-07-17T20:09:09Z

@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 · 2016-07-17T20:43:10Z

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 · 2016-07-18T12:44:35Z

in progress

mmerdes · 2016-07-18T13:43:02Z

@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 · 2016-07-19T07:52:06Z

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 · 2016-07-19T08:06:09Z

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 · 2016-07-19T12:52:24Z

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

mmerdes · 2016-07-19T13:14:45Z

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 · 2016-07-19T13:19:20Z

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 · 2016-07-19T13:34:33Z

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 · 2016-07-20T09:48:08Z

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

kevinvandervlist · 2016-07-21T09:45:24Z

Awesome, thanks.

For future reference, solved by 389de48

# 412

Issue: #401

sbrannen · 2016-07-22T19:29:12Z

@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 · 2016-07-24T15:10:25Z

I just verified it with commit 43b33fa, 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 · 2016-07-24T15:23:31Z

Great!

Thanks for letting us know, @kevinvandervlist.

marcphilipp added the type: bug label Jul 14, 2016

marcphilipp added this to the 5.0 M2 milestone Jul 14, 2016

kevinvandervlist closed this as completed Jul 14, 2016

kevinvandervlist reopened this Jul 14, 2016

sbrannen mentioned this issue Jul 17, 2016

Document bug fixes and changes in 5.0 M2 change log in User Guide #412

Closed

2 tasks

mmerdes self-assigned this Jul 18, 2016

mmerdes added the status: in progress label Jul 18, 2016

mmerdes removed the status: in progress label Jul 21, 2016

mmerdes closed this as completed Jul 21, 2016

mmerdes pushed a commit that referenced this issue Jul 21, 2016

Add description for bug fix #401 to release notes

de75258

# 412

sbrannen added a commit that referenced this issue Jul 22, 2016

Polish ClasspathScanner

791cb4d

Issue: #401

sbrannen changed the title ~~TestEngine crashes on invalid class names~~ Classpath scanning fails for malformed class names Jul 23, 2016

JFCote mentioned this issue Sep 29, 2017

Problem with malformed class name using scala + no test found #1082

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Classpath scanning fails for malformed class names #401

Classpath scanning fails for malformed class names #401

kevinvandervlist commented Jul 14, 2016 •

edited

marcphilipp commented Jul 14, 2016

kevinvandervlist commented Jul 14, 2016

marcphilipp commented Jul 16, 2016

kevinvandervlist commented Jul 17, 2016

sbrannen commented Jul 17, 2016

smoyer64 commented Jul 17, 2016

marcphilipp commented Jul 17, 2016

marcphilipp commented Jul 17, 2016

sbrannen commented Jul 17, 2016

smoyer64 commented Jul 17, 2016 •

edited

kevinvandervlist commented Jul 17, 2016

mmerdes commented Jul 18, 2016

mmerdes commented Jul 18, 2016 •

edited

mmerdes commented Jul 19, 2016

mmerdes commented Jul 19, 2016

marcphilipp commented Jul 19, 2016

mmerdes commented Jul 19, 2016 •

edited

mmerdes commented Jul 19, 2016

mmerdes commented Jul 19, 2016

mmerdes commented Jul 20, 2016

kevinvandervlist commented Jul 21, 2016

sbrannen commented Jul 22, 2016

kevinvandervlist commented Jul 24, 2016

sbrannen commented Jul 24, 2016

Classpath scanning fails for malformed class names #401

Classpath scanning fails for malformed class names #401

Comments

kevinvandervlist commented Jul 14, 2016 • edited

marcphilipp commented Jul 14, 2016

kevinvandervlist commented Jul 14, 2016

marcphilipp commented Jul 16, 2016

kevinvandervlist commented Jul 17, 2016

sbrannen commented Jul 17, 2016

smoyer64 commented Jul 17, 2016

marcphilipp commented Jul 17, 2016

marcphilipp commented Jul 17, 2016

sbrannen commented Jul 17, 2016

smoyer64 commented Jul 17, 2016 • edited

kevinvandervlist commented Jul 17, 2016

mmerdes commented Jul 18, 2016

mmerdes commented Jul 18, 2016 • edited

mmerdes commented Jul 19, 2016

mmerdes commented Jul 19, 2016

marcphilipp commented Jul 19, 2016

mmerdes commented Jul 19, 2016 • edited

mmerdes commented Jul 19, 2016

mmerdes commented Jul 19, 2016

mmerdes commented Jul 20, 2016

kevinvandervlist commented Jul 21, 2016

sbrannen commented Jul 22, 2016

kevinvandervlist commented Jul 24, 2016

sbrannen commented Jul 24, 2016

kevinvandervlist commented Jul 14, 2016 •

edited

smoyer64 commented Jul 17, 2016 •

edited

mmerdes commented Jul 18, 2016 •

edited

mmerdes commented Jul 19, 2016 •

edited