Fix multiple issues with ConditionEvaluationResult reason values

Prior to this commit, there were a few long-standing issues with
ConditionEvaluationResult, as outlined in #4715.

This commit addresses those issues as follows.

- When a "custom" reason is supplied along with a null value for the
  default reason to ConditionEvaluationResult.disabled(String, String),
  the generated reason is now "custom" instead of "null ==> custom".

- A blank reason (such as an empty string or a string containing only
  whitespace) is now treated the same as a null reason, resulting in an
  "empty" reason.

- The Javadoc for all factory methods in ConditionEvaluationResult now
  explicitly states that null or blank values are supported for reasons
  and that such values will result in an "empty" reason (i.e. an empty
  Optional).

See: #4698
Closes: #4715

Author: sbrannen

documentation/src/docs/asciidoc/release-notes/release-notes-5.13.3.adoc

@@ -39,6 +39,10 @@ repository on GitHub.
   with `@Nested`.
 * Stop reporting discovery issues for `DefaultImpls` classes generated by the Kotlin
   compiler for interfaces with non-abstract test methods.
+* When a `customReason` is supplied along with a `null` value for the default `reason` to
+  `ConditionEvaluationResult.disabled(String, String)`, the resulting reason is now
+  `"my custom reason"` instead of
+  `"null ++==>++ my custom reason"`.
 
 [[release-notes-5.13.3-junit-jupiter-deprecations-and-breaking-changes]]
 ==== Deprecations and Breaking Changes
@@ -48,7 +52,13 @@ repository on GitHub.
 [[release-notes-5.13.3-junit-jupiter-new-features-and-improvements]]
 ==== New Features and Improvements
 
-* ❓
+* A _blank_ reason supplied to a `ConditionEvaluationResult` factory method is now treated
+  the same as a `null` reason, resulting in an _empty_ `Optional` returned from
+  `ConditionEvaluationResult.getReason()`.
+* The Javadoc for factory methods in `ConditionEvaluationResult` now explicitly states
+  that both `null` and _blank_ values are supported for reason strings and that such
+  values will result in an _empty_ `Optional` returned from
+  `ConditionEvaluationResult.getReason()`.
 
 
 [[release-notes-5.13.3-junit-vintage]]

junit-jupiter-api/src/main/java/org/junit/jupiter/api/extension/ConditionEvaluationResult.java

@@ -29,8 +29,11 @@ public class ConditionEvaluationResult {
 	/**
 	 * Factory for creating <em>enabled</em> results.
 	 *
-	 * @param reason the reason why the container or test should be enabled
+	 * @param reason the reason why the container or test should be enabled; may
+	 * be {@code null} or <em>blank</em> if the reason is unknown
 	 * @return an enabled {@code ConditionEvaluationResult} with the given reason
+	 * or an <em>empty</em> reason if the reason is unknown
+	 * @see StringUtils#isBlank()
 	 */
 	public static ConditionEvaluationResult enabled(String reason) {
 		return new ConditionEvaluationResult(true, reason);
@@ -39,8 +42,11 @@ public static ConditionEvaluationResult enabled(String reason) {
 	/**
 	 * Factory for creating <em>disabled</em> results.
 	 *
-	 * @param reason the reason why the container or test should be disabled
+	 * @param reason the reason why the container or test should be disabled; may
+	 * be {@code null} or <em>blank</em> if the reason is unknown
 	 * @return a disabled {@code ConditionEvaluationResult} with the given reason
+	 * or an <em>empty</em> reason if the reason is unknown
+	 * @see StringUtils#isBlank()
 	 */
 	public static ConditionEvaluationResult disabled(String reason) {
 		return new ConditionEvaluationResult(false, reason);
@@ -50,13 +56,23 @@ public static ConditionEvaluationResult disabled(String reason) {
 	 * Factory for creating <em>disabled</em> results with custom reasons
 	 * added by the user.
 	 *
-	 * @param reason the default reason why the container or test should be disabled
-	 * @param customReason the custom reason why the container or test should be disabled
-	 * @return a disabled {@code ConditionEvaluationResult} with the given reasons
+	 * <p>If non-blank default and custom reasons are provided, they will be
+	 * concatenated using the format: <code>"reason&nbsp;==&gt;&nbsp;customReason"</code>.
+	 *
+	 * @param reason the default reason why the container or test should be disabled;
+	 * may be {@code null} or <em>blank</em> if the default reason is unknown
+	 * @param customReason the custom reason why the container or test should be
+	 * disabled; may be {@code null} or <em>blank</em> if the custom reason is unknown
+	 * @return a disabled {@code ConditionEvaluationResult} with the given reason(s)
+	 * or an <em>empty</em> reason if the reasons are unknown
 	 * @since 5.7
+	 * @see StringUtils#isBlank()
 	 */
 	@API(status = STABLE, since = "5.7")
 	public static ConditionEvaluationResult disabled(String reason, String customReason) {
+		if (StringUtils.isBlank(reason)) {
+			return disabled(customReason);
+		}
 		if (StringUtils.isBlank(customReason)) {
 			return disabled(reason);
 		}
@@ -67,9 +83,10 @@ public static ConditionEvaluationResult disabled(String reason, String customRea
 
 	private final Optional<String> reason;
 
+	@SuppressWarnings("NullAway") // StringUtils.isBlank() does not yet have a nullability @Contract
 	private ConditionEvaluationResult(boolean enabled, String reason) {
 		this.enabled = enabled;
-		this.reason = Optional.ofNullable(reason);
+		this.reason = StringUtils.isNotBlank(reason) ? Optional.of(reason.strip()) : Optional.empty();
 	}
 
 	/**

jupiter-tests/src/test/java/org/junit/jupiter/api/condition/ConditionEvaluationResultTests.java

@@ -39,24 +39,15 @@ void enabledWithReason() {
 				.isEqualTo("ConditionEvaluationResult [enabled = true, reason = 'reason']");
 	}
 
-	@EmptyReasonsTest
-	void enabledWithInvalidReason(@Nullable String reason) {
+	@BlankReasonsTest
+	void enabledWithBlankReason(@Nullable String reason) {
 		@SuppressWarnings("NullAway")
 		var result = ConditionEvaluationResult.enabled(reason);
 
 		assertThat(result.isDisabled()).isFalse();
-
-		if (reason == null) {
-			assertThat(result.getReason()).isEmpty();
-			assertThat(result).asString()//
-					.isEqualTo("ConditionEvaluationResult [enabled = true, reason = '<unknown>']");
-		}
-		// TODO Remove else-block once issues are addressed.
-		else {
-			assertThat(result.getReason()).contains(reason);
-			assertThat(result).asString()//
-					.isEqualTo("ConditionEvaluationResult [enabled = true, reason = '%s']", reason);
-		}
+		assertThat(result.getReason()).isEmpty();
+		assertThat(result).asString()//
+				.isEqualTo("ConditionEvaluationResult [enabled = true, reason = '<unknown>']");
 	}
 
 	@Test
@@ -69,28 +60,19 @@ void disabledWithDefaultReason() {
 				.isEqualTo("ConditionEvaluationResult [enabled = false, reason = 'default']");
 	}
 
-	@EmptyReasonsTest
-	void disabledWithInvalidDefaultReason(@Nullable String reason) {
+	@BlankReasonsTest
+	void disabledWithBlankDefaultReason(@Nullable String reason) {
 		@SuppressWarnings("NullAway")
 		var result = ConditionEvaluationResult.disabled(reason);
 
 		assertThat(result.isDisabled()).isTrue();
-
-		if (reason == null) {
-			assertThat(result.getReason()).isEmpty();
-			assertThat(result).asString()//
-					.isEqualTo("ConditionEvaluationResult [enabled = false, reason = '<unknown>']");
-		}
-		// TODO Remove else-block once issues are addressed.
-		else {
-			assertThat(result.getReason()).contains(reason);
-			assertThat(result).asString()//
-					.isEqualTo("ConditionEvaluationResult [enabled = false, reason = '%s']", reason);
-		}
+		assertThat(result.getReason()).isEmpty();
+		assertThat(result).asString()//
+				.isEqualTo("ConditionEvaluationResult [enabled = false, reason = '<unknown>']");
 	}
 
-	@EmptyReasonsTest
-	void disabledWithValidDefaultReasonAndInvalidCustomReason(@Nullable String customReason) {
+	@BlankReasonsTest
+	void disabledWithDefaultReasonAndBlankCustomReason(@Nullable String customReason) {
 		@SuppressWarnings("NullAway")
 		var result = ConditionEvaluationResult.disabled("default", customReason);
 
@@ -100,53 +82,30 @@ void disabledWithValidDefaultReasonAndInvalidCustomReason(@Nullable String custo
 				.isEqualTo("ConditionEvaluationResult [enabled = false, reason = 'default']");
 	}
 
-	@EmptyReasonsTest
-	void disabledWithInvalidDefaultReasonAndValidCustomReason(@Nullable String reason) {
+	@BlankReasonsTest
+	void disabledWithBlankDefaultReasonAndCustomReason(@Nullable String reason) {
 		@SuppressWarnings("NullAway")
 		var result = ConditionEvaluationResult.disabled(reason, "custom");
 
 		assertThat(result.isDisabled()).isTrue();
-
-		// TODO Convert to single assertion once issues are addressed.
-		// The following should hold for all null/blank default reasons.
-		// assertThat(result).asString().isEqualTo("ConditionEvaluationResult [enabled = false, reason = 'custom']");
-
-		if (reason == null) {
-			assertThat(result.getReason()).contains("null ==> custom");
-			assertThat(result).asString()//
-					.isEqualTo("ConditionEvaluationResult [enabled = false, reason = 'null ==> custom']");
-		}
-		else {
-			var generatedReason = reason + " ==> custom";
-			assertThat(result.getReason()).contains(generatedReason);
-			assertThat(result).asString()//
-					.isEqualTo("ConditionEvaluationResult [enabled = false, reason = '%s']", generatedReason);
-		}
+		assertThat(result.getReason()).contains("custom");
+		assertThat(result).asString().isEqualTo("ConditionEvaluationResult [enabled = false, reason = 'custom']");
 	}
 
-	@EmptyReasonsTest
-	void disabledWithInvalidDefaultReasonAndInvalidCustomReason(@Nullable String reason) {
+	@BlankReasonsTest
+	void disabledWithBlankDefaultReasonAndBlankCustomReason(@Nullable String reason) {
 		// We intentionally use the reason as both the default and custom reason.
 		@SuppressWarnings("NullAway")
 		var result = ConditionEvaluationResult.disabled(reason, reason);
 
 		assertThat(result.isDisabled()).isTrue();
-
-		if (reason == null) {
-			assertThat(result.getReason()).isEmpty();
-			assertThat(result).asString()//
-					.isEqualTo("ConditionEvaluationResult [enabled = false, reason = '<unknown>']");
-		}
-		// TODO Remove else-block once issues are addressed.
-		else {
-			assertThat(result.getReason()).contains(reason);
-			assertThat(result).asString()//
-					.isEqualTo("ConditionEvaluationResult [enabled = false, reason = '%s']", reason);
-		}
+		assertThat(result.getReason()).isEmpty();
+		assertThat(result).asString()//
+				.isEqualTo("ConditionEvaluationResult [enabled = false, reason = '<unknown>']");
 	}
 
 	@Test
-	void disabledWithValidDefaultReasonAndCustomReason() {
+	void disabledWithDefaultReasonAndCustomReason() {
 		var result = ConditionEvaluationResult.disabled("default", "custom");
 
 		assertThat(result.isDisabled()).isTrue();
@@ -159,7 +118,7 @@ void disabledWithValidDefaultReasonAndCustomReason() {
 	@ParameterizedTest(name = "[{index}] reason=\"{0}\"")
 	@NullSource
 	@ValueSource(strings = { "", " ", "   ", "\t", "\n" })
-	@interface EmptyReasonsTest {
+	@interface BlankReasonsTest {
 	}
 
 }