Skip to content

Commit e5d0e0b

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 (cherry picked from commit a0879c8)
1 parent e5c9a56 commit e5d0e0b

File tree

3 files changed

+56
-76
lines changed

3 files changed

+56
-76
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: 22 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
}
@@ -69,7 +85,7 @@ public static ConditionEvaluationResult disabled(String reason, String customRea
6985

7086
private ConditionEvaluationResult(boolean enabled, String reason) {
7187
this.enabled = enabled;
72-
this.reason = Optional.ofNullable(reason);
88+
this.reason = StringUtils.isNotBlank(reason) ? Optional.of(reason.trim()) : Optional.empty();
7389
}
7490

7591
/**

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

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

41-
@EmptyReasonsTest
42-
void enabledWithInvalidReason(String reason) {
43-
@SuppressWarnings("NullAway")
41+
@BlankReasonsTest
42+
void enabledWithBlankReason(String reason) {
4443
var result = ConditionEvaluationResult.enabled(reason);
4544

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

6151
@Test
@@ -68,29 +58,18 @@ void disabledWithDefaultReason() {
6858
.isEqualTo("ConditionEvaluationResult [enabled = false, reason = 'default']");
6959
}
7060

71-
@EmptyReasonsTest
72-
void disabledWithInvalidDefaultReason(String reason) {
73-
@SuppressWarnings("NullAway")
61+
@BlankReasonsTest
62+
void disabledWithBlankDefaultReason(String reason) {
7463
var result = ConditionEvaluationResult.disabled(reason);
7564

7665
assertThat(result.isDisabled()).isTrue();
77-
78-
if (reason == null) {
79-
assertThat(result.getReason()).isEmpty();
80-
assertThat(result).asString()//
81-
.isEqualTo("ConditionEvaluationResult [enabled = false, reason = '<unknown>']");
82-
}
83-
// TODO Remove else-block once issues are addressed.
84-
else {
85-
assertThat(result.getReason()).contains(reason);
86-
assertThat(result).asString()//
87-
.isEqualTo("ConditionEvaluationResult [enabled = false, reason = '%s']", reason);
88-
}
66+
assertThat(result.getReason()).isEmpty();
67+
assertThat(result).asString()//
68+
.isEqualTo("ConditionEvaluationResult [enabled = false, reason = '<unknown>']");
8969
}
9070

91-
@EmptyReasonsTest
92-
void disabledWithValidDefaultReasonAndInvalidCustomReason(String customReason) {
93-
@SuppressWarnings("NullAway")
71+
@BlankReasonsTest
72+
void disabledWithDefaultReasonAndBlankCustomReason(String customReason) {
9473
var result = ConditionEvaluationResult.disabled("default", customReason);
9574

9675
assertThat(result.isDisabled()).isTrue();
@@ -99,53 +78,28 @@ void disabledWithValidDefaultReasonAndInvalidCustomReason(String customReason) {
9978
.isEqualTo("ConditionEvaluationResult [enabled = false, reason = 'default']");
10079
}
10180

102-
@EmptyReasonsTest
103-
void disabledWithInvalidDefaultReasonAndValidCustomReason(String reason) {
104-
@SuppressWarnings("NullAway")
81+
@BlankReasonsTest
82+
void disabledWithBlankDefaultReasonAndCustomReason(String reason) {
10583
var result = ConditionEvaluationResult.disabled(reason, "custom");
10684

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

126-
@EmptyReasonsTest
127-
void disabledWithInvalidDefaultReasonAndInvalidCustomReason(String reason) {
90+
@BlankReasonsTest
91+
void disabledWithBlankDefaultReasonAndBlankCustomReason(String reason) {
12892
// We intentionally use the reason as both the default and custom reason.
129-
@SuppressWarnings("NullAway")
13093
var result = ConditionEvaluationResult.disabled(reason, reason);
13194

13295
assertThat(result.isDisabled()).isTrue();
133-
134-
if (reason == null) {
135-
assertThat(result.getReason()).isEmpty();
136-
assertThat(result).asString()//
137-
.isEqualTo("ConditionEvaluationResult [enabled = false, reason = '<unknown>']");
138-
}
139-
// TODO Remove else-block once issues are addressed.
140-
else {
141-
assertThat(result.getReason()).contains(reason);
142-
assertThat(result).asString()//
143-
.isEqualTo("ConditionEvaluationResult [enabled = false, reason = '%s']", reason);
144-
}
96+
assertThat(result.getReason()).isEmpty();
97+
assertThat(result).asString()//
98+
.isEqualTo("ConditionEvaluationResult [enabled = false, reason = '<unknown>']");
14599
}
146100

147101
@Test
148-
void disabledWithValidDefaultReasonAndCustomReason() {
102+
void disabledWithDefaultReasonAndCustomReason() {
149103
var result = ConditionEvaluationResult.disabled("default", "custom");
150104

151105
assertThat(result.isDisabled()).isTrue();
@@ -158,7 +112,7 @@ void disabledWithValidDefaultReasonAndCustomReason() {
158112
@ParameterizedTest(name = "[{index}] reason=\"{0}\"")
159113
@NullSource
160114
@ValueSource(strings = { "", " ", " ", "\t", "\n" })
161-
@interface EmptyReasonsTest {
115+
@interface BlankReasonsTest {
162116
}
163117

164118
}

0 commit comments

Comments
 (0)