improve class loading for tests#4051
Conversation
filiphr
left a comment
There was a problem hiding this comment.
This looks really really good @hduelme. From what I can see it halves the time. From around 12 minutes to around 6 minutes for all the tests.
I need to find some time to have a quick look around how we want to store the private fields and to understand hot the CompilerLauncherInterceptor is instantiated.
If I look at the example from the docs:
https://docs.junit.org/6.0.3/advanced-topics/launcher-api.html#launcher-interceptors-custom
the custom classloader is instantiated in the constructor, and the original one is reset in the intercept. However, it could be that ours is needed later and you did all the heavy lifting checks, but I want to play with it a bit.
|
@filiphr I did some experiments myself and restoring the ClassLoader to |
| orginalClassLoader = Thread.currentThread().getContextClassLoader(); | ||
| if ( !orginalClassLoader.getClass().isAssignableFrom( ModifiableURLClassLoader.class ) ) { | ||
| Thread.currentThread().setContextClassLoader( newClassLoader ); | ||
| } | ||
| return invocation.proceed(); |
There was a problem hiding this comment.
This looks a bit weird. Looking at this we end up with the originalClassLoader becoming an instance of the ModifiableURLClassLoader. Shouldn't we only set the originalClassLoader if it is not assignable from ModifiableURLClassLoader? i.e. if we actually change the context class loader?
There was a problem hiding this comment.
Good catch. That was not what should happen. I changed the implementation as you suggested.
|
@filiphr Good catch. That was not what should happen. That's should be fixed now |
|
Amazing. Thanks @hduelme. By the way, when you can, can you reach out to me by email? Your commits don't have an email I can use to reach out, that's why I am asking you to reach out to me 😀 |
I improved class loading for tests by replacing
LauncherDiscoveryListenerwithCompilerLauncherInterceptor(introduced in junit-team/junit-framework#3091). This change reduces overhead during test discovery and execution and also enables proper lifecycle handling, including correct closing ofClassLoaderinstances.I also attempted to implement #3961, but was unable to get it working. The InvocationInterceptor only allows interception at test execution time, which is too late in this case: by then, test classes have already been loaded. This leads to
IllegalAccessErrorissues when multipleClassLoaderinstances attempt to load the same package, which is not permitted.