Junit5: Safely generate display name for @ParameterizedTest if argument.toString() throws exception

[johnsoneal]

Overview

Using JUnit 5.2.0

I am using parameterized tests, and for the most part everything works as expected. However the name with pattern fails if any object has a bad toString(), even if the object is not referenced by the pattern. For example:

public class BadToStringTest {

    static Object[] data() {
        return new Object[][] { { "has a bad toString()", new BadToString() } };
    }

    @ParameterizedTest(name = "{0}")
    @MethodSource("data")
    void test(String name, Object bad) {
        // Empty Block
    }

    static class BadToString {
        @Override
        public String toString() {
            throw new RuntimeException("Very bad to string");
        }
    }
}

In the example an exception is thrown as all arguments are formatted not just the specified pattern.

I understand that throwing an exception from toString() is a really bad idea; however, it is somewhat unexpected that the formatter is calling toString() on arguments that it is not explicitly being asked to format.

As a side note, Hamcrest for a similar situation uses a toString() that conforms to Object#toString():

try {
    return String.valueOf(value);
}
catch (Exception e) {
    return value.getClass().getName() + "@" + Integer.toHexString(value.hashCode());
}

Deliverables

• [x] Ensure that invocation of toString() on arguments supplied to a @ParameterizedTest method does not prevent generation of the display name.
  
  
  
  • In other words, invoke toString() _safely_ (within a try-catch block).
  
  

[sbrannen]

Thanks for raising the issue and including a test case that reproduces the behavior!

[sbrannen]

Analysis

This issue occurs because toString() is invoked on all "consumed arguments" in ParameterizedTestNameFormatter.makeReadable(Object[]).

In the provided example Object bad is _consumed_ by the method even though it is not included in the custom display name.

[sbrannen]

FYI: I added _Deliverables_ and slated this for 5.3 RC1.

[marcphilipp]

If/when we implement this, it should be a common utility that we can reuse for assertions etc. as well.

[sbrannen]

I also updated the title accordingly.

[sbrannen]

@marcphilipp, the code in question already uses org.junit.platform.commons.util.StringUtils.nullSafeToString(Object).

So we could either add the exception handling support there or introduce a new method (e.g., safeToString(Object)) that delegates to nullSafeToString() but returns a default _toString()_ if an exception occurs.

[sbrannen]

FWIW, org.junit.jupiter.api.AssertionUtils.toString(Object) also currently delegates to StringUtils.nullSafeToString(Object).

[sbrannen]

I've labeled this as an "enhancement" since I'm not convinced it should qualify as a bug.

Thoughts?

[marcphilipp]

I think changing nullSafeToString() is fine. Having both safeToString() and nullSafeToString() would be weird IMHO.

In any case, I think we shouldn't call toString() unnecessarily for @ParameterizedTest arguments.

[sbrannen]

Having both safeToString() and nullSafeToString() would be weird IMHO.

Sure... at the very least regarding the names. 😛

In any case, I think we shouldn't call toString() unnecessarily for @ParameterizedTest arguments.

Ideally, yes, I agree that we should not do that.

But it will require slightly more _custom code_ to avoid that. I guess we'd have to figure out if each "consumed argument" is actually referenced in the pattern supplied to org.junit.jupiter.params.ParameterizedTestNameFormatter.formatSafely(String, Object[]) and move the logic for makeReadable() to formatSafely().

Thoughts?

[sbrannen]

Or... we could pass the pattern to makeReadable() and replace objects that are not referenced in the pattern with empty strings instead of ever invoking toString() on them.

[sbrannen]

@johnsoneal, out of curiosity, how did you realize you could return a 2D array from a @MethodSource factory method?

I don't think I've ever seen anyone do that, and the JavaDoc explicitly states the following.

If a parameterized test method declares multiple parameters, factory methods must return instances of Arguments, e.g. as Stream<Arguments>.

So, I'm guessing we should update the JavaDoc for @MethodSource. 😉

[marcphilipp]

... and add a test case for it. 😉

[sbrannen]

@MethodSource factory methods can return anything supported by org.junit.platform.commons.util.CollectionUtils.toStream(Object). So we need to update the JavaDoc for CollectionUtils.toStream(Object) and then sync that with the class-level JavaDoc for @MethodSource.

[johnsoneal]

@sbrannen

out of curiosity, how did you realize you could return a 2D array

Nice to see I have done something not seen by you guys before!

Their where two main reason for doing it this way. First the ParameterizedTest documentation references every thing as being indexed in a ArgumentsProvider:

In this context, an indexed argument is an argument for a given index in the Arguments provided by an ArgumentsProvider that is passed as an argument to the parameterized method at the same index in the method's formal parameter list.

And secondly the documentation for ObjectArrayArguments

It provides access to an array of objects to be used for invoking a @ParameterizedTest method.

To me this implied I could supply a 2D array and the _automatic type conversion_ would handle the creation of the ObjectArrayArguments.

This was my reasoning, however I guess from your comment this is not the intended usage.

[sbrannen]

Nice to see I have done something not seen by you guys before!

I agree: quite amusing.

ObjectArrayArguments? Where did you come across that? I think that's be gone for half a year or more. That's just Arguments now. 😉

To me this implied I could supply a 2D array and the automatic type conversion would handle the creation of the Arguments.

We ultimately use a Stream<Arguments> internally. So the automatic type conversion support doesn't come into play here. Rather, it's the simple fact that Object[][] is an instanceof Object[], and our CollectionUtils.toStream(Object) method supports converting an Object[] into a Stream. So in this particular case, with a 2D object array, we get back a Stream<Object[]> and then an additional internal stream converts each Object[] into an Arguments object, and we ultimately have a Stream<Arguments>. 😈

[sbrannen]

This was my reasoning, however I guess from your comment this is not the intended usage.

True... it wasn't intentional. But... if it works, it works.

[sbrannen]

I just realized this is the _Columbus_ issue: "In August #1492, Columbus sailed the ocean blue."

[jbduncan]

Wow, JUnit 5 has now received almost 1500 issues and PRs! I'd consider that a great milestone. :clinking_glasses:

[sbrannen]

Update:

So, I'm guessing we should update the JavaDoc for @MethodSource.

✅

... and add a test case for it.

✅

So we need to update the JavaDoc for CollectionUtils.toStream(Object) and then sync that with the class-level JavaDoc for @MethodSource.

✅

[sbrannen]

Addressed in 5bada91048eb1e9bcc6fba2d37cdee8f627e6f6a.