Skip to content

Commit 7c44f3b

Browse files
marcphilippmarcphilipp
committed
Report discovery issues for blank @SentenceFragments 
1 parent e0e9bf8 commit 7c44f3b

File tree

6 files changed

+219
-31
lines changed

6 files changed

+219
-31
lines changed

‎junit-jupiter-api/src/main/java/org/junit/jupiter/api/DisplayNameGenerator.java

Lines changed: 8 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -29,12 +29,9 @@
2929
import java.util.function.Predicate;
3030

3131
import org.apiguardian.api.API;
32-
import org.junit.platform.commons.logging.Logger;
33-
import org.junit.platform.commons.logging.LoggerFactory;
3432
import org.junit.platform.commons.support.ReflectionSupport;
3533
import org.junit.platform.commons.util.ClassUtils;
3634
import org.junit.platform.commons.util.Preconditions;
37-
import org.junit.platform.commons.util.StringUtils;
3835

3936
/**
4037
* {@code DisplayNameGenerator} defines the SPI for generating display names
@@ -350,8 +347,6 @@ class IndicativeSentences implements DisplayNameGenerator {
350347

351348
static final DisplayNameGenerator INSTANCE = new IndicativeSentences();
352349

353-
private static final Logger logger = LoggerFactory.getLogger(IndicativeSentences.class);
354-
355350
private static final Predicate<Class<?>> notIndicativeSentences = clazz -> clazz != IndicativeSentences.class;
356351

357352
public IndicativeSentences() {
@@ -502,22 +497,14 @@ private static Optional<IndicativeSentencesGeneration> findIndicativeSentencesGe
502497
}
503498

504499
private static String getSentenceFragment(AnnotatedElement element) {
505-
Optional<SentenceFragment> annotation = findAnnotation(element, SentenceFragment.class);
506-
if (annotation.isPresent()) {
507-
String sentenceFragment = annotation.get().value().trim();
508-
509-
// TODO [#242] Replace logging with precondition check once we have a proper mechanism for
510-
// handling validation exceptions during the TestEngine discovery phase.
511-
if (StringUtils.isBlank(sentenceFragment)) {
512-
logger.warn(() -> String.format(
513-
"Configuration error: @SentenceFragment on [%s] must be declared with a non-blank value.",
514-
element));
515-
}
516-
else {
517-
return sentenceFragment;
518-
}
519-
}
520-
return null;
500+
return findAnnotation(element, SentenceFragment.class) //
501+
.map(SentenceFragment::value) //
502+
.map(sentenceFragment -> {
503+
Preconditions.notBlank(sentenceFragment, String.format(
504+
"@SentenceFragment on [%s] must be declared with a non-blank value.", element));
505+
return sentenceFragment.trim();
506+
}) //
507+
.orElse(null);
521508
}
522509

523510
}

‎junit-platform-engine/src/main/java/org/junit/platform/engine/DiscoveryIssue.java

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,9 @@ interface Builder {
123123

124124
/**
125125
* Set the {@link TestSource} for the {@code DiscoveryIssue}.
126+
*
127+
* @param source the {@link TestSource} for the {@code DiscoveryIssue};
128+
* never {@code null} but potentially empty
126129
*/
127130
default Builder source(Optional<TestSource> source) {
128131
source.ifPresent(this::source);
@@ -131,11 +134,17 @@ default Builder source(Optional<TestSource> source) {
131134

132135
/**
133136
* Set the {@link TestSource} for the {@code DiscoveryIssue}.
137+
*
138+
* @param source the {@link TestSource} for the {@code DiscoveryIssue};
139+
* may be {@code null}
134140
*/
135141
Builder source(TestSource source);
136142

137143
/**
138144
* Set the {@link Throwable} that caused the {@code DiscoveryIssue}.
145+
*
146+
* @param cause the {@link Throwable} that caused the
147+
* {@code DiscoveryIssue}; never {@code null} but potentially empty
139148
*/
140149
default Builder cause(Optional<Throwable> cause) {
141150
cause.ifPresent(this::cause);
@@ -144,6 +153,9 @@ default Builder cause(Optional<Throwable> cause) {
144153

145154
/**
146155
* Set the {@link Throwable} that caused the {@code DiscoveryIssue}.
156+
*
157+
* @param cause the {@link Throwable} that caused the
158+
* {@code DiscoveryIssue}; may be {@code null}
147159
*/
148160
Builder cause(Throwable cause);
149161

‎junit-platform-launcher/src/main/java/org/junit/platform/launcher/core/DiscoveryIssueCollector.java

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,8 +24,24 @@
2424
import org.junit.platform.engine.DiscoveryIssue.Severity;
2525
import org.junit.platform.engine.DiscoverySelector;
2626
import org.junit.platform.engine.SelectorResolutionResult;
27+
import org.junit.platform.engine.TestSource;
2728
import org.junit.platform.engine.UniqueId;
29+
import org.junit.platform.engine.discovery.ClassSelector;
30+
import org.junit.platform.engine.discovery.ClasspathResourceSelector;
31+
import org.junit.platform.engine.discovery.DirectorySelector;
32+
import org.junit.platform.engine.discovery.FileSelector;
33+
import org.junit.platform.engine.discovery.MethodSelector;
34+
import org.junit.platform.engine.discovery.PackageSelector;
2835
import org.junit.platform.engine.discovery.UniqueIdSelector;
36+
import org.junit.platform.engine.discovery.UriSelector;
37+
import org.junit.platform.engine.support.descriptor.ClassSource;
38+
import org.junit.platform.engine.support.descriptor.ClasspathResourceSource;
39+
import org.junit.platform.engine.support.descriptor.DirectorySource;
40+
import org.junit.platform.engine.support.descriptor.FilePosition;
41+
import org.junit.platform.engine.support.descriptor.FileSource;
42+
import org.junit.platform.engine.support.descriptor.MethodSource;
43+
import org.junit.platform.engine.support.descriptor.PackageSource;
44+
import org.junit.platform.engine.support.descriptor.UriSource;
2945
import org.junit.platform.launcher.LauncherConstants;
3046
import org.junit.platform.launcher.LauncherDiscoveryListener;
3147

@@ -50,6 +66,7 @@ public void selectorProcessed(UniqueId engineId, DiscoverySelector selector, Sel
5066
if (result.getStatus() == FAILED) {
5167
this.issues.add(DiscoveryIssue.builder(Severity.ERROR, selector + " resolution failed") //
5268
.cause(result.getThrowable()) //
69+
.source(toSource(selector)) //
5370
.build());
5471
}
5572
else if (result.getStatus() == UNRESOLVED && selector instanceof UniqueIdSelector) {
@@ -60,6 +77,48 @@ else if (result.getStatus() == UNRESOLVED && selector instanceof UniqueIdSelecto
6077
}
6178
}
6279

80+
static TestSource toSource(DiscoverySelector selector) {
81+
if (selector instanceof ClassSelector) {
82+
return ClassSource.from(((ClassSelector) selector).getClassName());
83+
}
84+
if (selector instanceof MethodSelector) {
85+
MethodSelector methodSelector = (MethodSelector) selector;
86+
return MethodSource.from(methodSelector.getClassName(), methodSelector.getMethodName(),
87+
methodSelector.getParameterTypeNames());
88+
}
89+
if (selector instanceof ClasspathResourceSelector) {
90+
ClasspathResourceSelector resourceSelector = (ClasspathResourceSelector) selector;
91+
String resourceName = resourceSelector.getClasspathResourceName();
92+
return resourceSelector.getPosition() //
93+
.map(DiscoveryIssueCollector::convert) //
94+
.map(position -> ClasspathResourceSource.from(resourceName, position)) //
95+
.orElseGet(() -> ClasspathResourceSource.from(resourceName));
96+
}
97+
if (selector instanceof PackageSelector) {
98+
return PackageSource.from(((PackageSelector) selector).getPackageName());
99+
}
100+
if (selector instanceof FileSelector) {
101+
FileSelector fileSelector = (FileSelector) selector;
102+
return fileSelector.getPosition() //
103+
.map(DiscoveryIssueCollector::convert) //
104+
.map(position -> FileSource.from(fileSelector.getFile(), position)) //
105+
.orElseGet(() -> FileSource.from(fileSelector.getFile()));
106+
}
107+
if (selector instanceof DirectorySelector) {
108+
return DirectorySource.from(((DirectorySelector) selector).getDirectory());
109+
}
110+
if (selector instanceof UriSelector) {
111+
return UriSource.from(((UriSelector) selector).getUri());
112+
}
113+
return null;
114+
}
115+
116+
private static FilePosition convert(org.junit.platform.engine.discovery.FilePosition position) {
117+
return position.getColumn() //
118+
.map(column -> FilePosition.from(position.getLine(), column)) //
119+
.orElseGet(() -> FilePosition.from(position.getLine()));
120+
}
121+
63122
@Override
64123
public void issueEncountered(UniqueId engineId, DiscoveryIssue issue) {
65124
this.issues.add(issue);

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

Lines changed: 54 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@
1616
import static org.junit.jupiter.api.Assertions.assertThrows;
1717
import static org.junit.jupiter.api.Assertions.assertTrue;
1818
import static org.junit.platform.engine.discovery.DiscoverySelectors.selectClass;
19-
import static org.junit.platform.launcher.core.LauncherDiscoveryRequestBuilder.request;
19+
import static org.junit.platform.engine.discovery.DiscoverySelectors.selectMethod;
2020

2121
import java.lang.reflect.Method;
2222
import java.util.EmptyStackException;
@@ -30,7 +30,8 @@
3030
import org.junit.jupiter.api.extension.ExtendWith;
3131
import org.junit.jupiter.api.extension.ExtensionContext;
3232
import org.junit.jupiter.engine.AbstractJupiterTestEngineTests;
33-
import org.junit.platform.engine.TestDescriptor;
33+
import org.junit.platform.engine.DiscoveryIssue.Severity;
34+
import org.junit.platform.testkit.engine.EngineExecutionResults;
3435
import org.junit.platform.testkit.engine.Event;
3536

3637
/**
@@ -189,6 +190,30 @@ void checkDisplayNameGeneratedForIndicativeGeneratorWithCustomSentenceFragments(
189190
);
190191
}
191192

193+
@Test
194+
void blankSentenceFragmentOnClassYieldsError() {
195+
var results = discoverTests(selectClass(BlankSentenceFragmentOnClassTestCase.class));
196+
197+
var discoveryIssues = results.getDiscoveryIssues();
198+
assertThat(discoveryIssues).hasSize(1);
199+
assertThat(discoveryIssues.getFirst().severity()).isEqualTo(Severity.ERROR);
200+
assertThat(discoveryIssues.getFirst().cause().orElseThrow()) //
201+
.hasMessage("@SentenceFragment on [%s] must be declared with a non-blank value.",
202+
BlankSentenceFragmentOnClassTestCase.class);
203+
}
204+
205+
@Test
206+
void blankSentenceFragmentOnMethodYieldsError() throws Exception {
207+
var results = discoverTests(selectMethod(BlankSentenceFragmentOnMethodTestCase.class, "test"));
208+
209+
var discoveryIssues = results.getDiscoveryIssues();
210+
assertThat(discoveryIssues).hasSize(1);
211+
assertThat(discoveryIssues.getFirst().severity()).isEqualTo(Severity.ERROR);
212+
assertThat(discoveryIssues.getFirst().cause().orElseThrow()) //
213+
.hasMessage("@SentenceFragment on [%s] must be declared with a non-blank value.",
214+
BlankSentenceFragmentOnMethodTestCase.class.getDeclaredMethod("test"));
215+
}
216+
192217
@Test
193218
void displayNameGenerationInheritance() {
194219
check(DisplayNameGenerationInheritanceTestCase.InnerNestedTestCase.class, //
@@ -273,15 +298,17 @@ void indicativeSentencesOnClassTemplate() {
273298
}
274299

275300
private void check(Class<?> testClass, String... expectedDisplayNames) {
276-
var request = request().selectors(selectClass(testClass)).build();
277-
var descriptors = executeTests(request).allEvents().started().stream() //
278-
.map(Event::getTestDescriptor) //
279-
.skip(1); // Skip engine descriptor
280-
assertThat(descriptors).map(this::describe).containsExactlyInAnyOrder(expectedDisplayNames);
301+
var results = executeTestsForClass(testClass);
302+
check(results, expectedDisplayNames);
281303
}
282304

283-
private String describe(TestDescriptor descriptor) {
284-
return descriptor.getType() + ": " + descriptor.getDisplayName();
305+
private void check(EngineExecutionResults results, String[] expectedDisplayNames) {
306+
var descriptors = results.allEvents().started().stream() //
307+
.map(Event::getTestDescriptor) //
308+
.skip(1); // Skip engine descriptor
309+
assertThat(descriptors) //
310+
.map(it -> it.getType() + ": " + it.getDisplayName()) //
311+
.containsExactlyInAnyOrder(expectedDisplayNames);
285312
}
286313

287314
// -------------------------------------------------------------------------
@@ -622,4 +649,22 @@ public String getDisplayName(int invocationIndex) {
622649
}
623650
}
624651

652+
@SuppressWarnings("JUnitMalformedDeclaration")
653+
@IndicativeSentencesGeneration
654+
@SentenceFragment("")
655+
static class BlankSentenceFragmentOnClassTestCase {
656+
@Test
657+
void test() {
658+
}
659+
}
660+
661+
@SuppressWarnings("JUnitMalformedDeclaration")
662+
@IndicativeSentencesGeneration
663+
static class BlankSentenceFragmentOnMethodTestCase {
664+
@SentenceFragment("\t")
665+
@Test
666+
void test() {
667+
}
668+
}
669+
625670
}

‎jupiter-tests/src/test/java/org/junit/jupiter/engine/AbstractJupiterTestEngineTests.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ protected EngineDiscoveryResults discoverTests(Consumer<LauncherDiscoveryRequest
7171
}
7272

7373
protected EngineDiscoveryResults discoverTests(DiscoverySelector... selectors) {
74-
return discoverTests(defaultRequest().selectors(selectors));
74+
return discoverTests(request -> request.selectors(selectors));
7575
}
7676

7777
protected EngineDiscoveryResults discoverTests(LauncherDiscoveryRequestBuilder builder) {

‎platform-tests/src/test/java/org/junit/platform/launcher/core/DiscoveryIssueCollectorTests.java

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
/*
2+
* Copyright 2015-2025 the original author or authors.
3+
*
4+
* All rights reserved. This program and the accompanying materials are
5+
* made available under the terms of the Eclipse Public License v2.0 which
6+
* accompanies this distribution and is available at
7+
*
8+
* https://www.eclipse.org/legal/epl-v20.html
9+
*/
10+
11+
package org.junit.platform.launcher.core;
12+
13+
import static org.assertj.core.api.Assertions.assertThat;
14+
import static org.junit.platform.engine.discovery.DiscoverySelectors.selectClass;
15+
import static org.junit.platform.engine.discovery.DiscoverySelectors.selectClasspathResource;
16+
import static org.junit.platform.engine.discovery.DiscoverySelectors.selectDirectory;
17+
import static org.junit.platform.engine.discovery.DiscoverySelectors.selectFile;
18+
import static org.junit.platform.engine.discovery.DiscoverySelectors.selectMethod;
19+
import static org.junit.platform.engine.discovery.DiscoverySelectors.selectPackage;
20+
import static org.junit.platform.engine.discovery.DiscoverySelectors.selectUri;
21+
import static org.mockito.Mockito.mock;
22+
23+
import java.io.File;
24+
import java.net.URI;
25+
import java.util.stream.Stream;
26+
27+
import org.junit.jupiter.params.ParameterizedTest;
28+
import org.junit.jupiter.params.provider.MethodSource;
29+
import org.junit.platform.engine.DiscoveryIssue;
30+
import org.junit.platform.engine.DiscoveryIssue.Severity;
31+
import org.junit.platform.engine.DiscoverySelector;
32+
import org.junit.platform.engine.SelectorResolutionResult;
33+
import org.junit.platform.engine.TestSource;
34+
import org.junit.platform.engine.UniqueId;
35+
import org.junit.platform.engine.discovery.FilePosition;
36+
import org.junit.platform.engine.support.descriptor.ClassSource;
37+
import org.junit.platform.engine.support.descriptor.ClasspathResourceSource;
38+
import org.junit.platform.engine.support.descriptor.DirectorySource;
39+
import org.junit.platform.engine.support.descriptor.FileSource;
40+
import org.junit.platform.engine.support.descriptor.PackageSource;
41+
import org.junit.platform.engine.support.descriptor.UriSource;
42+
43+
class DiscoveryIssueCollectorTests {
44+
45+
@ParameterizedTest(name = "{0}")
46+
@MethodSource("pairs")
47+
void reportsFailedResolutionResultAsDiscoveryIssue(Pair pair) {
48+
var collector = new DiscoveryIssueCollector(mock());
49+
var failure = SelectorResolutionResult.failed(new RuntimeException("boom"));
50+
collector.selectorProcessed(UniqueId.forEngine("dummy"), pair.selector, failure);
51+
52+
var expectedIssue = DiscoveryIssue.builder(Severity.ERROR, pair.selector + " resolution failed") //
53+
.cause(failure.getThrowable()) //
54+
.source(pair.source).build();
55+
assertThat(collector.toNotifier().getAllIssues()).containsExactly(expectedIssue);
56+
}
57+
58+
public static Stream<Pair> pairs() {
59+
return Stream.of( //
60+
new Pair(selectClass("SomeClass"), ClassSource.from("SomeClass")), //
61+
new Pair(selectMethod("SomeClass#someMethod(int,int)"),
62+
org.junit.platform.engine.support.descriptor.MethodSource.from("SomeClass", "someMethod", "int,int")), //
63+
new Pair(selectClasspathResource("someResource"), ClasspathResourceSource.from("someResource")), //
64+
new Pair(selectClasspathResource("someResource", FilePosition.from(42)),
65+
ClasspathResourceSource.from("someResource",
66+
org.junit.platform.engine.support.descriptor.FilePosition.from(42))), //
67+
new Pair(selectClasspathResource("someResource", FilePosition.from(42, 23)),
68+
ClasspathResourceSource.from("someResource",
69+
org.junit.platform.engine.support.descriptor.FilePosition.from(42, 23))), //
70+
new Pair(selectPackage("some.package"), PackageSource.from("some.package")), //
71+
new Pair(selectFile("someFile"), FileSource.from(new File("someFile"))), //
72+
new Pair(selectFile("someFile", FilePosition.from(42)),
73+
FileSource.from(new File("someFile"),
74+
org.junit.platform.engine.support.descriptor.FilePosition.from(42))), //
75+
new Pair(selectFile("someFile", FilePosition.from(42, 23)),
76+
FileSource.from(new File("someFile"),
77+
org.junit.platform.engine.support.descriptor.FilePosition.from(42, 23))), //
78+
new Pair(selectDirectory("someDir"), DirectorySource.from(new File("someDir"))), //
79+
new Pair(selectUri("some:uri"), UriSource.from(URI.create("some:uri"))) //
80+
);
81+
}
82+
83+
record Pair(DiscoverySelector selector, TestSource source) {
84+
}
85+
}

0 commit comments

Comments
 (0)