[params] Use test class' classloader to load parameters

[kriegfrj]

Fixes #3291.

Overview

As noted in #3291, to support varied classloader environments, the argument converter/loader logic should ideally be classloader aware. The best thing to do is to use the test class' classloader to load any classes. This PR attempts to do this.

The declaring class (and hence its classloader) is available through the ParameterContext (getParameter().getDeclaringExecutable().getDeclaringClass()). However, the reference to the ParameterContext is lost in the SimpleArgumentConverter abstraction, from which DefaultArgumentConverter currently inherits. Hence I have changed DefaultArgumentConverter to implement ArgumentConverter directly. This technical represents a major (breaking) change to the package org.junit.jupiter.params.converter, because DefaultArgumentConverter is a public class in that package. However, because it's been annotated INTERNAL I don't think that that matters (any downstream users that have inherited from it probably deserve what they get... 😄).

I have tried to implement it as best as I can so that if (for whatever reason) the DefaultArgumentConverter doesn't have access to the parameter context, then it will fall back to its current behaviour. If one is present, then it will use the test class' classloader.

The implementation was complicated by the fact that DefaultArgumentsAccessor invokes DefaultArgumentsConverter.convert() directly, and DefaultArgumentsAccessor (and the ArgumentsAccessor API provides no ParameterContext. So there was no way to pass the necessary information in to convert(). I have added the ParameterContext as a constructor parameter to DefaultArgumentsAccessor to work around this. It might have been cleaner (less changes to the tests) to add a second constructor instead and have the original delegate to the new one,

The current implementation passes all current tests (at least, it did on my machine!) but I haven't added new tests for the enhanced class-loader-sensitive functionality as yet. I want to make sure that we're happy with this direction before I invest that time. There are a couple of different ways that this cat could be skinned (no offence to cats intended) and the interaction between the various default implementations is a bit complicated.

---

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

---

Definition of Done

• There are no TODOs left in the code
• Method preconditions are checked and documented in the method's Javadoc
• Coding conventions (e.g. for logging) have been followed
• Change is covered by automated tests including corner cases, errors, and exception handling
• Public API has Javadoc and @API annotations
• Change is documented in the User Guide and Release Notes

[sbrannen]

Superseded by commit f6e73ac

[sbrannen]

Hi @kriegfrj,

Thanks for the PR!

That's quite an extensive write up, so thanks for sharing your thought process and rationale as well.

Hence I have changed DefaultArgumentConverter to implement ArgumentConverter directly.

That was the same conclusion I came to.

This technical represents a major (breaking) change to the package org.junit.jupiter.params.converter, because DefaultArgumentConverter is a public class in that package. However, because it's been annotated INTERNAL I don't think that that matters (any downstream users that have inherited from it probably deserve what they get... 😄).

Exactly.

a couple of different ways that this cat could be skinned

Indeed, I took a different approach for some of the aspects. Feel free to take a look at my implementation, and if you have any questions or comments don't hesitate to speak up.

The current implementation passes all current tests (at least, it did on my machine!)

Did you run a full Gradle build?

I'm asking because "updating" ArgumentsAccessorKotlinTests was arguably the toughest challenge for me, requiring research into Kotlin features like declaring Object... var-args in Kotlin and passing them to a Java method that accepted them. Getting Mockito to work in Kotlin was also a challenge (for me), because when is a keyword in Kotlin. 😱

On the plus side, I learned a few things about Kotlin syntax, etc.

Cheers,

Sam

[sbrannen]

Did you run a full Gradle build?

Please disregard that. I'm assuming you did since you updated ArgumentsAccessorKotlinTests as well. 😇

It was just that my refactoring required more extensive changes in ArgumentsAccessorKotlinTests, and that's why I initially overlooked the changes you made to the Kotlin tests.

[kriegfrj]

That's quite an extensive write up, so thanks for sharing your thought process and rationale as well.

I apologise - it happened by accident. I started with a brief comment, then I checked the code to see if there might be any hiccups, added a couple more comments, check the code again... before I knew it I had a complete implementation and the doc explaining how I got there...

Hence I have changed DefaultArgumentConverter to implement ArgumentConverter directly.

That was the same conclusion I came to.

Encouraging to know we're both on the same page! 😄

I took a different approach for some of the aspects. Feel free to take a look at my implementation, and if you have any questions or comments don't hesitate to speak up.

LGTM. I'm glad you went with your own implementation - it seemed more compact than mine.

The only difference of any real consequence (that I can see) the default behaviour - I tried to use tryToLoadClass(String) as the default, instead of manually calling ClassLoaderUtils.getDefaultClassLoader(). My reasoning was that if ever you want to change the default behaviour, then you only have to change tryToLoadClass(String). Admittedly, also made the code a bit more ugly, so it was a tradeoff.