Junit5: Support widening conversion for parameter resolution

Created on 4 Oct 2017 · 27Comments · Source: junit-team/junit5

Overview

Current behavior: ParameterResolutionExceptionis thrown, e.g. ParameterResolver [ParameterResolverReproducerTest$IntegerResolver] resolved a value of type [java.lang.Integer] for parameter [long arg0] in executable [void ParameterResolverReproducerTest.failing(long)], but a value assignment compatible with [long] is required.

Desired behavior, e.g. a long parameter can be called with an Integer argument

Version: JUnit 5.0.1
Reproducer, see below
Java Spec: https://docs.oracle.com/javase/specs/jls/se7/html/jls-5.html#jls-5.2

Example Code

@ExtendWith(ParameterResolverReproducerTest.IntegerResolver.class)
class ParameterResolverReproducerTest {
    public static class IntegerResolver implements ParameterResolver {

        @Override
        public boolean supportsParameter(ParameterContext parameterContext, ExtensionContext extensionContext) throws ParameterResolutionException {
            return long.class.equals(parameterContext.getParameter().getType());
        }

        @Override
        public Object resolveParameter(ParameterContext parameterContext, ExtensionContext extensionContext) throws ParameterResolutionException {
            return Integer.valueOf(1);
        }
    }

    @Test
    void failing(long a) {
        // nothing to do
    }

    @Test
    void succeeding() {
        failing(Integer.valueOf(1));
    }
}

I guess one way to address this would be extending ReflectionUtils#primitiveToWrapperMap with some more possibilities.

If you agree on this, I can try to submit this via PR.

Cheers,
Andreas

Jupiter Platform parameterized tests programming model enhancement

Source

aaschmid

Most helpful comment

Resolved in master in commits 20411d8139e848db2f15f6294627ef03955db9c4 and 1fc39a7456cd8da3f5f8fad996090157b4d9f6eb.

sbrannen on 1 Apr 2018

🎉1 👍1

All 27 comments

I'm not sure that we would want to implement this support directly in ReflectionUtils#primitiveToWrapperMap since it is a general purpose utility for the entire JUnit Platform.

sbrannen on 13 Oct 2017

In any case, we will need to reach a consensus within the team in order to take the next steps here.

@junit-team/junit-lambda I'm adding the "team discussion" label and slating for 5.1 M1 to get it on our radar early.

sbrannen on 13 Oct 2017

@aaschmid since this is not technically a "bug", I have reworded the issue's description.

sbrannen on 13 Oct 2017

Take your time, I have already implemented a workaround as Java allows it and the users of my junit-dataprovider most likely used it, I would would like to continue supporting it :-)

aaschmid on 13 Oct 2017

Do you have a specific example of where auto-unboxing does not work?

For example, if you look at the testWithMultiArgMethodSource() example in the User Guide, that already demonstrates that an int can be boxed into an Integer (by Java) and then unboxed into an int by JUnit Jupiter.

sbrannen on 14 Oct 2017

I have already implemented a workaround

Got a link?

sbrannen on 14 Oct 2017

So my reproducer above does not help?

Here a short description: The actual problem is that resolveParameter returns Object such that every primitive type is automatically boxed for arguments. If the arguments would be of type int it could normally be applied to a method taking a parameter of type long, right? But because of the boxing it is now of type Interger and cannot be applied to a parameter of type long anymore. The Java Specification does allow it.

Here is the link to my workaround though it is quite hacky: https://github.com/TNG/junit-dataprovider/blob/b23ddb00334f916b8e5e9392ea1c4193c79c3570/junit-jupiter/src/main/java/com/tngtech/junit/dataprovider/resolver/DataProviderParameterResolver.java#L51.

If you have any further question, don't hesitate to ask.

aaschmid on 14 Oct 2017

Thanks for the link to your work-around. Yes... it's a bit hacky. 😉

So my reproducer above does not help?

My point was that (AFAIK) JUnit Jupiter already supports auto-unboxing in this context.

But... you claim that it does not.

With regard to _widening_ conversions, I am aware that JUnit Jupiter does not provide widening conversions for arguments to parameterized tests.

So, in summary, I am merely attempting to limit the scope of this issue to behavior that potentially needs to be changed.

If you have a concrete example of where auto-unboxing does not work, please provide a concrete example.

Thanks!

sbrannen on 14 Oct 2017

Hi @sbrannen,

I think the auto-unboxing is working fine, at least all my tests work find and no workaround is required :-)
The not working thing is the combination of auto-unboxing and widening conversion.

Cheers,
Andreas

aaschmid on 15 Oct 2017

I changed the issue title accordingly. @aaschmid Is this something you'd be interested to work on?

marcphilipp on 16 Mar 2018

@marcphilipp I can create a PR for this. What thoughts have you already got on this in your team discussion?

aaschmid on 16 Mar 2018

Nobody objected against supporting widening conversions. So, feel free to ahead and create a PR. 🙂

marcphilipp on 16 Mar 2018

@aaschmid, please keep my previous comment in mind:

I'm not sure that we would want to implement this support directly in ReflectionUtils#primitiveToWrapperMap since it is a general purpose utility for the entire JUnit Platform.

In other words, in your PR please try to introduce the widening support first within the junit-jupiter-params project, and if you find that you for some reason need to update core platform utilities (e.g., ReflectionUtils), please provide a short comment explaining why.

Thanks... and have fun!

sbrannen on 17 Mar 2018

Hi @sbrannen,
the problem does already occur in result handling of org.junit.jupiter.api.extension.ParameterResolver.resolveParameter(ParameterContext, ExtensionContext). In my opinion this would be the place to fix it. Maybe not by changing ReflectionUtils#primitiveToWrapperMap but also not only in junit-jupiter-params. Later would only fix it for org.junit.jupiter.params.provider.ArgumentsProvider.provideArguments(ExtensionContext).

Do you agree with that, or do you need further details?

Here are my use cases (and workarounds):

aaschmid on 17 Mar 2018

Thinking more about it, I am still not sure if org.junit.platform.commons.util.ReflectionUtils or even org.junit.platform.commons.util.ReflectionUtils#isAssignableTo isn't the correct place to fix it. I think it should be fixed generally because it is specified already in Java Spec (https://docs.oracle.com/javase/specs/jls/se7/html/jls-5.html#jls-5.2) section "Assignment Conversion".

But I will just do this with you go. Before that I will look for another solution in junit-juptier-engine (to also fix ParameterResolver with the reason from above).

aaschmid on 18 Mar 2018

@aaschmid You might be right. I'll bring this up in our next team meeting. For the time being, let's try to do it in DefaultArgumentConverter, ok?

marcphilipp on 20 Mar 2018

@aaschmid, thanks for investigating and voicing your concerns!

As @marcphilipp mentioned, we will discuss it within the team.

sbrannen on 20 Mar 2018

FWIW, I honestly have not analyzed the situation myself within the code base.

My goal was to minimize the impact if possible, but if it turns out that that does not make sense, I would of course be in favor in making the changes in ReflectionUtils.

sbrannen on 20 Mar 2018

Any news on this?

I provided a first version as suggested for DefaultArgumentConverter only, see https://github.com/junit-team/junit5/compare/master...aaschmid:issues/1092-widening-conversion?expand=1. The new method isUnboxingWideningConversion should maybe be simplified.
I can create a PR if you prefer but wanted to wait for your decision...

Edit: I currently omitted "User Guide" and "Release Notes" updates.

aaschmid on 30 Mar 2018

Hi @aaschmid,

I haven't verified this in code, but I _believe_ all we need to do is update ReflectionUtils.isAssignableTo() so that it relies on Class.isAssignableFrom()instead of Class.isInstance(). We might need to convert primitive types to corresponding types first, but like I said: I haven't tried it out, so it might just work as simply as that.

The invocation via ReflectionUtils.invokeMethod() should then work fine if ReflectionUtils.isAssignableTo() returns true.

sbrannen on 31 Mar 2018

or.... perhaps not.

Let me play around a bit and get back to you.... 😉

sbrannen on 31 Mar 2018

OK. I changed my mind.

I was misinterpreting what Class.isAssignableFrom() actually supports in terms of _widening_.

In any case, I have a solution working locally with changes only made to ReflectionUtils.

So I'll push that to a feature branch for public discussion.

sbrannen on 31 Mar 2018

Update:

I pushed my work to the following feature branch.

https://github.com/junit-team/junit5/commits/issues/1092-widening-conversion

@aaschmid, I took the liberty of copying your tests, but I set you as the _author_.

@junit-team/junit-lambda, if you're OK with the approach I've taken in the feature branch, I'll add documentation and push to master.

Thoughts?

sbrannen on 31 Mar 2018

Thanks for using me as author even if it isn't necessary ;-)
In my opinion your commit fixes this issue. It works now also without my workarounds on junit-dataprovider.

I also added some review comments to b6ee305.

aaschmid on 31 Mar 2018