Skip to content

Commit a0879c8

Browse files
sbrannensbrannen
committed
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
1 parent 6beec69 commit a0879c8

File tree

3 files changed

+57
-71
lines changed

3 files changed

+57
-71
lines changed

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

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,10 @@ repository on GitHub.
3939
with `@Nested`.
4040
* Stop reporting discovery issues for `DefaultImpls` classes generated by the Kotlin
4141
compiler for interfaces with non-abstract test methods.
42+
* When a `customReason` is supplied along with a `null` value for the default `reason` to
43+
`ConditionEvaluationResult.disabled(String, String)`, the resulting reason is now
44+
`"my custom reason"` instead of
45+
`"null ++==>++ my custom reason"`.
4246

4347
[[release-notes-5.13.3-junit-jupiter-deprecations-and-breaking-changes]]
4448
==== Deprecations and Breaking Changes
@@ -48,7 +52,13 @@ repository on GitHub.
4852
[[release-notes-5.13.3-junit-jupiter-new-features-and-improvements]]
4953
==== New Features and Improvements
5054

51-
* ❓
55+
* A _blank_ reason supplied to a `ConditionEvaluationResult` factory method is now treated
56+
the same as a `null` reason, resulting in an _empty_ `Optional` returned from
57+
`ConditionEvaluationResult.getReason()`.
58+
* The Javadoc for factory methods in `ConditionEvaluationResult` now explicitly states
59+
that both `null` and _blank_ values are supported for reason strings and that such
60+
values will result in an _empty_ `Optional` returned from
61+
`ConditionEvaluationResult.getReason()`.
5262

5363

5464
[[release-notes-5.13.3-junit-vintage]]

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

Lines changed: 23 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,11 @@ public class ConditionEvaluationResult {
2929
/**
3030
* Factory for creating <em>enabled</em> results.
3131
*
32-
* @param reason the reason why the container or test should be enabled
32+
* @param reason the reason why the container or test should be enabled; may
33+
* be {@code null} or <em>blank</em> if the reason is unknown
3334
* @return an enabled {@code ConditionEvaluationResult} with the given reason
35+
* or an <em>empty</em> reason if the reason is unknown
36+
* @see StringUtils#isBlank()
3437
*/
3538
public static ConditionEvaluationResult enabled(String reason) {
3639
return new ConditionEvaluationResult(true, reason);
@@ -39,8 +42,11 @@ public static ConditionEvaluationResult enabled(String reason) {
3942
/**
4043
* Factory for creating <em>disabled</em> results.
4144
*
42-
* @param reason the reason why the container or test should be disabled
45+
* @param reason the reason why the container or test should be disabled; may
46+
* be {@code null} or <em>blank</em> if the reason is unknown
4347
* @return a disabled {@code ConditionEvaluationResult} with the given reason
48+
* or an <em>empty</em> reason if the reason is unknown
49+
* @see StringUtils#isBlank()
4450
*/
4551
public static ConditionEvaluationResult disabled(String reason) {
4652
return new ConditionEvaluationResult(false, reason);
@@ -50,13 +56,23 @@ public static ConditionEvaluationResult disabled(String reason) {
5056
* Factory for creating <em>disabled</em> results with custom reasons
5157
* added by the user.
5258
*
53-
* @param reason the default reason why the container or test should be disabled
54-
* @param customReason the custom reason why the container or test should be disabled
55-
* @return a disabled {@code ConditionEvaluationResult} with the given reasons
59+
* <p>If non-blank default and custom reasons are provided, they will be
60+
* concatenated using the format: <code>"reason&nbsp;==&gt;&nbsp;customReason"</code>.
61+
*
62+
* @param reason the default reason why the container or test should be disabled;
63+
* may be {@code null} or <em>blank</em> if the default reason is unknown
64+
* @param customReason the custom reason why the container or test should be
65+
* disabled; may be {@code null} or <em>blank</em> if the custom reason is unknown
66+
* @return a disabled {@code ConditionEvaluationResult} with the given reason(s)
67+
* or an <em>empty</em> reason if the reasons are unknown
5668
* @since 5.7
69+
* @see StringUtils#isBlank()
5770
*/
5871
@API(status = STABLE, since = "5.7")
5972
public static ConditionEvaluationResult disabled(String reason, String customReason) {
73+
if (StringUtils.isBlank(reason)) {
74+
return disabled(customReason);
75+
}
6076
if (StringUtils.isBlank(customReason)) {
6177
return disabled(reason);
6278
}
@@ -67,9 +83,10 @@ public static ConditionEvaluationResult disabled(String reason, String customRea
6783

6884
private final Optional<String> reason;
6985

86+
@SuppressWarnings("NullAway") // StringUtils.isBlank() does not yet have a nullability @Contract
7087
private ConditionEvaluationResult(boolean enabled, String reason) {
7188
this.enabled = enabled;
72-
this.reason = Optional.ofNullable(reason);
89+
this.reason = StringUtils.isNotBlank(reason) ? Optional.of(reason.strip()) : Optional.empty();
7390
}
7491

7592
/**

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

Lines changed: 23 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -39,24 +39,15 @@ void enabledWithReason() {
3939
.isEqualTo("ConditionEvaluationResult [enabled = true, reason = 'reason']");
4040
}
4141

42-
@EmptyReasonsTest
43-
void enabledWithInvalidReason(@Nullable String reason) {
42+
@BlankReasonsTest
43+
void enabledWithBlankReason(@Nullable String reason) {
4444
@SuppressWarnings("NullAway")
4545
var result = ConditionEvaluationResult.enabled(reason);
4646

4747
assertThat(result.isDisabled()).isFalse();
48-
49-
if (reason == null) {
50-
assertThat(result.getReason()).isEmpty();
51-
assertThat(result).asString()//
52-
.isEqualTo("ConditionEvaluationResult [enabled = true, reason = '<unknown>']");
53-
}
54-
// TODO Remove else-block once issues are addressed.
55-
else {
56-
assertThat(result.getReason()).contains(reason);
57-
assertThat(result).asString()//
58-
.isEqualTo("ConditionEvaluationResult [enabled = true, reason = '%s']", reason);
59-
}
48+
assertThat(result.getReason()).isEmpty();
49+
assertThat(result).asString()//
50+
.isEqualTo("ConditionEvaluationResult [enabled = true, reason = '<unknown>']");
6051
}
6152

6253
@Test
@@ -69,28 +60,19 @@ void disabledWithDefaultReason() {
6960
.isEqualTo("ConditionEvaluationResult [enabled = false, reason = 'default']");
7061
}
7162

72-
@EmptyReasonsTest
73-
void disabledWithInvalidDefaultReason(@Nullable String reason) {
63+
@BlankReasonsTest
64+
void disabledWithBlankDefaultReason(@Nullable String reason) {
7465
@SuppressWarnings("NullAway")
7566
var result = ConditionEvaluationResult.disabled(reason);
7667

7768
assertThat(result.isDisabled()).isTrue();
78-
79-
if (reason == null) {
80-
assertThat(result.getReason()).isEmpty();
81-
assertThat(result).asString()//
82-
.isEqualTo("ConditionEvaluationResult [enabled = false, reason = '<unknown>']");
83-
}
84-
// TODO Remove else-block once issues are addressed.
85-
else {
86-
assertThat(result.getReason()).contains(reason);
87-
assertThat(result).asString()//
88-
.isEqualTo("ConditionEvaluationResult [enabled = false, reason = '%s']", reason);
89-
}
69+
assertThat(result.getReason()).isEmpty();
70+
assertThat(result).asString()//
71+
.isEqualTo("ConditionEvaluationResult [enabled = false, reason = '<unknown>']");
9072
}
9173

92-
@EmptyReasonsTest
93-
void disabledWithValidDefaultReasonAndInvalidCustomReason(@Nullable String customReason) {
74+
@BlankReasonsTest
75+
void disabledWithDefaultReasonAndBlankCustomReason(@Nullable String customReason) {
9476
@SuppressWarnings("NullAway")
9577
var result = ConditionEvaluationResult.disabled("default", customReason);
9678

@@ -100,53 +82,30 @@ void disabledWithValidDefaultReasonAndInvalidCustomReason(@Nullable String custo
10082
.isEqualTo("ConditionEvaluationResult [enabled = false, reason = 'default']");
10183
}
10284

103-
@EmptyReasonsTest
104-
void disabledWithInvalidDefaultReasonAndValidCustomReason(@Nullable String reason) {
85+
@BlankReasonsTest
86+
void disabledWithBlankDefaultReasonAndCustomReason(@Nullable String reason) {
10587
@SuppressWarnings("NullAway")
10688
var result = ConditionEvaluationResult.disabled(reason, "custom");
10789

10890
assertThat(result.isDisabled()).isTrue();
109-
110-
// TODO Convert to single assertion once issues are addressed.
111-
// The following should hold for all null/blank default reasons.
112-
// assertThat(result).asString().isEqualTo("ConditionEvaluationResult [enabled = false, reason = 'custom']");
113-
114-
if (reason == null) {
115-
assertThat(result.getReason()).contains("null ==> custom");
116-
assertThat(result).asString()//
117-
.isEqualTo("ConditionEvaluationResult [enabled = false, reason = 'null ==> custom']");
118-
}
119-
else {
120-
var generatedReason = reason + " ==> custom";
121-
assertThat(result.getReason()).contains(generatedReason);
122-
assertThat(result).asString()//
123-
.isEqualTo("ConditionEvaluationResult [enabled = false, reason = '%s']", generatedReason);
124-
}
91+
assertThat(result.getReason()).contains("custom");
92+
assertThat(result).asString().isEqualTo("ConditionEvaluationResult [enabled = false, reason = 'custom']");
12593
}
12694

127-
@EmptyReasonsTest
128-
void disabledWithInvalidDefaultReasonAndInvalidCustomReason(@Nullable String reason) {
95+
@BlankReasonsTest
96+
void disabledWithBlankDefaultReasonAndBlankCustomReason(@Nullable String reason) {
12997
// We intentionally use the reason as both the default and custom reason.
13098
@SuppressWarnings("NullAway")
13199
var result = ConditionEvaluationResult.disabled(reason, reason);
132100

133101
assertThat(result.isDisabled()).isTrue();
134-
135-
if (reason == null) {
136-
assertThat(result.getReason()).isEmpty();
137-
assertThat(result).asString()//
138-
.isEqualTo("ConditionEvaluationResult [enabled = false, reason = '<unknown>']");
139-
}
140-
// TODO Remove else-block once issues are addressed.
141-
else {
142-
assertThat(result.getReason()).contains(reason);
143-
assertThat(result).asString()//
144-
.isEqualTo("ConditionEvaluationResult [enabled = false, reason = '%s']", reason);
145-
}
102+
assertThat(result.getReason()).isEmpty();
103+
assertThat(result).asString()//
104+
.isEqualTo("ConditionEvaluationResult [enabled = false, reason = '<unknown>']");
146105
}
147106

148107
@Test
149-
void disabledWithValidDefaultReasonAndCustomReason() {
108+
void disabledWithDefaultReasonAndCustomReason() {
150109
var result = ConditionEvaluationResult.disabled("default", "custom");
151110

152111
assertThat(result.isDisabled()).isTrue();
@@ -159,7 +118,7 @@ void disabledWithValidDefaultReasonAndCustomReason() {
159118
@ParameterizedTest(name = "[{index}] reason=\"{0}\"")
160119
@NullSource
161120
@ValueSource(strings = { "", " ", " ", "\t", "\n" })
162-
@interface EmptyReasonsTest {
121+
@interface BlankReasonsTest {
163122
}
164123

165124
}

0 commit comments

Comments
 (0)