Theories failure messages describe enum arguments unhelpfully

[pimterry]

Theories are great, and they've been helping me produce some really nice tests, but now I want to define theories on every value of an enum, and failures here are quite tricky to debug because the error messages aren't too useful.

The unit test in this acts as a good example. It defines an enum, uses enum.values() as @DataPoints and then contains a theory called theoryTest that fails for one of the enum values. The error message this gives is ParameterizedAssertionError: theoryTest(allValues[1]).

This isn't that helpful, since the enum could be ordered in any way and still be broadly equivalent (and if it's coming from external code, the order could happily change arbitrarily). This is particularly annoying when you have a massive enum, want to check a theory for all values in it, at which point this error becomes entirely useless until you manually debug the theory, or count one by one through the current enum values. I've been retrofitting some theories around some old code with some fairly large enums, and this is not a fun place to be. By definition though, enum values already have meaningful names, and it'd be better if these were used, giving errors like ParameterizedAssertionError: theoryTest(BAD_VALUE).

(In addition there's the specific enum case, where the datapoint is @Datapoint public static ENUM enumValue = ENUM.A_MEANINGFUL_NAME. Here the message blames theoryTest(enumValue). That's not useful, and this is really only useful if you reliably name the datapoint variable holding your enum the same as the enum value's name anyway. Less annoying, but since it already has a real name that should describe it perfectly well already, it would be good to use that)

Attached code now does all of this, using the enum name (via toString(), since the Enum docs recommend using that instead of .name()) instead of the variable name, if the variable refers to an enum or array of enums. Enums seem like an acceptable special case for this, since they're a base Java type that's already a value with a name by definition. Sound reasonable?

[marcphilipp]

Hmm, good idea, thanks!

I'm thinking about generalizing this even more. How about reporting the above as allValues[1]: BAD_VALUE for all objects not just enums?

[pimterry]

Agreed, done

[marcphilipp]

The ternary expression can be simplified to value since "%s" wil print "null" when value is null (see http://docs.oracle.com/javase/7/docs/api/java/util/Formatter.html#syntax).

[pimterry]

Makes sense, neat, thanks. This is now fixed.

[marcphilipp]

Thanks!

@dsaff Please take a look.

[dsaff]

A couple of thoughts:

1. we need to protect against toString() exploding (which it does more often in first-time tested code than submitted code. :-)
2. Can we brainstorm about the format for a while? For example, perhaps "BAD_VALUE (from allValues[1])" would be more verbose, but also more immediately humanly readable? What do you think?

[pimterry]

Having an explicit 'from' so you can work out what's happening more intuitively is a good idea (especially with similar variable and value names), but I'd be a bit cautious with parentheses; the variable name in the message already has a function call surrounding it, and adding more parentheses makes the scope of that slightly confusing. Worth thinking about how this works with multiple parameters in the message too.

Some options for comparison:

ParameterizedAssertionError: theoryTest(nullDatapoint, someValues[0])

ParameterizedAssertionError: theoryTest(nullDatapoint: null, someValues[0]: "good value")

ParameterizedAssertionError: theoryTest(null (from nullDatapoint), "good value" (from someValues[0]))

ParameterizedAssertionError: theoryTest(null <from nullDatapoint>, "good value" <from someValues[0]>)

ParameterizedAssertionError: theoryTest(nullDatapoint -> null, someValues[0] -> "good value")

ParameterizedAssertionError: theoryTest(from nullDatapoint: null, from someValues[0]: "good value")

I think there's also probably a good argument for adding quoting of actual values too, as I've done above, since spaces/brackets/etc make it a little unclear otherwise. Personally, I prefer angle brackets (number 4) I think, but it's your call.

What should happen when toString() explodes? With the current patch you get the toString() exception at the point where the failure happens, but no parameter details, which is definitely bad. I'd suggest we want to still produce the same error message generally focussed on how the parameters failed, not toString (perhaps toString is expected to throw an exception; when testing failures in badly instantiated objects or similar), something like:

ParameterizedAssertionError: theoryTest(null <from bad>, "good value" <from values[0]>, [toString() threw RuntimeException: Error message] <from thirdDatapoint>)

Maybe that's getting a bit carried away with the many types of brackets though?

[dsaff]

I also like the angle brackets, and think the treatment of toString() you propose makes sense. Can you add this in? Thanks!

[dsaff]

One more thought: Object#toString itself (which produces MyMumbleFooClass@34d56f) is often not terribly useful. However, I think I'm OK with this, if it motivates users to produce useful toStrings. :-)

[pimterry]

Tis done

[dsaff]

Sorry here, but JUnit style is to explicitly import each method.

[dsaff]

Nice, thanks!

[dsaff]

Can you update the release notes at https://github.com/KentBeck/junit/wiki/4.12-release-notes? Thanks!