Implicit boolean conversion in parameterized tests should be strict and accept only true or false

[s-rwe]

We encountered a glitch in parameterized tests, where tests can be unintentionally running with false values for an argument, for example if there is a typo in the arguments of a @CsvSource. If such tests still happen to be successful, it's likely that no one will notice the mistake.

The implicit conversion is just using Boolean::valueOf under the hood AFAICS (StringToBooleanAndCharPrimitiveConverter), which is very lenient in terms of "bad" input, which all just gets mapped to false.

Sample snippet to show problematic cases (which would have clearly been intended to be "true" values):

  @ParameterizedTest
  @CsvSource(value = {
      "ẗrue",
      "tru"
  })
  public void testAllArgsTrue(Boolean value) {
    Assertions.assertEquals(value, true);
  }

Since this can hide problems and prevent tests from running through as intended, it should be reasonable to make the implicit conversion just fail on "invalid" input, to show mistakes and force developers to correct them accordingly (i.e. ensure that only values true + false are acceptable, with case-insensitive matching).

This is also an example for which customizing the implicit argument converters could be helpful (#853 which was closed, I think that possibility doesn't exist). But IMO it should be fine to just adjust the default conversion in general, to avoid such accidental mistakes in general.

[sbrannen]

Hi @s-rwe,

Congratulations on raising your first issue for JUnit 5! 👍

I agree that we should explicitly check for true or false, ignoring case.

So I'm slating this for 5.10.

Are you interested in submitting a PR to address this?

[s-rwe]

Sure, thanks - created PR #3178 accordingly

[Treverix]

This was a BAD idea and breaks many existing tests on our side.

The general rule is that it looks for a constructor or factory method which takes a string and calls that to create the object. For Boolean, this means that the string (case insensitive) 'true' creates a Boolean.TRUE and everything else Boolean.FALSE.

We actually made use of this for creating karnaugh tables in CSV sources, which have three values: true, false and don't care. The don't care is added as a '?' or 'X' symbol and creates a Boolean.FALSE (which is ok, could also be Boolean.TRUE - because; we don't care).

Real (but simple) Example:

    @ParameterizedTest
    @CsvSource({
            // isReadOnly, hasWriteAccess, isEditable
            "  false,      true,           true",
            "  false,      false,          false",
            "  true,       ?,              false",
    })
    void isEditableTest(final boolean isReadOnly, final boolean hasWriteAccess, final boolean isEditable) {
    //...
    }

If something is read-only, we don't care if the user has write access or not - it is not editable.

Sure, we could visit all tests now and replace '?' or 'X' by 'false' but that immediately changes the semantics of the test.

IMHO, this should never have been accepted.