Junit5: Use static factory method or constructor as fallback for implicit argument conversion

[juergenzimmermann]

Overview

Original Title: Introduce implicit argument conversion for UUID and URL

Original Description: In https://github.com/junit-team/junit5/issues/1218 implicit arg conversion was introduced for Locale, URI, and Currency. I'd appreciate if also java.util.UUID and java.net.URL can be supported, by e.g. UUID.fromString(String name) and new URL(String) as @hisener did in https://github.com/junit-team/junit5/pull/1221.

Related Issues

• #1218
• #1232

Current Proposal

See https://github.com/junit-team/junit5/issues/1231#issuecomment-355829162 and https://github.com/junit-team/junit5/issues/1231#issuecomment-355832818.

Deliverables

• [x] Use static factory method or constructor as fallback for implicit argument conversion.
• [x] Document in User Guide.
• [x] Document in Release Notes.

[sormuras]

Good idea.

Which other "Java Foundation" classes should be supported?

[juergenzimmermann]

Maybe java.math.BigDecimal, java.math.BigInteger, and java.io.File​

[sormuras]

Mh... thinking about some "general magic" like:

1. For class T look for a public constructor with a single String parameter - if available, use it.
2. Look for a static factory method T of(String) - if available, use it.

This default fall-back would kick in before failing with "no conversion available".

[hisener]

@sormuras Are you working on this? I tried to implement something you said.

1. Checks a constructor with a String parameter as you said.
2. Checks static T methodName(String). methodName is also generic instead of of.

The code is available on https://github.com/hisener/junit5/commit/8a425d8532e73c17ed57ebe2eea7c4ef76b92c10

[sormuras]

Ja, I'm on it. Cleanup the current implementation as well.

[sbrannen]

FYI: Spring has had generic conversion support for years. We considered doing something similar for parameterized tests but opted not to (yet). Though I was and still am in favor of a generic fallback mechanism.

For inspiration from Spring, check out ObjectToObjectConverter.

[sbrannen]

1. Checks static T methodName(String). methodName is also generic instead of of.

I'm afraid that technique is too risky: picking the first such possible candidate (via findFirst()) is unpredictable due to the fact that the JVM does not guarantee the order in which methods are returned via reflection.

Beyond that, it would be a bit too much of a surprise for my taste.

I think the best we can do is to support:

• constructors that accept a single string
• static factory methods that accept a single string, where the method is named either of or from

[sormuras]

Why the restriction on the factory name?

[sbrannen]

Ja, I'm on it. Cleanup the current implementation as well.

Let's be a bit cautious there: I don't want us to oversimplify things and use reflection all of the time for common use cases.

In other words, let's let ourselves be inspired by Spring in this regard, too: have hard-coded converters for common types and only use reflection as a _fallback_ mechanism for common types we have yet to support and for user code that the framework cannot knowingly support.

[sbrannen]

Why the restriction on the factory name?

See my above comments.

In general, we should support a well-documented list of supported factory names. Anything else would be nondeterministic and unreliable.

[sbrannen]

Actually, to follow Spring's lead here, the list of supported static factory methods would be:

• valueOf(String)
• of(String)
• from(String)
• fromString(String) - _not supported by Spring but useful for things like UUID_

If you have ideas for other _common_ static factory method names, speak up. 😉

[hisener]

valueOf, of and from cannot cover java.util. For example, TimeZone has static TimeZone getTimeZone(String ID) and UUID has static UUID fromString(String name).

So, still need to have a static Map for these conversions?

[sormuras]

Handing over to @sbrannen -- copied from DefaultArgumentConverterTests

    @Test
    void convertsStringsToBigDecimalInstances() {
        assertConverts("123.456e789", BigDecimal.class, new BigDecimal("123.456e789"));
    }

    @Test
    void convertsStringsToBigIntegerInstances() {
        assertConverts("1234567890123456789", BigInteger.class, new BigInteger("1234567890123456789"));
    }

    @Test
    void convertsStringsToUUIDInstances() {
        String uuid = "d043e930-7b3b-48e3-bdbe-5a3ccfb833db";
        assertConverts(uuid, UUID.class, UUID.fromString(uuid));
    }

    @Test
    void convertsStringsToURLInstances() throws Exception {
        assertConverts("http://junit.org/junit5", URL.class, new URL("http://junit.org/junit5"));
    }

    @Test
    void convertsStringsToFileInstances() {
        assertConverts("file", File.class, new File("file"));
        assertConverts("/file", File.class, new File("/file"));
        assertConverts("/some/file", File.class, new File("/some/file"));
    }

[sbrannen]

So, still need to have a static Map for these conversions?

Yes.

See explanation here: https://github.com/junit-team/junit5/issues/1231#issuecomment-355824678

[sbrannen]

valueOf, of and from cannot cover java.util. For example, TimeZone has static TimeZone getTimeZone(String ID) and UUID has static UUID fromString(String name).

I'll add fromString to the list of supported static factory methods.

TimeZone would still be supported via the status quo.

[sbrannen]

Update:

Added _Supported Static Factory Methods_ section to issue description.

[sbrannen]

Update:

Updated issue title to reflect new scope.

[sbrannen]

Update:

I will spin off the explicit support for java.util.UUID, java.net.URL, java.math.BigDecimal, java.math.BigInteger, and java.io.File​ into a separate issue and leave this issue as it stands now (i.e., to introduce a new fallback mechanism, since the comments in this issue have focused on that topic).

[sbrannen]

FYI: the new issue is #1232.

[sbrannen]

Please note that both #1231 and #1232 are currently assigned to me.

[marcphilipp]

Proposal: Allow arbitrary names for static factory methods, but require that there's only a single one that accepts a String and returns an instance of the class. If there are none or more than one, fallback to the constructor that accepts a String, if available.

I'm not sure whether we should require the factory methods or constructors to be public. Thoughts?

[sbrannen]

@marcphilipp, your proposal sounds good.

That would ensure that we have a deterministic, reliable algorithm, which is my primary goal.

Regarding the requirement that such static factory methods or constructors be public, I think we can simply require that such members be non-private as we already do for test methods, etc. That would at least be consistent with regard to the overall programming model in Jupiter.

[sbrannen]

_in progress_

[sbrannen]

_Work In Progress_: current work on this issue can be seen in the following feature branch.

https://github.com/junit-team/junit5/tree/issues/1231-generic-string-to-obj-converter

[sbrannen]

FYI: I just _force pushed_ my latest changes...

https://github.com/junit-team/junit5/commit/dd66ae68206c0d60e21770ffeba9e13178090e34

[sbrannen]

GenericStringToObjectConverter is now fully tested, refactored, and contains caching for factory lookups.

[sbrannen]

So, all that's left is documentation... unless somebody sees any issues with the current implementation.

@junit-team/junit-lambda, any comments on the WIP?

[marcphilipp]

any comments on the WIP?

I added one comment on the commit. Other than that, it looks good to me! 👍

[sbrannen]

Updates:

• Added ParameterizedTestIntegrationTests.executesWithImplicitGenericConverter()
• Implemented "null object pattern" to avoid repeatedly searching for non-existent things.
  
  
  
  • See GenericStringToObjectConverter.NULL_EXECUTABLE and related code.
  
  

[sbrannen]

Update:

Since I switched to using the null object pattern (and therefore always storing a value in the cache), I have now switched to factoryExecutableCache.computeIfAbsent(...) as well.

[sbrannen]

@marcphilipp, let me know if the switch to factoryExecutableCache.computeIfAbsent in the latest (force pushed) commit is along the lines of what you were inquiring about earlier.

[marcphilipp]

@sbrannen Looks good to me! I've refactored it to cache Function<String, Object> instead of Executable in b3626aaf5eda9460661075c12977f9c34023e434. What do you think?

[sbrannen]

Looks good to me!

😄

I've refactored it to cache Function<String, Object> instead of Executable in b3626aaf5eda9460661075c12977f9c34023e434. What do you think?

I'm fine with the Function instead of the Executable: it's certainly cleaner and less code that way. The lambdas might incur a slight performance hit, but I'm sure it's negligible. 😉

Thanks for refactoring!

[sbrannen]

Merged feature branch into master in commit a6ef26bdc329fec12209dade3afbca3784ea6f0c.

[hisener]

Actually, to follow Spring's lead here, the list of supported static factory methods would be:

• valueOf(String)
• of(String)
• from(String)
• fromString(String)

@sbrannen I want to ask this: current implementation does not care the name of the method, right? (!private) static T factoryMethod(String) is ok?

[sormuras]

(!private) static T factoryMethod(String) is ok?

As long it is unique, meaning there's no overload like static T factoryMethodDoingMore(String), yes.

See documentation here: http://junit.org/junit5/docs/snapshot/user-guide/#writing-tests-parameterized-tests-argument-conversion-implicit-fallback

It uses: public static Book fromTitle(String title)