Remove or relax the new rule for strict String to Boolean conversion

[Treverix]

#3178 made String to Boolean conversion more strict (added with version 5.10.0).

I strongly suggest to remove that feature or relax on the rule, because now we can't use our karnaugh maps (KM) on CSV sources anymore.

Karnaugh maps allow three values: false, true and don't care (or 0, 1, don't care).

Consider this (simple) example. We test if a method correctly computes, that a form is editable

    @ParameterizedTest
    @CsvSource(delimiter = '|', textBlock = """
        # isReadOnly | hasWriteAccess | isEditable
        # -----------+----------------+------------
          false      | true           | true
        # -----------+----------------+------------
          false      | false          | false
          true       | ?              | false
        # -----------+----------------+------------
        """)
    void isEditableTest(final boolean isReadOnly, final boolean hasWriteAccess, final boolean isEditable) {
   // ...
  }

If 'read-only' is true, then we don't care about user access rights - it's not editable. The implementation is not required to check that. Technically, we get a false injected, but: we don't care. Can be false or true. The implementation does not care and neither does the test. It shall work with false and true and it's ok to test with one of both.

Karnaugh maps can get a lot more complex, if the method under test has more then just two logical conditions. Those KM in CSV sources greatly improve readability and maintainability of unit tests.

#3178 makes that impossible now, because '?' or 'X' for 'don't care' are no accepted values anymore.

The common rule in JUnit is that the system looks for a constructor or static factory method, that takes a String. Here, it uses either new Boolean(String) or Boolean.valueOf(String) and both return Boolean.TRUE only if the String is true (case insensitive). #3178 breaks that rule.

I understand, that this is helpful to detect typos in parameterized test values. But I still think, that it adds more problems then value.

I suggest, that JUnit get's updated and either

• removes that feature again or
• adds a flag to opt out with a configuration parameter or
• the existing rule allows two extra symbols: ? and x (case insensitve) so that we can keep our karnaugh maps in CSV sources.

[sbrannen]

Have you considered using Boolean instead of boolean in order to support 3 states?

• true
• false
• null (don't care)

In addition, you can already configure what you consider null values.

Combining those two concepts gives us the following, which I imagine might suit your needs.

@ParameterizedTest
@CsvSource(delimiter = '|', nullValues = {"?", "x", "X"}, textBlock = """
		# isReadOnly | hasWriteAccess | isEditable
		# -----------+----------------+------------
		  false      | true           | true
		# -----------+----------------+------------
		  false      | false          | false
		  true       | ?              | false
		# -----------+----------------+------------
		""")
void isEditableTest(Boolean isReadOnly, Boolean hasWriteAccess, Boolean isEditable) {
	// ...
}

[Treverix]

@sbrannen Is there a difference between using boolean and Boolean? Do the newly added strict rules only apply to the primitive boolean type but not to the wrapper? #3177 suggested the change for boolean values in general, not just for primitives. And the title of #3178 suggests, that it affects the wrapper ('String to Boolean conversion')

Release notes say

• Implicit type conversion of boolean values like in @CsvSource is now stricter, only allowing values "true" or "false" (case-insensitive), in order to make accidental mistakes apparent and to avoid potential confusion.

Documentation says now:

Target Type         Example
boolean/Boolean     "true" → true (only accepts values 'true' or 'false', case-insensitive)

and I received reports that some or many tests wouldn't work after an upgrade, but up to this point I only assumed, that it would affect both 'boolean' and 'Boolean' at the same time. If using Boolean is not affected, then I'll close this ticket.

Even if release notes and documention would be misleading (RN) or event incorrect (docs)

[s-rwe]

Or as further alternatives, you could also consider explicit argument conversion for such values, instead of the implicit one - which would also give you full control over which actual value the "?"-values should become mapped to for the tests at runtime (like always true, always false, or possibly a random value).

That should work via either:

1. A custom ArgumentConverter<String, Boolean> and @ConvertWith on the parameters of test-methods that need it
2. A custom, own data-type (e.g. KarnaughBoolean) as parameters in test-methods instead of using booleans, capable of parsing the 3rd (don't care) value, and having an accessor for getting the actual boolean value out again.

IMO that would also make it much more explicit (towards the actual test-code) that you're really dealing with a 3-value type here.

[sbrannen]

The release notes and user guide are perhaps a bit misleading.

The strictness for what constitutes true or false applies equally to boolean and Boolean regarding the input strings.

However, a Boolean can naturally also have a null value. In Java a Boolean can always be used to mimic three states: true, false, and null. Whereas, a boolean can only ever be true or false.

I was therefore suggesting that you could use a null value to indicate "don't care" values in your input data. Then, in your test, you could treat null as either true or false as you wish.

[Treverix]

@sbrannen Ah, I see. But that's different semantics. 'null' is absence which is different from 'don't care'. And we couldn't use true, false, null and don't care on the same input table.

'null' is no input, '?' is any input (but never nothing). Also, I want to avoid 'null' as input, as it will lead to NPEs on outboxing. And I want to avoid extra boilerplate in the test, which converts each null to a false (for each possible input in the matrix.

It would have to map the null values to a valid boolean in the method, like so. Which is quite some distracting boilerplate, that we should avoid in test methods.

    @ParameterizedTest
    @CsvSource(delimiter = '|', nullValues = {"?", "x", "X"}, textBlock = """
		# isFoo  | isBar | isFooBar | isBaz | expectedResult
		# -------+-------+----------+-------+----------------
		  true   | ?     | ?        | ?     | true
		  false  | true  | ?        | ?     | true
		  false  | false | true     | ?     | true
		  false  | false | false    | true  | true
		# -------+-------+----------+-------+----------------
		  false  | false | false    | false | true
		# -------+-------+----------+-------+----------------
		""")
    void isEditableTest(boolean isFoo, Boolean isBar_, Boolean isFooBar_, Boolean isBaz_, boolean expectedResult) {
        boolean isBar = isBar_ == null ? DONT_CARE : isBar_;   // DONT_CARE is a static const like boolean DONT_CARE=false
        boolean isFooBar = isFooBar_ == null ? DONT_CARE : isFooBar_;
        boolean isBaz = isBaz_ == null ? DONT_CARE : isBaz_;

        // ...
    }

[sbrannen]

@Treverix, if using null to model "don't care" values doesn't work for you, I recommend you take one of the approaches suggested by @s-rwe in #3490 (comment).

[Treverix]

@s-rwe ArgumentConverter is too complicated to give it to all devs. It can't appear on each position on the method signature and the implementation needs to compute the correct argument index. From my experience with that, I'd avoid it here and limit it to some special cases.

For special type like KarnaughBoolean, we miss the outboxing or implicit conversion. Event though I could imagine implementing it as a record like so

record KarnaughBoolean(boolean value) {
    
    public static KarnaughBoolean from(final String value) {
        return new KarnaughBoolean(Boolean.parseBoolean(value));
    }
}

and use it in the test like so

    @ParameterizedTest
    @CsvSource(delimiter = '|', textBlock = """
        # isReadOnly | hasWriteAccess | isEditable
        # -----------+----------------+------------
          false      | true           | true
        # -----------+----------------+------------
          false      | false          | false
          true       | ?              | false
        # -----------+----------------+------------
        """)
    void isEditableTest(final KarnaughBoolean isReadOnly, final KarnaughBoolean hasWriteAccess, final KarnaughBoolean isEditable) {
   // ...
   assertThat(result).isEqualTo(isEditable.value());
  }

Basically, I'd have to invest in boilerplate to revert to the pre-5.10 behavior.

[Treverix]

@sbrannen See me last comment. A custom type seems to be the cheapest workaround (which still has to be communicated to all devs), but I still don't understand, why a breaking change was introduced just to make it easier for unexperienced users to spot their typos. At the cost of others, who now have to invest in change test code to be able to update JUnit on their side.

JUnit is not a typo checker. It's not it's purpose.

[s-rwe]

@Treverix - yes, each of these variants needs some amount of changes, we're just trying to point out alternatives.

I don't understand what should be complex for Developers when using the ArgumentConverter approach though, or where any sort of "argument index" comes into play with that...? An ArgumentConverter would just need to be implemented once with a few lines of code; and all that dev's would need to know is the @ConvertWith annotation, to enable it on the (boolean-) parameters where it is needed.

So taking one of the sample signatures above, it could look like this if an ArgumentConverter named KBoolConverter were available (to make 3 of the parameters support the undefined-value):

void isEditableTest(boolean isFoo, @ConvertWith(KBoolConverter.class) boolean isBar, @ConvertWith(KBoolConverter.class) boolean isFooBar, @ConvertWith(KBoolConverter.class) isBaz, boolean expectedResult) {
  [...]

While that does indeed clutter the signatures up a bit, I still think it makes things just more explicit.

The nice thing with that approach (compared to the other alternative I mentioned) is that actual test-code would otherwise not need to be changed at all, it's purely the signatures that would need to be adjusted.

[sbrannen]

@s-rwe ArgumentConverter is too complicated to give it to all devs. It can't appear on each position on the method signature and the implementation needs to compute the correct argument index. From my experience with that, I'd avoid it here and limit it to some special cases.

As @s-rwe mentioned above, it's not all that complicated, and you don't need to compute any indexes.

For example...

package example;

import org.junit.jupiter.params.converter.TypedArgumentConverter;

class KarnaughBooleanConverter extends TypedArgumentConverter<String, Boolean> {

	KarnaughBooleanConverter() {
		super(String.class, Boolean.class);
	}

	@Override
	protected Boolean convert(String value) {
		return Boolean.parseBoolean(value);
	}
}

package example;

import java.lang.annotation.Documented;
import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;

import org.junit.jupiter.params.converter.ConvertWith;

@Target(ElementType.PARAMETER)
@Retention(RetentionPolicy.RUNTIME)
@Documented
@ConvertWith(KarnaughBooleanConverter.class)
public @interface KarnaughBoolean {
}

package example;

import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.CsvSource;

class KarnaughTests {

	@ParameterizedTest
	@CsvSource(delimiter = '|', textBlock = """
			# isFoo  | isBar | isFooBar | isBaz | expectedResult
			# -------+-------+----------+-------+----------------
			  true   | ?     | ?        | ?     | true
			  false  | true  | ?        | ?     | true
			  false  | false | true     | ?     | true
			  false  | false | false    | true  | true
			# -------+-------+----------+-------+----------------
			  false  | false | false    | false | true
			# -------+-------+----------+-------+----------------
			""")
	void isEditableTest(boolean isFoo, @KarnaughBoolean boolean isBar, @KarnaughBoolean boolean isFooBar,
			@KarnaughBoolean boolean isBaz, boolean expectedResult) {
		// ...
	}

}

With this approach, you only need to annotate targeted boolean parameters with @KarnaughBoolean, and you also get total control over what the KarnaughBooleanConverter does.

You could also shorten the name of @KarnaughBoolean to @Karnaugh if you like.

[sbrannen]

but I still don't understand, why a breaking change was introduced just to make it easier for unexperienced users to spot their typos. At the cost of others, who now have to invest in change test code to be able to update JUnit on their side.

@Treverix, I understand your concerns, and we'll discuss this topic within the team.